Skip to content
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.

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

Add generics to ArgumentMetadata::getAttributes #45094

Merged
merged 1 commit into from Mar 17, 2022
Merged

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Jan 20, 2022

Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

This is a bit of an unfortunate situation because the function does too many things. If stronger typing was applied earlier on, it would have been evident that typing this correctly is pretty much impossible, and that would have helped guide the design to use a separate function to retrieve all attributes and some attributes.

e.g. the ideal outcome would be for getAttributes(Foo::class) to be typed as such:

    /**
     * @template T
     * @param class-string<T> $name
     * @return array<T>
     */

So that the result is guaranteed to be an array of Foo objects.

The fact that $name is nullable throws all this off though.. So I guess the only good way forward would be to deprecate null, or deprecate passing $name and add a getAttributesByClassName(string $class, int $flags = 0) ?

I'm not sure if merging this as is is valuable or not, or if you'd rather get the deprecation and new method targetting 6.1?

@carsonbot carsonbot added this to the 5.3 milestone Jan 20, 2022
@Seldaek Seldaek changed the title Add generics to ArgumentMetadata::getAttributes [HttpKernel] Add generics to ArgumentMetadata::getAttributes Jan 20, 2022
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Do you need this as a bugfix? We usually merge this kind of change to the feature branch.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 20, 2022

That reminds me #40783, we should not forget about it 👼

@Seldaek
Copy link
Member Author

Seldaek commented Jan 20, 2022

Well, do I need it? No, I worked around it already with an instanceof check to calm PHPStan down..

I just assumed this was going to be a simple tweak that could be done as bugfix, but then I noticed the API is kinda bad and thus the types are too broad and yeah.. the usual rabbit hole :)

@derrabus
Copy link
Member

Okay, let's target 6.1 then. You never know who's parsing those docblocks these days. 🙈

@derrabus derrabus modified the milestones: 5.3, 6.1 Jan 20, 2022
@derrabus derrabus changed the base branch from 5.3 to 6.1 January 20, 2022 16:13
@Seldaek
Copy link
Member Author

Seldaek commented Jan 20, 2022

I'm fine rebasing this for 6.1, but the question remains what the better API is.. do others care about fixing the API and deprecating things one way or the other? Or is it just me? :)

@ro0NL
Copy link
Contributor

ro0NL commented Jan 27, 2022

try https://psalm.dev/r/eb6309306d

@stof
Copy link
Member

stof commented Jan 27, 2022

This syntax for conditional types works in psalm, but is not supported in phpstan or in PHPStorm (without the psalm plugin) or other IDEs. So this makes it unacceptable to be used in the Symfony phpdoc.

@ro0NL
Copy link
Contributor

ro0NL commented Jan 27, 2022

phpstan/phpstan#3853 we wait

personally, psalm is like a god to me ;);)

@nicolas-grekas
Copy link
Member

(rebase needed)

@Seldaek
Copy link
Member Author

Seldaek commented Mar 16, 2022

Ok rebased. Added a second method to have cleaner return types. I guess one could add a deprecation warning if you pass $name to getAttributes() but I'm not sure if it's worth bothering people with this.

@Seldaek Seldaek force-pushed the patch-23 branch 3 times, most recently from 2a0de9d to 7475c24 Compare March 16, 2022 16:05
@carsonbot carsonbot changed the title [HttpKernel] Add generics to ArgumentMetadata::getAttributes Add generics to ArgumentMetadata::getAttributes Mar 16, 2022
@fabpot
Copy link
Member

fabpot commented Mar 17, 2022

Thank you @Seldaek.

Seldaek added a commit to Seldaek/symfony that referenced this pull request Mar 17, 2022
fabpot added a commit that referenced this pull request Mar 17, 2022
This PR was squashed before being merged into the 6.1 branch.

Discussion
----------

Make use of the new getAttributesOfType method

Refs #45094

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against the latest branch.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

Use the new method from #45094

Commits
-------

b1fb345 Make use of the new getAttributesOfType method
@fabpot fabpot mentioned this pull request Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants