[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).

mtarld · 2024-09-18T16:16:27Z

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

@@ -204,6 +204,16 @@ private function loadAttributes(\ReflectionMethod|\ReflectionClass|\ReflectionPr
        }
    }

+    private function setAttributeSerializedNameForGroups(SerializedName $annotation, AttributeMetadataInterface $attributeMetadata): void


Suggested change

private function setAttributeSerializedNameForGroups(SerializedName $annotation, AttributeMetadataInterface $attributeMetadata): void

private function setAttributeSerializedNameForGroups(SerializedName $attribute, AttributeMetadataInterface $attributeMetadata): void

(same for setAttributeSerializedPathForGroups)

setAttributeContextsForGroups's corresponding argument still have $annotation as its name. Do I really need to rename it now?

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

mtarld · 2024-10-29T11:23:47Z

src/Symfony/Component/Serializer/CHANGELOG.md

@@ -12,6 +12,7 @@ CHANGELOG
 * Add support for configuring multiple serializer instances with different
   default contexts, name converters, sets of normalizers and encoders
 * Add support for collection profiles of multiple serializer instances
+ * Support serialized names and paths configuration per group


Suggested change

* Support serialized names and paths configuration per group

* Add support for serialized names and paths configuration per group

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)

mtarld · 2024-10-29T11:30:30Z

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

    {
-        $this->serializedName = $serializedName;
+        $groups = 2 <= \func_num_args() ? (func_get_arg(1) ?: ['*']) : ['*'];


Suggested change

$groups = 2 <= \func_num_args() ? (func_get_arg(1) ?: ['*']) : ['*'];

$groups = \func_get_args()[1] ?? ['*'];

same elsewhere

mtarld · 2024-10-29T11:34:08Z

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

@@ -21,6 +21,9 @@
 * @internal
 *
 * @author Kévin Dunglas <[email protected]>
+ *
+ * @method string[]       getSerializedNames() Gets all the serialized names per group ("*" being the base name applied to all groups).


Suggested change

* @method string[] getSerializedNames() Gets all the serialized names per group ("*" being the base name applied to all groups).

* @method string[] getSerializedNames() Gets all the serialized names per group.

I'm not sure if we should specify the * specifics here

mtarld · 2024-10-29T12:03:41Z

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

+     *
+     * @return array<string, string>
+     */
+    public static function getSerializedNamesFromAttributeMetadata(AttributeMetadataInterface $attributeMetadata): array


Could you add a description saying that this method is here for BC purposes?

mtarld · 2024-10-29T12:04:50Z

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

@@ -117,22 +121,22 @@ private function getCacheValueForAttributesMetadata(string $class, array $contex

        $classMetadata = $this->metadataFactory->getMetadataFor($class);

+        $contextGroups = (array) ($context[AbstractNormalizer::GROUPS] ?? []);


You might follow #58705, as it might change this part of the code.

[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

Oct	NOV	Dec
	03
2023	2024	2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

daniser commented Sep 11, 2024 •

edited

Loading

carsonbot commented Sep 11, 2024

mtarld Sep 18, 2024

daniser Sep 19, 2024

mtarld Sep 18, 2024

daniser Sep 19, 2024

OskarStark left a comment

OskarStark Sep 19, 2024

daniser Sep 20, 2024

mtarld Oct 29, 2024

mtarld Oct 29, 2024

mtarld Oct 29, 2024

mtarld Oct 29, 2024

mtarld Oct 29, 2024

mtarld Oct 29, 2024

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

	private function setAttributeSerializedNameForGroups(SerializedName $annotation, AttributeMetadataInterface $attributeMetadata): void
	private function setAttributeSerializedNameForGroups(SerializedName $attribute, AttributeMetadataInterface $attributeMetadata): void

	* Support serialized names and paths configuration per group
	* Add support for serialized names and paths configuration per group

	$groups = 2 <= \func_num_args() ? (func_get_arg(1) ?: ['']) : [''];
	$groups = \func_get_args()[1] ?? ['*'];

	* @method string[] getSerializedNames() Gets all the serialized names per group ("*" being the base name applied to all groups).
	* @method string[] getSerializedNames() Gets all the serialized names per group.

		@@ -117,22 +121,22 @@ private function getCacheValueForAttributesMetadata(string $class, array $contex

		$classMetadata = $this->metadataFactory->getMetadataFor($class);

		$contextGroups = (array) ($context[AbstractNormalizer::GROUPS] ?? []);

[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

Conversation

daniser commented Sep 11, 2024 • edited Loading

carsonbot commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniser commented Sep 11, 2024 •

edited

Loading