8000 Use the PropertyAccessor component instead of our custom solution by javiereguiluz · Pull Request #625 · EasyCorp/EasyAdminBundle · GitHub
[go: up one dir, main page]
Skip to content

Use the PropertyAccessor component instead of our custom solution #625

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

Use the PropertyAccessor component instead of our custom solution #625

wants to merge 9 commits into from

Conversation

javiereguiluz
Copy link
Collaborator

No description provided.

@yceruto
Copy link
Collaborator
yceruto commented Dec 8, 2015

@javiereguiluz I guess this leads to improved performance too? symfony/symfony#16838

@javiereguiluz
Copy link
Collaborator Author

@yceruto probably. And less lines of code for us, less bugs, less maintenance, etc. But first, let's try to make tests green for this change 😄

ogizanagi
ogizanagi reviewed Dec 9, 2015
View reviewed changes
Configuration/Configurator.php
$fieldConfiguration['isReadable'] = $getter || $isPublic;
$fieldConfiguration['isWritable'] = $setter || $isPublic;
$fieldConfiguration['isReadable'] = $this->accessor->isReadable($instance , $fieldName);
$fieldConfiguration['isWritable'] = $this->accessor->isWritable($instance, $fieldName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, those methods were added in 2.5 only :/
We should have a BC layer temporary until 2.3 support is dropped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are really bad news 😭 I'll add that BC layer then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Create a decorator for the PropertyAccessor, it should be the best solution.

@ogizanagi
Copy link
Contributor

I'm all in for this ! 👍

@Pierstoval
Copy link
Contributor

👍 , it makes me want time to go faster for 2.3 to reach end of life, to avoid all these workarounds 😛

@javiereguiluz
Copy link
Collaborator Author
8000

I've refactored this code completely. It now supports 2.3, but I haven't added any BC layer. The problem is divided into two parts: read a value and set a value.

Before we had an introspectGettersAndSetters() that set isReadable, isWritable, getter, setter, etc. for each property of each entity and for each view. This was necessary because we used a custom solution instead of the PropertyAccessor component. Now I've removed everything.

  1. When reading a value, we wrap the readValue() method in a try ... catch and we display the usual Inaccessible label for values that cannot be read. When we drop support for 2.3, we can replace the try .. catch by a proper if .. $accessor->isReadable(...)
  2. When writing a value (this only happens in the controller), we don't check if isWritable and we don't wrap it in a try .. catch That's because in this case we wan't to fail hard. Otherwise, if we fail silently, the user may think that the value was modified. If you try to edit a value which is not writable, the app should fail in a clear way.

This change simplifies everything and shaves hundreds of lines of code. Thoughts?

ogizanagi
ogizanagi reviewed Dec 10, 2015
View reviewed changes
Tests/PropertyAccessor/PropertyAccessorTest.php
$accessor = new PropertyAccessor();
$object = new FooBarClass();

$this->assertSame($expectedResult, $accessor->isReadable($object, $property));
Copy link
Contributor

Choose a reason for hiding this comment

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

Still not available as of 2.3 ^^'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch. Fixed (with a markTestSkipped for 2.3).

@ogizanagi
Copy link
Contributor

@javiereguiluz : Those changes are fine to me, especially as it simplifies things a lot. But someone might have relied one those metadata. So this is potentially a BC break I guess.
I would have opted for a decorated PropertyAccessor however, as suggested by @Pierstoval, in order to avoid this try/catch over a simple if, and smooth the upgrade path when we'll drop 2.3 support.
I agree about hard failing when writing values.

👍

ogizanagi
ogizanagi reviewed Dec 10, 2015
View reviewed changes
Tests/PropertyAccessor/PropertyAccessorTest.php
$accessor = new PropertyAccessor();
$object = new FooBarClass();

if (!method_exists($accessor, 'isReadable')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CS (indentation).
However, if we completely rely on the Symfony PropertyAccessor, without decorating/modifying it, do we have to test it at all ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I know we should remove this test. I maintained it because we are doing the transition and I wanted to be sure that it works as we expect. Should I remove it this very same pull request?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. Good idea in fact to keep it until now. As soon as the PR is ready to be merged, it can be removed 😃

@javiereguiluz
Copy link
Collaborator Author

Fixed all the details and added a note in the UPGRADE file (in c7c6331)

@javiereguiluz javiereguiluz mentioned this pull request Dec 10, 2015
Merged
javiereguiluz added a commit that referenced this pull request Dec 10, 2015
@javiereguiluz
minor #627 Removed an unneeded test (javiereguiluz)
36c8120 
This PR was merged into the master branch.

Discussion
----------

Removed an unneeded test

Once #625 is merged, we no longer need this test (PropertyAccessor component is already tested).

And it allows as to remove more than 400 lines of code!

Commits
-------

bdc21f8 Removed an unneeded test
Pierstoval
Pierstoval reviewed Dec 11, 2015
View reviewed changes
UPGRADE.md
@@ -27,8 +41,8 @@ Upgrade to 1.9.2
the value of boolean properties. It has been replaced by a private method
called `updateEntityProperty()`.

Upgrade to 1.8.0
----------------
Upgrade to 1.8.0 (8/November/2015)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like having the / in a full date when the month is written in letters.
Either put 08/11/2015 or (better I think) put November 8th 2015

@javiereguiluz javiereguiluz deleted the use_propertyaccessor branch December 13, 2015 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@javiereguiluz @yceruto @ogizanagi @Pierstoval
0