At the moment some of my ownMetadataAwareNameConverter 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!

[Serializer] Support serialized names and paths configuration per group

c31acc5

daniser requested a review fromdunglas as acode owner

September 11, 2024 14:48

carsonbot added Status: Needs Review Feature Serializer labels

Sep 11, 2024

carsonbot added this to the7.2 milestone

Sep 11, 2024

Copy link

carsonbot commentedSep 11, 2024

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 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

Sergey Danilchenkoand others added4 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

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.php OutdatedShow resolvedHide resolved

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

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

Copy link

Contributor

mtarldSep 18, 2024

Choose a reason for hiding this comment

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

Suggested change

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

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

Copy link

Author

daniserSep 19, 2024

Choose a reason for hiding this comment

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

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 OutdatedShow resolvedHide resolved

src/Symfony/Component/Serializer/NameConverter/MetadataAwareNameConverter.phpShow resolvedHide resolved

Sergey Danilchenko added7 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

Copy link

Contributor

OskarStark left a comment

Choose a reason for hiding this comment

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

Massive work in this PR, wow 👍 Thank you!

I added some comments

src/Symfony/Component/Serializer/Attribute/SerializedName.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Serializer/CHANGELOG.md OutdatedShow resolvedHide resolved

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.phpShow resolvedHide resolved

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

		$groups = $line['groups'] ?? [];

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

Copy link

Contributor

OskarStarkSep 19, 2024

Choose a reason for hiding this comment

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

What about' '?

Copy link

Author

daniserSep 20, 2024

Choose a reason for hiding this comment

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

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.phpShow resolvedHide resolved

src/Symfony/Component/Serializer/Tests/NameConverter/MetadataAwareNameConverterTest.php OutdatedShow resolvedHide resolved

Sergey Danilchenkoand others added4 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

src/Symfony/Component/Serializer/CHANGELOG.md OutdatedShow resolvedHide resolved

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) ?: ['*'];

Copy link

Contributor

mtarldOct 29, 2024

Choose a reason for hiding this comment

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

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

Copy link

Author

daniserNov 11, 2024

Choose a reason for hiding this comment

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

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 tono groups".
As alternative to fallback to['*'], we can throw exception. WDYT?

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.phpShow resolvedHide resolved

src/Symfony/Component/Serializer/Mapping/AttributeMetadataInterface.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Serializer/Mapping/AttributeMetadata.phpShow resolvedHide resolved

src/Symfony/Component/Serializer/NameConverter/MetadataAwareNameConverter.phpShow resolvedHide resolved

Sergey Danilchenko added4 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 added3 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

Labels

Feature Serializer Status: Needs Review

5 participants

Movatterモバイル変換

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 commentedSep 11, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

carsonbot commentedSep 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtarldSep 18, 2024

Choose a reason for hiding this comment

Uh oh!

daniserSep 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!

OskarStarkSep 19, 2024

Choose a reason for hiding this comment

Uh oh!

daniserSep 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtarldOct 29, 2024

Choose a reason for hiding this comment

Uh oh!

daniserNov 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniser commentedSep 11, 2024•
edited
Loading