Skip to content

[Validator] Enhance PHPDoc on Constraint::getTargets() #58493

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
Oct 9, 2024

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Oct 8, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? no
Issues -
License MIT

Provides better static analysis on constraints.

@carsonbot carsonbot added this to the 7.2 milestone Oct 8, 2024
@alexandre-daubois alexandre-daubois force-pushed the constraint-phpdoc branch 2 times, most recently from b2b9050 to 987af2f Compare October 8, 2024 11:53
* Constraint::CLASS_CONSTRAINT and Constraint::PROPERTY_CONSTRAINT.
*
* @return string|string[] One or more constant values
* @return self::CLASS_CONSTRAINT|self::PROPERTY_CONSTRAINT|array<self::CLASS_CONSTRAINT|self::PROPERTY_CONSTRAINT> One or more constant values
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't this work?

Suggested change
* @return self::CLASS_CONSTRAINT|self::PROPERTY_CONSTRAINT|array<self::CLASS_CONSTRAINT|self::PROPERTY_CONSTRAINT> One or more constant values
* @return self::*_CONSTRAINT|array<self::*_CONSTRAINT> One or more constant values

Copy link
Member Author

@alexandre-daubois alexandre-daubois Oct 8, 2024

Choose a reason for hiding this comment

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

It works: https://phpstan.org/r/e511c5d3-eacc-4d1d-b9b3-c45683a3429f

I hesitated, because we could unintentionally include new constants ending with _CONSTRAINT. But if it seems OK to you, I'm fine with it 👍

Copy link
Member

Choose a reason for hiding this comment

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

It appears to not work for Psalm (https://psalm.dev/r/25734b245c), which is a requirement for us (as this is what we use in our own CI).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, let's keep it as-is then. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

then:

Suggested change
* @return self::CLASS_CONSTRAINT|self::PROPERTY_CONSTRAINT|array<self::CLASS_CONSTRAINT|self::PROPERTY_CONSTRAINT> One or more constant values
* @return self::CLASS_CONSTRAINT|self::PROPERTY_CONSTRAINT|array<self::CLASS_CONSTRAINT|self::PROPERTY_CONSTRAINT>

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed!

@alexandre-daubois alexandre-daubois force-pushed the constraint-phpdoc branch 3 times, most recently from 7f5ce66 to bc81173 Compare October 8, 2024 14:58
@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit 5cabe45 into symfony:7.2 Oct 9, 2024
8 of 10 checks passed
@alexandre-daubois alexandre-daubois deleted the constraint-phpdoc branch October 9, 2024 14:28
xabbuh added a commit that referenced this pull request Oct 14, 2024
…return types (xabbuh)

This PR was merged into the 7.2 branch.

Discussion
----------

[ErrorHandler] resolve class constant types when patching return types

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

account for types like the ones added in #58493

Commits
-------

01eb32c resolve class constant types when patching return types
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.

6 participants