NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Add phpdoc for constraint#42136

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

Jump to bottom

Closed

VincentLanglet wants to merge1 commit intosymfony:5.4fromVincentLanglet:constraint

Closed

Add phpdoc for constraint#42136

VincentLanglet wants to merge1 commit intosymfony:5.4fromVincentLanglet:constraint

Conversation

Copy link

Contributor

VincentLanglet commentedJul 15, 2021•
edited
Loading

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

When extending some constraint and overriding some property, it's not always simple to know what can be passed to the properties and when it can be null. Those properties should have phpdoc to define their type.

Plus it avoid a psalm errorNonInvariantDocblockPropertyType for the developer which use a more specific phpdoc (because atm, properties are considered as mixed).

And, it will help the typing for PHP8 on the branch 6.0@nicolas-grekas :)

Add phpdoc for constraint

a7fdb47

carsonbot added the Status: Needs Review label

Jul 15, 2021

Copy link

Member

derrabus commentedJul 15, 2021

Fixing Doc blocks should be done as a bugfix on lower branches (4.4 mainly, in this case).

Copy link

ContributorAuthor

VincentLanglet commentedJul 15, 2021

Fixing Doc blocks should be done as a bugfix on lower branches (4.4 mainly, in this case).

I fully agree on this.
But on one of my last PR there was this debate#40604 and it ended with the decision it should be on 5.x.

If the core team now decide it's a bugfix, I'm fine doing it on 4.4 but I'd like some confirmation before doing this PR again.

Copy link

Member

derrabus commentedJul 15, 2021

Fair enough. Sorry for the confusion. 😞

nicolas-grekas added this to the5.4 milestone

Jul 19, 2021

Copy link

Member

nicolas-grekas commentedJul 19, 2021

We try to remove phpdoc actually.
We have a rule where the default type of a property is enough clue to tell its type.
I'm fine adding phpdoc when it really brings value, but I'd better remove them otherwise.

Copy link

ContributorAuthor

VincentLanglet commentedJul 19, 2021•
edited
Loading

We have a rule where the default type of a property is enough clue to tell its type.
I'm fine adding phpdoc when it really brings value, but I'd better remove them otherwise.

The rule has some sens, but it's not because a property has a default value that another type is forbidden
For instance
https://github.com/symfony/symfony/pull/42136/files#diff-738b9954d83ec3560d6a0b70f45958640c89abd268d72bba647e87c0ff13e185R50
The default value is an array, but looking at the code, a string is allowed too.

My main issue was that psalm and phpstan consider asmixed every property without typehint or phpdoc. Witch is pretty annoying when overriding a constraint class/property and adding some phpdoc. I would have prefer to add the phpdoc here rather than in the stub of psalm-symfony repository. The phpdoc can be removed in 6.0 when the type will be added.

Copy link

Member

nicolas-grekas commentedJul 19, 2021•
edited
Loading

That's an issue with psalm/phpstan, it might be fixed with a plugin maybe?
But I'm overall 👎 to "pollute" the code base with useless phpdoc, we already spend too much time on maintaining these.

Copy link

ContributorAuthor

VincentLanglet commentedJul 19, 2021

That's an issue with psalm/phpstan, it might be fixed with a plugin maybe?
But I'm overall 👎 to "pollute" the code base with useless phpdoc, we already spend too much time on maintaining these.

So I should remove the phpdoc of property with default value and keep the others ?