Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] AddSerializedPath annotation to flatten nested attributes#43534
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedOct 15, 2021
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:
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! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
carsonbot commentedOct 16, 2021
Hey! I think@jeroennoten has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
2758fde tofe5d453Compare02e59c6 toa824bd0Comparefc0d799 to9e6dd9aCompareI tried rebasing this from 5.4 to 6.0 by following thecontribution guidelines but it seems like I screwed that up as things got a little messy ... is there any way I can salvage this PR or do I need to create a new one based on the 6.0 branch? |
a15a534 tofdcc545Comparea15a534 to992a563Compare@boenner You only forgot to change the base branch of the pull request. I did it for you and squashed your commits to trigger the CI. |
c210064 to5d84bf2CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Mapping/AttributeMetadataInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Mapping/Loader/XmlFileLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Mapping/Loader/YamlFileLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Component/Serializer/Mapping/Loader/schema/dic/serializer-mapping/serializer-mapping-1.0.xsd OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
bd4277c toef74981CompareThe review is finished and the |
Can you please update the PR's description if it's not in sync? |
ceff3bd to08a1119Compare
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I force-pushed some CS changes. Failures are unrelated or false-positives (fabbot).
Thank you@boenner. |
This PR was merged into the 6.2 branch.Discussion----------[Serializer] SerializedPath documentationAdds documentation for the `SerializedPath` annotation (symfony/symfony#43534). Thisfixes#17389.Commits-------375c089 Documentation for the SerializedPath definition
Sincesymfony/symfony#43534 (released with `symfony/serializer` 6.2.0),Symfony's ObjectNormalizer has a hard dependency to the`symfony/property-access` package. As we're using the ObjectNormalizer,we must ensure to meet this requirement by explicitly adding thepackage as requirement in `composer.json`.
Sincesymfony/symfony#43534 (released with `symfony/serializer` 6.2.0),Symfony's ObjectNormalizer has a hard dependency to the`symfony/property-access` package. As we're using the ObjectNormalizer,we must ensure to meet this requirement by explicitly adding thepackage as requirement in `composer.json`.
…enner)This PR was merged into the 6.2 branch.Discussion----------[Serializer] Fix serialized path for non-scalar values| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fix#49494| License | MIT| Doc PR | noThis relates to#49494 and#49225. When non-scalar values are normalized, they are normalized twice in the `normalize()` function:```phpif (null !== $attributeValue && !\is_scalar($attributeValue)) {$stack[$attribute] = $attributeValue;}$data = $this->updateData($data, $attribute, $attributeValue, $class, $format, $attributeContext, $attributesMetadata, $classMetadata);```and a bit later:```phpforeach ($stack as $attribute => $attributeValue) {...$data = $this->updateData($data, $attribute, $this->serializer->normalize($attributeValue, $format, $childContext), $class, $format, $attributeContext, $attributesMetadata, $classMetadata);}```For non-scalar values with a `SerializedPath` annotation this leads to an exception because the serializer is trying to re-populate the path. Running `updateData()` only once fixes this, but breaks a couple of tests across the components as it changes the order of elements in the serialized string (non-scalar values will be pushed to the end). Other than the string comparisons, nothing seems to break. This was also an issue while reviewing the PR for the `SerializedPath` annotation (#43534 (comment)) and got reverted because of the potential BC break.I'm not sure what benefit normalizing twice brings, so I added the test from `@HonzaMatosik`, changed the behavior in the normalizer and fixed the broken tests. If that's not the preferred solution here, I'd be ok with just eleminating the "The element you are trying to set is already populated" exception in the `SerializedPath` and allow overwriting values.Commits-------d82ec41 [Serializer] Fix serializedpath for non scalar types
…enner)This PR was merged into the 6.2 branch.Discussion----------[Serializer] Fix serialized path for non-scalar values| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | Fix #49494| License | MIT| Doc PR | noThis relates tosymfony/symfony#49494 andsymfony/symfony#49225. When non-scalar values are normalized, they are normalized twice in the `normalize()` function:```phpif (null !== $attributeValue && !\is_scalar($attributeValue)) {$stack[$attribute] = $attributeValue;}$data = $this->updateData($data, $attribute, $attributeValue, $class, $format, $attributeContext, $attributesMetadata, $classMetadata);```and a bit later:```phpforeach ($stack as $attribute => $attributeValue) {...$data = $this->updateData($data, $attribute, $this->serializer->normalize($attributeValue, $format, $childContext), $class, $format, $attributeContext, $attributesMetadata, $classMetadata);}```For non-scalar values with a `SerializedPath` annotation this leads to an exception because the serializer is trying to re-populate the path. Running `updateData()` only once fixes this, but breaks a couple of tests across the components as it changes the order of elements in the serialized string (non-scalar values will be pushed to the end). Other than the string comparisons, nothing seems to break. This was also an issue while reviewing the PR for the `SerializedPath` annotation (symfony/symfony#43534 (comment)) and got reverted because of the potential BC break.I'm not sure what benefit normalizing twice brings, so I added the test from `@HonzaMatosik`, changed the behavior in the normalizer and fixed the broken tests. If that's not the preferred solution here, I'd be ok with just eleminating the "The element you are trying to set is already populated" exception in the `SerializedPath` and allow overwriting values.Commits-------d82ec41d18 [Serializer] Fix serializedpath for non scalar types
Uh oh!
There was an error while loading.Please reload this page.
As suggested by@derrabus in#32080, I'm creating a PR for this.
In order to normalize/denormalize nested attributes, the
@SerializedPathannotation can be used:with
The annotations needs to be used with a valid
PropertyPathstring for this to work.Open todos: