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

PropertyInfoLoader should not try to add validation to non-existent property #31936

Conversation

@weaverryan
Copy link
Member

weaverryan commented Jun 7, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31796 (see #31796 (comment))
License MIT
Doc PR not needed

With auto-validation, if a class has a setter (e.g. setFoo()) but there is no foo property, it still tries to add validation to that property, resulting in a:

Property "foo" does not exist in class "App\Entity\Bar

This fixes that. I believe it's "just this simple", but I don't have any experience with the code in this area yet.

Cheers!

@weaverryan weaverryan force-pushed the weaverryan:no-auto-validation-on-non-existent-prop branch from 4af9fa1 to 89f72a1 Jun 7, 2019
@xabbuh
xabbuh approved these changes Jun 7, 2019
@@ -60,6 +60,10 @@ public function loadClassMetadata(ClassMetadata $metadata)
continue;
}

if (false === property_exists($className, $property)) {

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 7, 2019

Author Member

property_exists() is what is used in PropertyMetadata, which triggered the exception. This matches that.

@@ -46,6 +46,7 @@ public function testLoadClassMetadata()
'alreadyMappedNotBlank',
'alreadyPartiallyMappedCollection',
'readOnly',
'nonExistentField',

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 7, 2019

Author Member

No asserts were added, but, without the fix, these changes are enough to trigger an exception to be thrown while running this test.

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Jun 7, 2019
@weaverryan weaverryan force-pushed the weaverryan:no-auto-validation-on-non-existent-prop branch from 89f72a1 to b702598 Jun 7, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jun 7, 2019

Thank you @weaverryan.

@nicolas-grekas nicolas-grekas merged commit b702598 into symfony:4.3 Jun 7, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
nicolas-grekas added a commit that referenced this pull request Jun 7, 2019
…-existent property (weaverryan)

This PR was merged into the 4.3 branch.

Discussion
----------

PropertyInfoLoader should not try to add validation to non-existent property

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31796 (see #31796 (comment))
| License       | MIT
| Doc PR        | not needed

With auto-validation, if a class has a setter (e.g. `setFoo()`) but there is no `foo` property, it still tries to add validation to that property, resulting in a:

> Property "foo" does not exist in class "App\Entity\Bar

This fixes that. I believe it's "just this simple", but I don't have any experience with the code in this area yet.

Cheers!

Commits
-------

b702598 Fixing bug where PropertyInfoLoader tried to add validation to non-existent properties
@weaverryan weaverryan deleted the weaverryan:no-auto-validation-on-non-existent-prop branch Jun 7, 2019
@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Jun 8, 2019

Looks good to me! Thank you Ryan.

@fabpot fabpot mentioned this pull request Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.