8000 [Form] Add generics to DataTransformerInterface by VincentLanglet · Pull Request #47412 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Add generics to DataTransformerInterface #47412

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
Merged
merged 1 commit into from
Sep 13, 2022
Merged

[Form] Add generics to DataTransformerInterface #47412

merged 1 commit into from
Sep 13, 2022

Conversation

VincentLanglet
Copy link
Contributor
Q A
Branch? 6.2
Bug fix? no
New feature? not really
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

Now that some generics are added to the Symfony codebase, I think we could add more.
For people using phpstan/psalm this will be helpful:

  • It avoid duplicating the generics in both psalm-plugin/phpstan-plugin
  • It avoid duplicating code between symfony and plugins
  • When phpstan/psalm report an error it's less confusing to look for the generic in the symfony file rather the plugin one.

You can look at the current generic definition
for psalm https://github.com/psalm/psalm-plugin-symfony/blob/master/src/Stubs/common/Component/Form/DataTransformerInterface.stubphp
for phpstan https://github.com/phpstan/phpstan-symfony/blob/1.2.x/stubs/Symfony/Component/Form/DataTransformerInterface.stub

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@VincentLanglet VincentLanglet marked this pull request as ready for review August 28, 2022 14:12
@carsonbot carsonbot added this to the 6.2 milestone Aug 28, 2022
VincentLanglet
VincentLanglet commented Aug 28, 2022
View reviewed changes
...ny/Component/Form/Extension/Core/DataTransformer/DateTimeToHtml5LocalDateTimeTransformer.php
@@ -40,8 +42,8 @@ public function transform(mixed $dateTime): string
return '';
}

if (!$dateTime instanceof \DateTime && !$dateTime instanceof \DateTimeInterface) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DateTime implements DateTimeInterface

...mfony/Component/Form/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformer.php
@@ -95,7 +97,7 @@ public function transform(mixed $dateTime): string
/**
* Transforms a localized date string/array into a normalized date.
*
* @param string|array $value Localized date string/array
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method start with if (!\is_string($value)) {

@carsonbot
Copy link

Hey!

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

Cheers!

Carsonbot

@xabbuh xabbuh added the Form label Sep 2, 2022
@carsonbot carsonbot changed the title Add generics to DataTransformerInterface [Form] Add generics to DataTransformerInterface Sep 2, 2022
nicolas-grekas
nicolas-grekas reviewed Sep 12, 2022
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM, here are some comments to iterate.

wouterj
wouterj approved these changes Sep 12, 2022
View reviewed changes
@fabpot
Copy link
Member
fabpot commented Sep 13, 2022

Thank you @VincentLanglet.

@fabpot fabpot merged commit 8a66294 into symfony:6.2 Sep 13, 2022
@stof stof mentioned this pull request Nov 2, 2022
Merged
nicolas-grekas added a commit that referenced this pull request Nov 2, 2022
@nicolas-grekas
minor #48079 Fix the DataTransformerInterface generic types to suppor…
d496640 
…t the patcher (stof)

This PR was merged into the 6.2 branch.

Discussion
----------

Fix the DataTransformerInterface generic types to support the patcher

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        | n/a

The type patcher adding return types handles template types only for methods that already have a return type. As `DataTransformerInterface` is not typed yet in Symfony 6.x, we need to keep the future native types in ``@return`` for the patcher tool. This puts the generic type (introduced in #47412) in ``@psalm`-return`, as done in #48012 (I detected this issue when regenerating the return type for that other PR)

Commits
-------

d9fd5c7 Fix the DataTransformerInterface generic types to support the patcher
@VincentLanglet VincentLanglet mentioned this pull request Dec 20, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wouterj wouterj wouterj approved these changes

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

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

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

Assignees
No one assigned
Labels
Form Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
6 participants
@VincentLanglet @carsonbot @fabpot @nicolas-grekas @wouterj @xabbuh
0