8000 [PropertyAccessor] Fix unable to write to singular property using setter while plural adder/remover exist by karser · Pull Request #28962 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
326da00
[PropertyAccessor] Fix unable to write to singular property using set…
karser Oct 23, 2018
4dcc11d
comply with the project coding standards
karser Oct 23, 2018
91983f7
[PropertyAccessor] isWritable is not a guard but helps to reproduce t…
karser Oct 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
comply with the project coding standards
  • Loading branch information
@karser
karser committed Oct 23, 2018
commit 4dcc11df1aaa89610caadbd41dec39412eaacc55
1 change: 0 additions & 1 deletion src/Symfony/Component/PropertyAccess/PropertyAccessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,6 @@ private function getWriteAccessInfo(string $class, string $property, $value): ar
$camelized = $this->camelize($property);
$singulars = (array) Inflector::singularize($camelized);


if (!isset($access[self::ACCESS_TYPE])) {
$setter = 'set'.$camelized;
$getsetter = lcfirst($camelized); // jQuery style, e.g. read: last(), write: last($item)
Expand Down
10 changes: 5 additions & 5 deletions src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestSingularAndPluralProps.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,26 @@

/**
* Notice we don't have getter/setter for emails
* because we count on adder/remover
* because we count on adder/remover.
*/
class TestSingularAndPluralProps
{
/** @var string|null */
private $email;

/** @var array */
private $emails = [];
private $emails = array();

/**
* @return null|string
* @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

{
return $this->email;
}

/**
* @param null|string $email
* @param string|null $email
*/
public function setEmail(?string $email): void
{
Expand Down Expand Up @@ -60,6 +60,6 @@ public function addEmail(string $email): void
*/
public function removeEmail(string $email): void
{
$this->emails = array_diff($this->emails, [$email]);
$this->emails = array_diff($this->emails, array($email));
}
}
6 changes: 3 additions & 3 deletions src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestClassMagicGet;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestClassSetValue;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestClassTypeErrorInsideCall;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestSingularAndPluralProps;
use Symfony\Component\PropertyAccess\Tests\Fixtures\Ticket5775Object;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TypeHinted;
use Symfony\Component\PropertyAccess\Tests\Fixtures\TestSingularAndPluralProps;

class PropertyAccessorTest extends TestCase
{
Expand Down Expand Up @@ -694,10 +694,10 @@ public function testWriteToPluralPropertyWhileSingularOneExists()
$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');

$this->propertyAccessor->setValue($object, 'emails', ['test@email.com']);
$this->propertyAccessor->setValue($object, 'emails', array('test@email.com'));
}

self::assertEquals(['test@email.com'], $object->getEmails());
self::assertEquals(array('test@email.com'), $object->getEmails());
self::assertNull($object->getEmail());
}
}
0