Skip to content

[Serializer] [PropertyAccessor] Ignore non-collection interface generics #52699

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

Merged
merged 1 commit into from
Jun 9, 2024

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Nov 23, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #41996 #49924
License MIT

PhpDocExtractor and PhpStanDocExtractor are following an old version of PSR-5 with used to define collections as the following:

generic = collection-type "<" [type-expression "," *SP] type-expression ">"

But, it does conflict with non-collection generics.

This issue will automatically be solved if the TypeInfo is merged in Symfony. But for older versions (<7.1 at least), it needs a fix.

I was not able to find a proper bug fix without introducing a BC break, so this PR proposes to opt-on the "non-collection generics" parsing, such as stcClass<int> for example.

To opt-out from parsing these generics, it'll be required to set ignore_docblock_generics in the context. And this key/value will become obsolete as soon as the TypeInfo is introduced.

I'm not sure how to proceed though, should we only merge it in 5.4, and 6.3 supposing that the TypeInfo might be merged in 7.x? Should we document it only in these branches?

EDIT: I finally ignored PHPDoc generics that aren't well known collection generic types so that the process will fall back to other resolvers (such as reflection resolver for example)

@mtarld mtarld requested a review from dunglas as a code owner November 23, 2023 08:10
@carsonbot carsonbot added this to the 5.4 milestone Nov 23, 2023
@carsonbot carsonbot changed the title [Serializer][PropertyAccessor] Allow to ignore non-collection generics [Serializer] [PropertyAccessor] Allow to ignore non-collection generics Nov 23, 2023
@nicolas-grekas
Copy link
Member

What about considering this as an improvement and merging into 6.4? Then we can do the BC break in 7.0.
I'd be fine saying those annotations aren't supported in 5.4 personnally.

@mtarld
Copy link
Contributor Author

mtarld commented Nov 24, 2023

It also means that it won't be supported in 6.3 and 7.0 (supposing that the TypeInfo will come in 7.1). But I'm fine with that as well, as 6.4 matters the most, being an LTS.

@mtarld mtarld changed the base branch from 5.4 to 6.4 November 24, 2023 05:00
@mtarld mtarld force-pushed the fix/generic-doc-block branch 3 times, most recently from a170c98 to 788659f Compare November 24, 2023 05:07
@mtarld mtarld changed the title [Serializer] [PropertyAccessor] Allow to ignore non-collection generics [Serializer][PropertyAccessor] Allow to ignore non-collection generics Nov 24, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 6.4 Nov 24, 2023
@mtarld mtarld force-pushed the fix/generic-doc-block branch from 788659f to a59732f Compare November 28, 2023 08:56
@mtarld mtarld changed the base branch from 6.4 to 5.4 November 28, 2023 08:57
@mtarld mtarld force-pushed the fix/generic-doc-block branch from a59732f to eaac5c8 Compare November 28, 2023 08:58
@mtarld mtarld changed the title [Serializer][PropertyAccessor] Allow to ignore non-collection generics [PropertyAccessor] Allow to ignore non-collection generics Nov 28, 2023
@mtarld mtarld changed the title [PropertyAccessor] Allow to ignore non-collection generics [PropertyAccessor] Ignore non-collection interface generics Nov 28, 2023
@mtarld mtarld force-pushed the fix/generic-doc-block branch from eaac5c8 to 7f06cac Compare December 19, 2023 13:58
@mtarld mtarld force-pushed the fix/generic-doc-block branch from 7f06cac to 487b24c Compare January 5, 2024 10:40
@mtarld mtarld force-pushed the fix/generic-doc-block branch from 487b24c to 9ecc19d Compare March 20, 2024 07:48
@carsonbot carsonbot changed the title [PropertyAccessor] Ignore non-collection interface generics [Serializer] [PropertyAccessor] Ignore non-collection interface generics Apr 5, 2024
@mtarld mtarld force-pushed the fix/generic-doc-block branch from 9ecc19d to 7b0d86b Compare April 18, 2024 12:53
@mtarld mtarld force-pushed the fix/generic-doc-block branch from 7b0d86b to e31aeeb Compare April 18, 2024 12:57
@fabpot fabpot modified the milestones: 6.4, 5.4 Jun 9, 2024
@fabpot
Copy link
Member

fabpot commented Jun 9, 2024

Thank you @mtarld.

@fabpot fabpot merged commit f4b0c27 into symfony:5.4 Jun 9, 2024
9 of 12 checks passed
@mtarld mtarld deleted the fix/generic-doc-block branch June 9, 2024 08:09
@xabbuh
Copy link
Member

xabbuh commented Jun 25, 2024

@mtarld I have merged these changes up to the 7.1 branch which now fails with one error. I tried to find out what we need to change but wasn't able to resolve it. In case you want to take a look at it. :)

@mtarld
Copy link
Contributor Author

mtarld commented Jun 26, 2024

@xabbuh, indeed new tests must be updated like the current ones. I created PR fixing it: #57530.
Thanks for reaching me 🙂

nicolas-grekas added a commit that referenced this pull request Jun 26, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] Fix generics related test

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        |
| License       | MIT

Update tests accordingly to #52699 (in the same way as the legacy tests).

Commits
-------

9245561 [PropertyInfo] Fix generics related test
@soyuka
Copy link
Contributor

soyuka commented Jul 1, 2024

HI, since that we're observing a collection: false instead of true for properties typed as:

\Doctrine\Common\Collections\ArrayCollection<Foo>

is_a(ArrayCollection::class, \ArrayAccess::class, true) is true not sure why it fails, any idea @mtarld ?

@andyexeter
Copy link
Contributor

We're seeing the same issue as @soyuka. Looks like it will be fixed once #57617 is merged

xabbuh added a commit that referenced this pull request Jul 6, 2024
… (mtarld)

This PR was merged into the 5.4 branch.

Discussion
----------

[PropertyInfo] Handle collection in PhpStan same as PhpDoc

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | Fix #52699 (comment)
| License       | MIT

On #52699, a [comment](#52699 (comment)) suggested to support classes that inherit from collection classes (such as doctrine collections for example).
This has been done for PHPDoc, but not in PHPStan.

This PR adds this missing behavior to PHPStan (and adds the missing PHPDoc related test).

Commits
-------

f098683 [PropertyInfo] Handle collection in PhpStan same as PhpDoc
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.

9 participants