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

Allow null values as argument 1 for FieldDescriptionInterface::getFieldValue() #4998

Merged

Conversation

phansys
Copy link
Member

@phansys phansys commented Mar 6, 2018

I am targeting this branch, because this is BC fix.

Changelog

### Fixed
- `getFieldValue` from `BaseFieldDescription` can now handle getting a value from null objects.

Subject

As context, I must mention the scenario in which the received subject is null.
I have a simple set of geo entities (City, State, Country) which have many-to-one association respectively (a country may have n states, a state may have n cities). I use these entities within others, like User. The association between User and City is many-to-one and is NULLABLE. So, in my UserAdmin::configureShowFields() I have the following call:

ShowMapper::add('city.state', EntityType::class, ['class' => State::class])

then, when there is no city for a user, value passed to BaseFieldDescription::getFieldValue() is null.
From #4947, with the described situation, this method throws the mentioned exception, since get_class() receives null as argument.


If you want to contribute with another approach for this fix, please let me know.

@phansys phansys force-pushed the get_field_value_with_null_object branch from 1b5ac1d to 7b985d3 Compare March 6, 2018 15:04
@kunicmarko20
Copy link
Contributor

Please write a changelog directed at end users. Keep in mind that they probably do not care what file or class you changed, but what features you provided / what bugs you fixed. Try to answer the question "What benefit do I get from upgrading?"
More on http://keepachangelog.com

@phansys
Copy link
Member Author

phansys commented Mar 6, 2018

IMO, the provided title plus the Fixed section at CHANGELOG is enough to end users to know which issue is this PR resolving, but I'll try to reword Subject section in order to add the context provided at #4947 (review) directly here.

@jordisala1991
Copy link
Member

jordisala1991 commented Mar 6, 2018

@kunicmarko20 is not talking about what a user can see if it gets into this PR.

The changelog goes automatically to this file (you don't have to change anything): https://github.com/sonata-project/SonataAdminBundle/blob/3.x/CHANGELOG.md

Reading this on the changelog An exception has been thrown during the rendering of a template ("Warning: get_class() expects parameter 1 to be object, null given"). gives you 0 idea of what is fixed, because that could happen anywhere on the project.

I reworded a bit your changelog (keeping your old one commented). It could be better, but it is a step.

@greg0ire greg0ire merged commit 4783e09 into sonata-project:3.x Mar 7, 2018
@greg0ire
Copy link
Contributor

greg0ire commented Mar 7, 2018

Thanks @phansys

@phansys phansys deleted the get_field_value_with_null_object branch March 7, 2018 11:06
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

5 participants