Opened 19 months ago

Closed 13 months ago

Last modified 10 months ago

#21940 closed Cleanup/optimization (fixed)

Consistent contribute_to_class's virtual_only

Reported by: Kronuz 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 Kronuz 19 months ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 19 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 19 months ago by timo

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Has patch set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

comment:3 Changed 18 months ago by anubhav9042

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 18 months ago by anubhav9042 (previous) (diff)

comment:4 Changed 14 months ago by mitar

  • Cc mmitar@… added

comment:6 Changed 14 months ago by kostko

  • Cc jernej@… added

comment:8 Changed 13 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 1ed6fbcf44f0eebf29fd9b36be03bc5d73acee0a:

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

Thanks Kronuz for the suggestion.

comment:9 Changed 10 months ago by gavinwahl

Can this be backported to 1.6 and 1.7?

comment:10 Changed 10 months ago by carljm

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 10 months ago by carljm (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top