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

Use propertyAccessor for BaseFieldDescription::getFieldValue #6798

Merged
merged 1 commit into from
Jan 24, 2021

Conversation

VincentLanglet
Copy link
Member

@VincentLanglet VincentLanglet commented Jan 22, 2021

It will solve #6795 and we will rely on propertyAccessor implementation instead of a custom one.

We're loosing the parameters option but I'm not sure it's used and it's solvable by using a custom template instead.

Changelog

### Deprecated
- FieldDescription `parameters` option.

@VincentLanglet VincentLanglet force-pushed the propertyAccessor branch 2 times, most recently from e754451 to 8778aeb Compare January 22, 2021 12:46
@VincentLanglet VincentLanglet requested a review from a team January 22, 2021 12:50
Copy link
Member

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need property access as composer dep?

@VincentLanglet
Copy link
Member Author

Do we need property access as composer dep?

We already have

@VincentLanglet VincentLanglet requested a review from a team January 22, 2021 13:19
@franmomu
Copy link
Member

If we use the PropertyAccessor, what about to replace almost everything in getFieldValue?

if ($this->isVirtual() || null === $object) {
return null;
}

I think (without trying it) all code below those lines can be replace using PropertyAccessor::getValue

@VincentLanglet
Copy link
Member Author

If we use the PropertyAccessor, what about to replace almost everything in getFieldValue?

if ($this->isVirtual() || null === $object) {
return null;
}

I think (without trying it) all code below those lines can be replace using PropertyAccessor::getValue

If I replace everything, I lost the code option then.

@VincentLanglet VincentLanglet merged commit a82675c into sonata-project:3.x Jan 24, 2021
@VincentLanglet VincentLanglet deleted the propertyAccessor branch January 24, 2021 20:38
@franmomu
Copy link
Member

This PR introduces a BC break since code is always set in the current persistence bundles.

@VincentLanglet
Copy link
Member Author

This PR introduces a BC break since code is always set in the current persistence bundles.

@franmomu Do you have a use case ?
The code option should be still supported.

@franmomu
Copy link
Member

franmomu commented Feb 14, 2021

Apparently it is always like: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/b656ccab02db9abf397e04a69ca0e2bda9bfd455/src/Builder/ListBuilder.php#L127

The problem I guess is that before, code option added a getter option to try to fetch the value, but now if is set and does not exists it only executes: return $object->{$getter}();.

Edit: sonata-project/SonataDoctrineORMAdminBundle#1306

@VincentLanglet
Copy link
Member Author

I made #6855

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.

4 participants