Skip to content

[PropertyInfo][Serializer] Add support of list and trigger deprecation when filling a list-typed property with an array #49056

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from
Open

[PropertyInfo][Serializer] Add support of list and trigger deprecation when filling a list-typed property with an array #49056

wants to merge 1 commit into from

Conversation

alexandre-daubois
Copy link
Member

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Fix #48887
License MIT
Doc PR Todo

As this improvement could break existing code, I went for the deprecation triggering way.

Reproducer:

class TestCommand extends Command
{
    // ...

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        dump($this->serializer->deserialize(<<<JSON
          {
            "list": {
              "test": 1,
              "2": 2
            }
          }
JSON, Foo::class, 'json'));

        return Command::SUCCESS;
    }
}

class Foo
{
    /**
     * @var list<string>
     */
    public array $list;
}

Results on:

image

@carsonbot carsonbot added this to the 6.3 milestone Jan 21, 2023
@carsonbot carsonbot changed the title [Serializer][PropertyInfo] Add support of list and trigger deprecation when filling a list-typed property with an array [PropertyInfo][Serializer] Add support of list and trigger deprecation when filling a list-typed property with an array Jan 21, 2023
@alexandre-daubois alexandre-daubois force-pushed the propertyinfo-serializer-lists branch from f6826e9 to e01dfd6 Compare January 21, 2023 11:37
@carsonbot
Copy link

Hey!

I think @EmilMassey has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@EmilMassey
Copy link

@alexandre-daubois There's still a failing test, that should be taken care of

EmilMassey
EmilMassey reviewed Mar 2, 2023
View reviewed changes
src/Symfony/Component/PropertyInfo/Type.php Outdated
@@ -35,10 +35,8 @@ class Type

/**
* List of PHP builtin types.
*
* @var string[]
Copy link

Choose a reason for hiding this comment

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

Why is this line removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a mistake, thanks for pointing this out 👍

@alexandre-daubois alexandre-daubois force-pushed the propertyinfo-serializer-lists branch 6 times, most recently from 930e6cc to 26f67ef Compare March 2, 2023 15:31
@alexandre-daubois
[PropertyInfo][Serializer] Add support of list and trigger deprecat…
f539aa6 
…ion when filling a list-typed property with an array
@alexandre-daubois alexandre-daubois force-pushed the propertyinfo-serializer-lists branch from 26f67ef to f539aa6 Compare March 2, 2023 15:40
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@mtarld
Copy link
Contributor

mtarld commented Jan 13, 2024

Maybe we can wait for #52510 and #53160 to be merged, to parse list<xxx>?

But I think that such a feature could be great indeed 👍

@chalasr
Copy link
Member

chalasr commented Jan 14, 2024

I agree we would better move forward with the PRs linked above instead as they cover a larger scope that includes the feature provided by this PR.

@alexandre-daubois
Copy link
Member Author

Totally forgot about this one 😄 Totally agree with you, let's close this

@mtarld
Copy link
Contributor

mtarld commented Jan 15, 2024

But half of your PR is still needed, I think (the validation/deprecation part)

@alexandre-daubois
Copy link
Member Author

Alright, I'll wait for TypeInfo to be merged then I'll get back to this one 🙂

@chalasr
Copy link
Member

chalasr commented Jan 15, 2024

👍 Indeed, to be rebased on top of #52510 and #53160 once merged

@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EmilMassey EmilMassey EmilMassey left review comments

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
Deprecation Feature PropertyInfo Serializer Status: Needs Work
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

Serializer/PropertyInfo treats list<Type> as Type[] and does not enforce sequential indexes
8 participants
@alexandre-daubois @carsonbot @EmilMassey @mtarld @chalasr @fabpot @nicolas-grekas @xabbuh