NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[PropertyInfo][Serializer] Fixed extracting ignored properties for Serializer#39299

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

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:5.1fromjaver:fix-extracting-ignored-properties-for-serializer

Dec 10, 2020

Merged

[PropertyInfo][Serializer] Fixed extracting ignored properties for Serializer#39299

nicolas-grekas merged 1 commit intosymfony:5.1fromjaver:fix-extracting-ignored-properties-for-serializer

Dec 10, 2020

Conversation

Copy link

Contributor

javer commentedDec 3, 2020•
edited
Loading

Q	A
Branch?	5.1
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

Fixed typo inSerializerExtractor::getProperties() introduced in8526d7c which leads to the error after#37040.
$serializerClassMetadata is instance ofSymfony\Component\Serializer\Mapping\ClassMetadata, which doesn't containisIgnored method, this methods is located inSymfony\Component\Serializer\Mapping\AttributeMetadata which is$serializerAttributeMetadata here. More over, it doesn't make sense to check the method existence in one class and call it for another.

javer requested a review fromdunglas as acode owner

December 3, 2020 12:08

carsonbot added Status: Needs Review PropertyInfo Serializer Bug labels

Dec 3, 2020

Copy link

carsonbot commentedDec 3, 2020

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

carsonbot changed the title~~[PropertyInfo] [Serializer] Fixed extracting ignored properties for Serializer~~[PropertyInfo][Serializer] Fixed extracting ignored properties for Serializer

Dec 3, 2020

Copy link

ContributorAuthor

javer commentedDec 3, 2020

It seems thatPropertyInfo Tests is not the appropriate place to write this test because of the dependency onSerializer component annotation. ButSerializer component doesn't useSerializerExtractor, it usesAbstractNormalizer which contains duplicated logic of handling ignored properties which is correct there. That's whySerializer tests are green despite the fact that mistake is obvious and this annotation doesn't work for those who usePropertyInfoExtractor directly. For example, it isObjectModelDescriber ofnelmio/api-doc-bundle:4.0.1 sincenelmio/NelmioApiDocBundle#1665

javer mentioned this pull request

Dec 3, 2020

Private properties exposure with Symfony 5.2nelmio/NelmioApiDocBundle#1756

Closed

jderusse modified the milestones:4.4,5.1

Dec 4, 2020

jderusse reviewed

Dec 4, 2020

View reviewed changes

Copy link

Member

jderusse left a comment

Choose a reason for hiding this comment

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

Good catch!

Could you, please, rebase your branch on top of 5.1 ? and change the base branch of this PR?

javer force-pushed thefix-extracting-ignored-properties-for-serializer branch from776462c todd14446Compare

December 5, 2020 15:59

javer changed the base branch from5.x to5.2

December 5, 2020 15:59

Copy link

ContributorAuthor

javer commentedDec 5, 2020

I have rebased on top of 5.2 and have changed target branch to 5.2, because in 5.1 this error is actually not reproducible, because you have to use@Groups() annotation with@Ignore() annotation for the same property to reach this error, and it doesn't make sense. This error is easily reproducible only after changes in#37040 which landed only in 5.2.

javer force-pushed thefix-extracting-ignored-properties-for-serializer branch fromdd14446 tofcd8e3aCompare

December 5, 2020 20:00

javer changed the base branch from5.2 to5.1

December 5, 2020 20:00

Copy link

ContributorAuthor

javer commentedDec 5, 2020

Anyway, I have rebased on top of 5.1 and have changed target branch to 5.1.

javer force-pushed thefix-extracting-ignored-properties-for-serializer branch 2 times, most recently fromadc5448 to039cd98Compare

December 5, 2020 20:55

dunglas approved these changes

Dec 6, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Dec 6, 2020

Copy link

ContributorAuthor

javer commentedDec 7, 2020

FYI Failing tests are not related to the current fix, PhpDocExtractorTest fails with Symfony 4.1 under PHP 7.3, and it wasn't affected in the current PR.

Copy link

Member

fabpot commentedDec 8, 2020

@javer Can you have a look at the failing test and try to fix it?

Copy link

ContributorAuthor

javer commentedDec 8, 2020

@fabpot These tests cannot be fixed in 5.1 branch, because they also fail on 4.1 branch because of BC break introduced in one of the dependency ofsymfony/property-info:https://github.com/phpDocumentor/TypeResolver/releases/tag/0.5.0.

