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

[PropertyAccessor] Fix unable to write to singular property using setter while plural adder/remover exist #28962

Closed

Conversation

karser
Copy link
Contributor

@karser karser commented Oct 23, 2018

Q A
Branch? 3.4, 4.1 and master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28961
License MIT

Please take a look at the tests I added - they describe the issue. It has to do with the priorities: findAdderAndRemover('User', 'email') is called earlier than $this->isMethodAccessible('User', 'setEmail', 1)

{
$object = new TestSingularAndPluralProps();

if ($this->propertyAccessor->isWritable($object, 'emails')) {
Copy link
Member

Choose a reason for hiding this comment

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

instead of having this if, I would have $this->assertTrue($this->propertyAccessor->isWritable($object, 'emails'). We want the property to be writable in this test (and same thing for the previous test).

and maybe we should even write without guarding the call at all (so that if the property is not writable, we get the detailed error message in the failure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isWritable is not a guard here but necessary condition for reproducing this issue.
isWritable internally caches the info to writePropertyCache with 3rd argument as array getWriteAccessInfo(\get_class($object), $property, array()); which is used by findAdderAndRemover.
Do you still think assertTrue is needed for isWritable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't call isWritable - then getWriteAccessInfo will be called by writeProperty with 3rd argument as string: $this->getWriteAccessInfo(\get_class($object), $property, $value); and findAdderAndRemover will not be called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe this would be better?

$this->propertyAccessor->isWritable($object, 'email'); //cache access info
$this->propertyAccessor->setValue($object, 'email', 'test@email.com');

@karser
Copy link
Contributor Author

karser commented Oct 23, 2018

The error on appveyor has nothing to do with the changes I made:

There was 1 error:
1) Symfony\Component\Process\Tests\ProcessTest::testWaitUntilSpecificOutput
Symfony\Component\Process\Exception\ProcessTimedOutException: The process "c:\php\php.exe "C:\projects\symfony\src\Symfony\Component\Process\Tests/KillableProcessWithOutput.php"" exceeded the timeout of 60 seconds.

@nicolas-grekas nicolas-grekas changed the title [PropertyAccessor] Fix unable to write to singular property using set… [PropertyAccessor] Fix unable to write to singular property using setter while plural adder/remover exist Oct 24, 2018
/**
* @return string|null
*/
public function getEmail(): ?string
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the PR.
If the issue exists on 3.4, can you please rebase+retarget your PR for branch 3.4?
This will require writing it using PHP5.5 compatible code (that's why I'm commenting on this line :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolas-grekas I made my changes php5 compatible in a new php5 branch.
How do I rebase the branch of this PR with the php5 branch?

Copy link
Member

Choose a reason for hiding this comment

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

maybe close this PR and submit a new one would be the easiest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sent my changes in another PR #28966

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolas-grekas but still in general, what is the correct git flow for this situation? That is, rebase and squash my changes in order to prepare for PR?

Copy link
Member

Choose a reason for hiding this comment

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

rebase the branch (without creating a new one) using git rebase -i
then retarget using the "Edit" above

@nicolas-grekas
Copy link
Member

Replaced by #28966

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants