Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:5.4frommtarld:fix/generic-doc-block
Jun 9, 2024

Conversation

@mtarld
Copy link
Contributor

@mtarldmtarld commentedNov 23, 2023•
edited
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#41996#49924
LicenseMIT

PhpDocExtractor andPhpStanDocExtractor 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 theTypeInfo 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 asstcClass<int> for example.

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

I'm not sure how to proceed though, should we only merge it in 5.4, and 6.3 supposing that theTypeInfo 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)

@carsonbotcarsonbot added this to the5.4 milestoneNov 23, 2023
@carsonbotcarsonbot changed the title[Serializer][PropertyAccessor] Allow to ignore non-collection generics[Serializer] [PropertyAccessor] Allow to ignore non-collection genericsNov 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
ContributorAuthor

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

@mtarldmtarld changed the base branch from5.4 to6.4November 24, 2023 05:00
@mtarldmtarldforce-pushed thefix/generic-doc-block branch 3 times, most recently froma170c98 to788659fCompareNovember 24, 2023 05:07
@mtarldmtarld changed the title[Serializer] [PropertyAccessor] Allow to ignore non-collection generics[Serializer][PropertyAccessor] Allow to ignore non-collection genericsNov 24, 2023
@nicolas-grekasnicolas-grekas modified the milestones:5.4,6.4Nov 24, 2023
@mtarldmtarld changed the base branch from6.4 to5.4November 28, 2023 08:57
@mtarldmtarld changed the title[Serializer][PropertyAccessor] Allow to ignore non-collection generics[PropertyAccessor] Allow to ignore non-collection genericsNov 28, 2023
@mtarldmtarld changed the title[PropertyAccessor] Allow to ignore non-collection generics[PropertyAccessor] Ignore non-collection interface genericsNov 28, 2023
@mtarldmtarldforce-pushed thefix/generic-doc-block branch from7f06cac to487b24cCompareJanuary 5, 2024 10:40
@mtarldmtarldforce-pushed thefix/generic-doc-block branch from487b24c to9ecc19dCompareMarch 20, 2024 07:48
@carsonbotcarsonbot changed the title[PropertyAccessor] Ignore non-collection interface generics[Serializer] [PropertyAccessor] Ignore non-collection interface genericsApr 5, 2024
@mtarldmtarldforce-pushed thefix/generic-doc-block branch from9ecc19d to7b0d86bCompareApril 18, 2024 12:53
@mtarldmtarldforce-pushed thefix/generic-doc-block branch from7b0d86b toe31aeebCompareApril 18, 2024 12:57
@fabpotfabpot modified the milestones:6.4,5.4Jun 9, 2024
@fabpot
Copy link
Member

Thank you@mtarld.

@fabpotfabpot merged commitf4b0c27 intosymfony:5.4Jun 9, 2024
@mtarldmtarld deleted the fix/generic-doc-block branchJune 9, 2024 08:09
@xabbuh
Copy link
Member

@mtarld I have merged these changes up to the7.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
ContributorAuthor

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

xabbuh reacted with heart emoji

nicolas-grekas added a commit that referenced this pull requestJun 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       | MITUpdate tests accordingly to#52699 (in the same way as the legacy tests).Commits-------9245561 [PropertyInfo] Fix generics related test
This was referencedJun 28, 2024
@soyuka
Copy link
Contributor

HI, since that we're observing acollection: false instead oftrue for properties typed as:

\Doctrine\Common\Collections\ArrayCollection<Foo>

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

paullallier and andyexeter reacted with thumbs up emoji

@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 requestJul 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       | MITOn#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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@fabpotfabpotfabpot approved these changes

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

+1 more reviewer

@maxheliasmaxheliasmaxhelias approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

9 participants

@mtarld@nicolas-grekas@fabpot@xabbuh@soyuka@andyexeter@stof@maxhelias@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp