Fixed #24716 -- Deprecated Field._get_val_from_obj()

[ovangle]

The method duplicates the functionality of Field.value_from_object
and has the additional downside of being a privately named public
API method.

All usage of the method in django.* libraries have been replaced
with a call to getattr, to reduce the overhead of an additional,
unecessary function call.

No unittests are added, since this is just the removal of a single method,
which was already amply tested by the serialization tests.

[timgraham]

It doesn't seem best to me to recommend using value_from_object() in the docs but use getattr in the code. Yes, we avoid a function call, but it may make the fields less extensible. It would be great if we could test some third-party fields and see how they are affected by the change.

There is also the issue that if a custom field is overriding _get_val_from_obj() it will no longer be called in all the places Django makes use of it.

Please see the Deprecating a Feature checklist. In particular, we need documentation. A Trac ticket should also be created to track the change. Thanks.

[ovangle]

I fail to see how it makes anything less extensible -- _get_val_from_obj was never a hook to customise attribute access and it was only ever called by the various implementations of value_to_string. There is nothing you could customise by overriding it that you couldn't already implement as part of value_to_string.

I wanted to remove value_from_object and save_form_data as well (which is why I reimplemented using getattr) for redundancy reasons, but figured some people might think that having an alternative to getattr/setattr is more friendly to new users. Which it is, and they don't suffer from the additional problem of having "private" method names.

As far as the discrepancy between the documentation and the implementation goes, getattr is more consistent with the rest of the django.models.fields package. The micro-optimisation of the additional function call saves nothing, since django is IO bound.

Trac ticket #24716
I've also added deprecation details to the relevant release documentation.

[timgraham]

Here are examples of people overriding _get_val_from_obj(). Maybe it's okay to break backwards compatibility if the gains are sufficient, but if so, we need to document how to adapt existing code that does so.

https://github.com/search?utf8=%E2%9C%93&q=%22def+_get_val_from_obj%22&type=Code&ref=searchresults

[ovangle]

I took a quick browse through the first page of results, they're all identical, copy_pasted from appengine_django, and none of them override _get_val_from_obj, they ducktype it out. All of them also ducktype value_to_string, so they can't be calling _get_val_from_obj anywhere anyway.

Whether the "gains" are clear enough, I can't say. As it stands there are two identical methods which both do exactly the same (useless and redundant) thing, one of which is (unfortunately) named as a private method and should be removed.

The migration path is simple -- rename all calls to _get_val_from_obj with value_from_object. It's right there in both the deprecation warnings and is pretty damn straightforward.

[ovangle]

And just looked through the next five pages of results. All exactly the same c+v code. Why the hell do people do that? Argh.

[timgraham]

will be removed

[timgraham]

I think using value_from_object() rather than getattr() results in better readability. Especially since people may copy from Django's fields, I would still prefer to follow the documentation and use that method in our custom fields.

[ovangle]

I understand that, but internal consistency and readability is important too. 99% of all field values are fetched using getattr within the django.db.fields api. It took me a while when looking into the code when I first started working on the composite fields stuff to realise that _get_val_from_obj and value_from_object did absolutely nothing special.

(which incidentally, is the reason I wanted to change it in the first place -- readability).

And what does it matter if the user c+vs the getattr method for attribute access from django core code? It's identical to the method call and getattr should be readable enough for anybody who knows the python builtins and their semantics.

getattr(model, field.attname)

"Get the attribute from the model, using the attname of the field". Pretty readable as far as I'm concerned. Using field.attname rather than field.name is the only slightly confusing part. But I'm hoping that we will be able to remove the name vs. attname distinction once the composite fields work is done (but that isn't planned as part of the initial work).

[timgraham]

I guess I would be less averse if Field.attname were formally documented. "Field attribute reference" in docs/ref/models/fields.txt seems like a good place.

[ovangle]

I made the suggested documentation changes and documented Field.attname. I've left custom-model-fields.txt (largely) as is, because I do think that code outside the fields package should use value_from_object.