$ git checkout 4.1$ composer update$ ./phpunit src/Symfony/Component/PropertyInfo/#!/usr/bin/env phpPHPUnit 6.5.14 by Sebastian Bergmann and contributors.Testing src/Symfony/Component/PropertyInfo/.................................F...........................F.  63 / 165 ( 38%)........................F...................................... 126 / 165 ( 76%).......................................                         165 / 165 (100%)Time: 242 ms, Memory: 6.00MBThere were 3 failures:1) Symfony\Component\PropertyInfo\Tests\PhpDocExtractor\PhpDocExtractorTest::testExtract with dataset#21 ('i', array(Symfony\Component\PropertyInfo\Type Object (...), Symfony\Component\PropertyInfo\Type Object (...)), null, null)Failed asserting that two arrays are equal.--- Expected+++ Actual@@ @@ Array (     0 => Symfony\Component\PropertyInfo\Type Object (-'builtinType' =>'string'-'nullable' =>true-'class' => null+'builtinType' =>'object'+'nullable' =>false+'class' =>'string'@@ @@-'nullable' =>true+'nullable' =>falsesrc/Symfony/Component/PropertyInfo/Tests/Extractor/PhpDocExtractorTest.php:382) Symfony\Component\PropertyInfo\Tests\PhpDocExtractor\PhpDocExtractorTest::testExtractTypesWithCustomPrefixes with dataset#21 ('i', array(Symfony\Component\PropertyInfo\Type Object (...), Symfony\Component\PropertyInfo\Type Object (...)), null, null)Failed asserting that two arrays are equal.--- Expected+++ Actual@@ @@ Array (     0 => Symfony\Component\PropertyInfo\Type Object (-'builtinType' =>'string'-'nullable' =>true-'class' => null+'builtinType' =>'object'+'nullable' =>false+'class' =>'string'@@ @@-'nullable' =>true+'nullable' =>falsesrc/Symfony/Component/PropertyInfo/Tests/Extractor/PhpDocExtractorTest.php:553) Symfony\Component\PropertyInfo\Tests\PhpDocExtractor\PhpDocExtractorTest::testExtractTypesWithNoPrefixes with dataset#21 ('i', array(Symfony\Component\PropertyInfo\Type Object (...), Symfony\Component\PropertyInfo\Type Object (...)), null, null)Failed asserting that two arrays are equal.--- Expected+++ Actual@@ @@ Array (     0 => Symfony\Component\PropertyInfo\Type Object (-'builtinType' =>'string'-'nullable' =>true-'class' => null+'builtinType' =>'object'+'nullable' =>false+'class' =>'string'@@ @@-'nullable' =>true+'nullable' =>falsesrc/Symfony/Component/PropertyInfo/Tests/Extractor/PhpDocExtractorTest.php:65FAILURES!Tests: 165, Assertions: 246, Failures: 3.

After applying the following patch:

diff --git a/composer.json b/composer.jsonindex f861cbca31..4302251895 100644--- a/composer.json+++ b/composer.json@@ -102,7 +102,7 @@     },     "conflict": {         "phpdocumentor/reflection-docblock": "<3.0||>=3.2.0,<3.2.2",-        "phpdocumentor/type-resolver": "<0.3.0",+        "phpdocumentor/type-resolver": "<0.3.0||>=0.5.0",         "phpunit/phpunit": "<5.4.3"     },     "provide": {diff --git a/src/Symfony/Component/PropertyInfo/composer.json b/src/Symfony/Component/PropertyInfo/composer.jsonindex 88f177d004..bcf0693f5a 100644--- a/src/Symfony/Component/PropertyInfo/composer.json+++ b/src/Symfony/Component/PropertyInfo/composer.json@@ -35,7 +35,7 @@     },     "conflict": {         "phpdocumentor/reflection-docblock": "<3.0||>=3.2.0,<3.2.2",-        "phpdocumentor/type-resolver": "<0.3.0",+        "phpdocumentor/type-resolver": "<0.3.0||>=0.5.0",         "symfony/dependency-injection": "<3.4"     },     "suggest": {

tests do not fail:

$ composer updateLoading composer repositories with package informationUpdating dependencies (including require-dev)Package operations: 0 installs, 2 updates, 0 removals  - Downgrading phpdocumentor/reflection-common (dev-master cf8df60 => 1.0.1):  Checking out 21bdeb5f65  - Downgrading phpdocumentor/type-resolver (1.x-dev 6a467b8 => 0.4.0):  Checking out 9c97770899Package doctrine/doctrine-cache-bundle is abandoned, you should avoid using it. No replacement was suggested.Package doctrine/reflection is abandoned, you should avoid using it. Use roave/better-reflection instead.Writing lock fileGenerating autoload filescomposer/package-versions-deprecated: Generating version class...composer/package-versions-deprecated: ...done generating version class25 packages you are using are lookingfor funding.Use the`composer fund`command to find out more!$ ./phpunit src/Symfony/Component/PropertyInfo/#!/usr/bin/env phpPHPUnit 6.5.14 by Sebastian Bergmann and contributors.Testing src/Symfony/Component/PropertyInfo/...............................................................  63 / 165 ( 38%)............................................................... 126 / 165 ( 76%).......................................                         165 / 165 (100%)Time: 232 ms, Memory: 6.00MBOK (165 tests, 248 assertions)

If we updatephpdocumentor/type-resolver to the next version - it starts failing again:

$ composer updateLoading composer repositories with package informationUpdating dependencies (including require-dev)Package operations: 0 installs, 1 update, 0 removals  - Updating phpdocumentor/type-resolver (0.4.0 => 0.5.0):  Checking out 7159da1ff4Package doctrine/doctrine-cache-bundle is abandoned, you should avoid using it. No replacement was suggested.Package doctrine/reflection is abandoned, you should avoid using it. Use roave/better-reflection instead.Writing lock fileGenerating autoload filescomposer/package-versions-deprecated: Generating version class...composer/package-versions-deprecated: ...done generating version class25 packages you are using are lookingfor funding.Use the`composer fund`command to find out more!MacBook-Pro-Vadim-Work:symfony javer$ ./phpunit src/Symfony/Component/PropertyInfo/#!/usr/bin/env phpPHPUnit 6.5.14 by Sebastian Bergmann and contributors.Testing src/Symfony/Component/PropertyInfo/.................................F...........................F.  63 / 165 ( 38%)........................F...................................... 126 / 165 ( 76%).......................................                         165 / 165 (100%)Time: 163 ms, Memory: 6.00MB...FAILURES!Tests: 165, Assertions: 246, Failures: 3.

I don't know why tests for EOL version of Symfony 4.1 are started during build for 5.1 branch and how we can fix them in this case, could you please help? I have a patch which should be applied on top of 4.1 branch, but not the current 5.1 branch. Should I create PR to 4.1? Is it expected behavior for EOL versions of Symfony?

Copy link

ContributorAuthor

javer commentedDec 8, 2020

@nicolas-grekas Could you please explain as the author of the#33653 feature why it's required in 5.1 build that the affected component tests in 4.1 should be green if they are red in the branch 4.1 itself? Version 4.1 is not supported anymore, it means that we cannot fix issue related to external dependency BC break (see#33588), therefore it's impossible to merge any fixes to PropertyInfo component in Symfony 5.1 and 5.2, because mentioned above fix landed in 4.3, so only for Symfony 5.3 "checkout previous major and test with patched components" will checkout 4.3 and PropertyInfo tests will be green.

And how can I help you to merge this tiny fix? What's my next steps?

Copy link

Member

nicolas-grekas commentedDec 10, 2020

@javer good question, looks like an issue with the CI.

Copy link

Member

stof commentedDec 10, 2020

the tests are meant to be run with the version before it, which should be installing Symfony 4.4, not 4.1. So

symfony/.travis.yml

Lines 202 to 210 inf66b853

	-\|
	# For the feature-branch, when deps=high, the version before it is checked out and tested with the locally patched components
	if [[ $deps = high && $TRAVIS_BRANCH = *.x ]]; then
	export FLIP='🙃'
	export SYMFONY_VERSION=$(git ls-remote -q --heads \| grep -o '/[1-9]\.[0-9].*' \| tail -n 1 \| sed s/.//) &&
	git fetch --depth=2 origin $SYMFONY_VERSION &&
	git checkout -m FETCH_HEAD &&
	export COMPONENTS=$(find src/Symfony -mindepth 2 -type f -name phpunit.xml.dist -printf '%h\n' \| sort)
	fi

looks buggy.

javer force-pushed thefix-extracting-ignored-properties-for-serializer branch from039cd98 tod280d30Compare

December 10, 2020 19:02

Copy link

ContributorAuthor

javer commentedDec 10, 2020•
edited
Loading

I have fixed calculation of the previous version, and all tests are finally passed.

Now exactly the previous version is taken, i.e 4.4 for 5.0, 5.0 for 5.1, 5.1 for 5.2 and so on. If always should be the previous LTS version, i.e. 4.4 for 5.0...5.4 - please tell me, I will fix.