Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#21940 closed Cleanup/optimization (fixed)

Consistent contribute_to_class's virtual_only

Reported by: German M. Bravo Owned by: nobody
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: mmitar@…, jernej@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I needed to create a field like class for my django project and I wanted it to be a virtual field... virtual_only is there for that reason, but my field was inheriting from FileField, and FileField.contribute_to_class() doesn't take virtual_only and so it doesn't passes it to Field.contribute_to_class(). I believe passing virtual_only attribute should be consistent across the source code. I'm attaching a simple patch to fix this. One warning would be for custom fields made by users which have their own contribute_to_class (those will need to be changed to properly receive the virtual_only parameter as optional.)

Attachments (1)

#21940-virtual_only_in_contribute_to_class.diff (4.1 KB) - added by German M. Bravo 3 years ago.

Download all attachments as: .zip

Change History (11)

Changed 3 years ago by German M. Bravo

comment:1 Changed 3 years ago by Tim Graham

Would it be better to simply accept and pass on **kwargs? I think that would eliminate the minor backwards compatibility concerns as well.

comment:2 Changed 3 years ago by Tim Graham

Component: UncategorizedDatabase layer (models, ORM)
Has patch: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:3 Changed 3 years ago by ANUBHAV JOSHI

IMO what is done is better, because

In the contribute_to_class in Field class:

def contribute_to_class(self, cls, name, virtual_only=False)

If from subclass:

super(AutoField, self).contribute_to_class(cls, name, **kwargs)

There is only virtual_only as keyword argument, if we pass **kwargs then it will be possible to pass any other variable to it as well, which is wrong I believe.

Last edited 3 years ago by ANUBHAV JOSHI (previous) (diff)

comment:4 Changed 2 years ago by Mitar

Cc: mmitar@… added

comment:6 Changed 2 years ago by Jernej Kos

Cc: jernej@… added

comment:8 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 1ed6fbcf44f0eebf29fd9b36be03bc5d73acee0a:

Fixed #21940 -- Added kwargs to contribute_to_class() of model fields..

Thanks Kronuz for the suggestion.

comment:9 Changed 2 years ago by Gavin Wahl

Can this be backported to 1.6 and 1.7?

comment:10 Changed 2 years ago by Carl Meyer

Definitely not 1.6, it's in security-fixes-only mode. And I don't think this meets the criteria in our backporting policy for backport to 1.7 either.

Last edited 2 years ago by Carl Meyer (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top