[Serializer] Support serialized names and paths configuration per group #58236

daniser · 2024-09-11T14:48:35Z

Q	A
Branch?	7.2
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #30483, #53858
License	MIT

At the moment some of my own MetadataAwareNameConverter tests are failing. I have tried to fix them, but everything I tried so far breaks existing tests. Just can't figure out what I have to do. Any help would be greatly appreciated!

carsonbot · 2024-09-11T14:48:40Z

Hey!

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

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.2 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

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.php

mtarld · 2024-09-18T16:12:55Z

src/Symfony/Component/Serializer/Attribute/SerializedPath.php

     */
-    public function __construct(string $serializedPath)
+    public function __construct(string $serializedPath, string|array $groups = [])


Suggested change

public function __construct(string $serializedPath, string|array $groups = [])

public function __construct(string $serializedPath, string|array $groups = ['*'])

And then we can check that $groups is not empty
(same for SerializedName)

What should we do in case $groups is empty? Throw exception or fall back to ['*']?
(BTW, I've implemented second option).

src/Symfony/Component/Serializer/Mapping/Loader/AttributeLoader.php

src/Symfony/Component/Serializer/NameConverter/MetadataAwareNameConverter.php

…tters and setters

… is null

…y in serialized name/path setters

OskarStark

Massive work in this PR, wow 👍 Thank you!

I added some comments

src/Symfony/Component/Serializer/Attribute/SerializedName.php

src/Symfony/Component/Serializer/CHANGELOG.md

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.php

OskarStark · 2024-09-19T20:49:39Z

src/Symfony/Component/Serializer/Mapping/Loader/YamlFileLoader.php

+                    $groups = $line['groups'] ?? [];
+
+                    if ($serializedName = $line['serialized_name'] ?? false) {
+                        if (!\is_string($serializedName) || '' === $serializedName) {


What about ' '?

Could you please elaborate? Do I need to trim string and then check for emptiness? Or just check for single space? What's the point?

src/Symfony/Component/Serializer/Tests/Mapping/Loader/YamlFileLoaderTest.php

src/Symfony/Component/Serializer/Tests/NameConverter/MetadataAwareNameConverterTest.php

src/Symfony/Component/Serializer/CHANGELOG.md

mtarld · 2024-10-29T11:24:59Z

src/Symfony/Component/Serializer/Attribute/SerializedPath.php

    {
        try {
            $this->serializedPath = new PropertyPath($serializedPath);
        } catch (InvalidPropertyPathException $pathException) {
            throw new InvalidArgumentException(\sprintf('Parameter given to "%s" must be a valid property path.', self::class));
        }
+
+        $this->groups = ((array) $groups) ?: ['*'];


I'm not sure that we must fall back to ['*'] here, maybe an empty array can be valid in some cases (same for SerializedName)

I've tried that but it led to more failing tests. So I reverted it back until we decide what it could actually mean, so we could change the tests.
In my point of view, empty array here is nonsense, as it means "apply serialized name/path to no groups".
As alternative to fallback to ['*'], we can throw exception. WDYT?

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.php

src/Symfony/Component/Serializer/Mapping/AttributeMetadataInterface.php

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.php

src/Symfony/Component/Serializer/NameConverter/MetadataAwareNameConverter.php

# Conflicts: # src/Symfony/Component/Serializer/CHANGELOG.md # src/Symfony/Component/Serializer/Mapping/Loader/AttributeLoader.php

# Conflicts: # src/Symfony/Component/Serializer/NameConverter/MetadataAwareNameConverter.php

[Serializer] Support serialized names and paths configuration per group

c31acc5

daniser requested a review from dunglas as a code owner September 11, 2024 14:48

carsonbot added Status: Needs Review Feature Serializer labels Sep 11, 2024

carsonbot added this to the 7.2 milestone Sep 11, 2024

Sergey Danilchenko and others added 4 commits September 11, 2024 17:10

CS fixes

5f869f9

Fix static analysis

c7d2c4d

Revert changes to exception message

bb65ab8

Merge branch '7.2' into serialized-names-and-paths-per-group

899dc71

mtarld reviewed Sep 18, 2024

View reviewed changes

Sergey Danilchenko added 7 commits September 19, 2024 11:48

Attribute metadata: default asterisk group in serialized name/path ge…

223ad89

…tters and setters

Attribute metadata: properly unset data if given serialized name/path…

796ed93

… is null

Attribute metadata: get rid of unsafe array_filter function usage

0b12b51

Attribute metadata: update merging logic

7f6ac69

Default asterisk group in serialized name/path attributes

a46ed52

Attribute metadata: fall back to asterisk group if group list is empt…

dcab6c1

…y in serialized name/path setters

Name converter: add default groups to context during normalization

8492935

OskarStark reviewed Sep 19, 2024

View reviewed changes

Sergey Danilchenko and others added 4 commits September 20, 2024 10:05

CS fix

db5cc11

Update CHANGELOG.md

d2d075e

Update tests

4a044f0

Merge branch '7.2' into serialized-names-and-paths-per-group

3911bd4

daniser mentioned this pull request Sep 20, 2024

[Serializer] Allow to add groups to SerializedName annotation/attribute #46432

Open

mtarld reviewed Oct 29, 2024

View reviewed changes

Sergey Danilchenko added 4 commits November 5, 2024 12:33

Merge branch '7.2' into serialized-names-and-paths-per-group

fa5badf

# Conflicts: # src/Symfony/Component/Serializer/CHANGELOG.md # src/Symfony/Component/Serializer/Mapping/Loader/AttributeLoader.php

Minor changes

3b3c2ba

Do not fall back to '*' group in serialized name/path attributes

719120a

Change way of retrieving BC-friendly arguments

d4bcf65

Sergey Danilchenko added 3 commits November 11, 2024 11:10

Merge branch '7.2' into serialized-names-and-paths-per-group

cf43396

# Conflicts: # src/Symfony/Component/Serializer/NameConverter/MetadataAwareNameConverter.php

Partial revert due to failing tests

3fbe2c6

Partial revert due to failing tests

b3f789f

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

	public function __construct(string $serializedPath, string\|array $groups = [])
	public function __construct(string $serializedPath, string\|array $groups = ['*'])

Uh oh!

[Serializer] Support serialized names and paths configuration per group #58236

Are you sure you want to change the base?

[Serializer] Support serialized names and paths configuration per group #58236

Uh oh!

Conversation

daniser commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonbot commented Sep 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtarld Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

daniser Sep 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OskarStark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OskarStark Sep 19, 2024

Choose a reason for hiding this comment

Uh oh!

daniser Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtarld Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

daniser Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniser commented Sep 11, 2024 •

edited

Loading