Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a deprecation warning #5058

Merged
merged 1 commit into from Sep 27, 2017
Merged

Fixed a deprecation warning #5058

merged 1 commit into from Sep 27, 2017

Conversation

thierryba
Copy link
Contributor

@thierryba thierryba commented Apr 7, 2017

Simple fix that's using deprecated django api.
was getting "/usr/local/filewave/python/lib/python3.6/site-packages/djangorestframework-3.6.1-py3.6.egg/rest_framework/fields.py:1774: RemovedInDjango20Warning: Usage of ForeignObjectRel.to attribute has been deprecated. Use the model attribute instead."

@tomchristie
Copy link
Member

tomchristie commented Apr 7, 2017

Seems valid to me, yup.

I'm slightly wary of introducing this change in a minor version, in case there's any unexpected breakages with some older, but still supported, versions of Django.

Opinions from anyone else?

@tomchristie tomchristie added this to the 3.6.3 Release milestone Apr 7, 2017
@rpkilby
Copy link
Member

rpkilby commented Apr 7, 2017

Yep - I'm pretty sure that this would break django 1.8 (see django-filter compat). It looks like to_internal_value isn't covered by the tests.

@tomchristie
Copy link
Member

tomchristie commented Apr 27, 2017

Have re-milsetoned a few weeks ago, but just to make it explicit:

We'll aim to merge this in for 3.7.0.

Thanks so much for the fix. Good catch!

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 20, 2017

Django 1.8 will still be with us for a while. I think we need a compat shim (and test) as per @rpkilby's comment. Not a biggie. Worth having.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Needs shim and test as per comment.

@rpkilby
Copy link
Member

rpkilby commented Sep 20, 2017

btw, the current recommendation in the 2.0 release notes is to drop support for versions below 1.11

https://docs.djangoproject.com/en/dev/releases/2.0/#third-party-library-support-for-older-version-of-django

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 20, 2017

There's a good question as to when we drop what, i.e. do we really intend to maintain Python 2.7 compat right until 1.11 is EOL (?) , but I think we need to keep 1.8 for now.

@rpkilby
Copy link
Member

rpkilby commented Sep 20, 2017

Yeah, dropping Django 1.8 support should probably coincide with adding Django 2.0 support.

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 22, 2017

@thierryba Can I ask how you triggered the warning here?

I'm trying to reproduce this in a minimal test case and can't quite see how you'd end up in the if rel is not None branch.

E.G. If you're using a ForeignKey, you'd either get a PrimaryKeyRelatedField or specify a nested serializer.

So, what's your setup such that you hit this? Thanks!

@thierryba
Copy link
Contributor Author

thierryba commented Sep 22, 2017

@carltongibson I actually cannot reproduce it any more. I've obviously switched to newer versions of all 3rdparty including the latest django rest framework. Sorry.

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 22, 2017

@thierryba OK. Thanks for the follow-up.

@encode/django-rest-framework-core-team Can someone check my reasoning here... — I don't think ModelField will ever be relational:

First we fetch the target field info:

def get_field_info(model):
"""
Given a model class, returns a `FieldInfo` instance, which is a
`namedtuple`, containing metadata about the various field types on the model
including information about their relationships.
"""
opts = model._meta.concrete_model._meta
pk = _get_pk(opts)
fields = _get_fields(opts)
forward_relations = _get_forward_relationships(opts)
reverse_relations = _get_reverse_relationships(opts)
fields_and_pk = _merge_fields_and_pk(pk, fields)
relationships = _merge_relationships(forward_relations, reverse_relations)
return FieldInfo(pk, fields, forward_relations, reverse_relations,
fields_and_pk, relationships)

Then build_field uses this, as the info parameter:

def build_field(self, field_name, info, model_class, nested_depth):
"""
Return a two tuple of (cls, kwargs) to build a serializer field with.
"""
if field_name in info.fields_and_pk:
model_field = info.fields_and_pk[field_name]
return self.build_standard_field(field_name, model_field)
elif field_name in info.relations:
relation_info = info.relations[field_name]
if not nested_depth:
return self.build_relational_field(field_name, relation_info)
else:

But question: won't a relational field always end up coming out of build_relational_field — which never returns ModelField?

If so then the correct fix here is to just delete the whole if rel is not None branch.

  1. What did I miss?
  2. How do I construct a simple test case for that?

Ta!

@tomchristie
Copy link
Member

tomchristie commented Sep 25, 2017

If so then the correct fix here is to just delete the whole if rel is not None branch.

It's not clear if that's viable without more digging.

As it happens we're rolling the 3.7 release, and I think it'd be very reasonable for use to move to "1.9, 1.10, 1.11, 2.0" as our supported targets. I think the path of least resistance will be to update our version support, and then merge this in.

@pszpetkowski
Copy link

pszpetkowski commented Sep 26, 2017

@tomchristie
Just my two cents:

  • I'd keep 1.8 support until extended support ends in April 2018 (since it is an LTS version)
  • There is no reason to support Django 1.9 since it has already gone EOL since April
  • Django 1.10 will go EOL in December, so it might be a good idea to convince users to migrate to 1.11

In my opinion there is little reason to provide support for versions of Django which are not supported, because they do not even get security fixes and so, there is no point in using them in production. Some people however might need a bit more time to make a leap from 1.8 to 1.11.

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 27, 2017

@piotr-szpetkowski Given 1.8s LTS status I had thought to keep it for this release.

As @rpkilby noted, once Django 2.0 is released we'll be making 1.11 the minimum supported version (following the guidelines). This will be despite the fact that 1.8 will not then be EOL.

Given the hoops we need to jump through here we're just going to bring that change forward. (Though I think we can still keep 1.10 for now...)

Django REST Framework 3.6 will continue to function with Django 1.8. People stuck on 1.8 can pin to that.

The bottom line is that if you're on Django 1.8 your time is much better spent updating Django than on updating Django REST Framework. (If you're on 1.8 you can't claim a concern for the latest features...)

Similar reasoning will apply soon enough when we think about dropping support for Python 2.7. If you're on Python 2.7 your time is much better spent updating Python than on updating Django REST Framework.

Copy link
Collaborator

@carltongibson carltongibson left a comment

OK, with #5457 coming in, we'll take this as is.

@thierryba Thanks for the input! 👍

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 27, 2017

This will be despite the fact that 1.8 will not then be EOL.

To be honest, it's ok to drop 1.8.
if a project is active it should switch away from it and it wouldn't be an issue. If it's not active, there's no reason DRF needs to be upgraded anyway.

@carltongibson carltongibson merged commit 760268a into encode:master Sep 27, 2017
1 check passed
carltongibson added a commit that referenced this pull request Sep 27, 2017
Remove Django 1.8 & 1.9 from README and setup.py
carltongibson added a commit that referenced this pull request Sep 27, 2017
Remove Django 1.8 & 1.9 from README and setup.py
carltongibson added a commit that referenced this pull request Sep 28, 2017
Remove Django 1.8 & 1.9 from README and setup.py
carltongibson added a commit that referenced this pull request Oct 5, 2017
Remove Django 1.8 & 1.9 from README and setup.py
carltongibson added a commit that referenced this pull request Oct 5, 2017
Remove Django 1.8 & 1.9 from README and setup.py
carltongibson pushed a commit that referenced this pull request Oct 6, 2017
* Set version number for 3.7.0 release

* Rename release notes section

Moved issue links to top for easier access.
(Can move back later)

* Add release note for #5273

* Add release note for #5440

* Add release note for #5265

Strict JSON handling

* Add release note for #5250

* Add release notes for #5170

* Add release notes for #5443

* Add release notes for #5448

* Add release notes for #5452

* Add release not for #5342

* Add release notes for 5454

* Add release notes for #5058 & #5457

Remove Django 1.8 & 1.9 from README and setup.py

* Release notes for merged 3.6.5 milestone tickets

Tickets migrated to 3.7.0 milestone.

* Add release notes for #5469

* Add release notes from AM 2ndOct

* Add final changes to the release notes.

* Add date and milestone link

Move issue links back to bottom.

* Update translations from transifex

* Begin releae anouncement

* Add release note for #5482

* 3.7 release announcement & related docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants