Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[PropertyAccess] fix adder/setter priority#29350

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
karser wants to merge1 commit intosymfony:3.4fromkarser:adder-vs-setter

Conversation

@karser
Copy link
Contributor

QA
Branch?3.4
Bug fix?yes
New feature?no
BC breaks?it fixes BC break introduced by#28966
Deprecations?no
Tests pass?no, so far it is a reproducer
Fixed tickets#29340
LicenseMIT

@karser
Copy link
ContributorAuthor

I reverted (locally) changes from#28966 and I'm trying to fix it differently.

@xabbuh indeed, the problem is ingetWriteAccessInfo()
Instead of checking whether the property itself is plural or single, it checks if the passed$value an array:
image
The problem is that whenisWritable is called - it doesn't have$value and an empty array is passed, just as a stub.
image

I added a check if the property name plural. So it uses adder/remover only if the property name is actual plural.

That is -$camelized preserves its original form (plural of singular),$singluars doesn't have plural by design.
So if$singulars contain the original passed value - we 100% sure the singular is meant here. And vice versa. So we can't use it for adder/remover because plural is required here.
Tests pass locally.

@xabbuh@nicolas-grekas makes sense?

image

@xabbuh
Copy link
Member

my idea to fix the issue would be#29355

@karser
Copy link
ContributorAuthor

@xabbuh I see - it shouldn't cache thegetWriteAccessInfo result without taking into account$value.
But still it doesn't distinguish, for example,isPropertyWritable('email') andisPropertyWritable('emails') they both will be callinggetWriteAccessInfo with an empty array argument. I think that the property name should tell whether it's plural or singular.

What do you think of this approach?

$plural = empty(array_intersect(array($camelized), $singulars));$useAdderAndRemover = $plural && (\is_array($value) || $value instanceof \Traversable)

antograssiot added a commit to antograssiot/core that referenced this pull requestNov 27, 2018
antograssiot added a commit to antograssiot/core that referenced this pull requestNov 27, 2018
antograssiot added a commit to antograssiot/core that referenced this pull requestNov 27, 2018
@nicolas-grekasnicolas-grekas changed the title[PropertyAccess] Added reproducer for adder/setter priority #29340 #28966[PropertyAccess] fix adder/setter priorityNov 29, 2018
$camelized =$this->camelize($property);
$singulars = (array) Inflector::singularize($camelized);

if ($useAdderAndRemover && !\in_array($camelized,$singulars)) {
Copy link
Member

Choose a reason for hiding this comment

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

unfortunately, this does not work for words where singular and plural are the same (aircraft for example)

Copy link
Member

Choose a reason for hiding this comment

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

see the added test in#29355

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, there is a lot of exceptionsThat are Both Plural and Singular. Although I'm still confused why an empty array is passed togetWriteAccessInfo(). Just to implicitly allow adder/remover?

Copy link
Member

Choose a reason for hiding this comment

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

Probably something like that, though I don't know for sure. Maybe the git history reveals some more background information.

@nicolas-grekas
Copy link
Member

Closing in favor of#29355, thanks for your help!

karser and xabbuh reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh left review comments

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

4 participants

@karser@xabbuh@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp