- Issue created by @dpi
- Merge request !8934Improve static analysis by changing string to list of possible values β (Open) created by dpi
- Issue was unassigned.
- Status changed to Needs review
about 1 month ago 4:45am 26 July 2024 - Status changed to RTBC
about 1 month ago 3:41pm 28 July 2024 - π¦πΊAustralia dpi Perth, Australia
Would an Enum fit in here?
Per issue summary, and β¨ Use constants for user email notification types Needs work
- π«π·France nod_ Lille
I don't think we have that anywhere else in core, do you know how it behaves with api.drupal.org? today we have this list as description of the parameter here: https://api.drupal.org/api/drupal/core%21modules%21user%21user.module/fu...
- π«π·France nod_ Lille
From what I see api.drupal.org should handle it fine, but this is not following our doc standards: https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... β params needs a type, not a list of values.
Next steps would be to convert to enum and use that in the @param documentation. Tempted to won't fix this one.
- π¦πΊAustralia mstrelan
I must say I don't particularly like this, the enum would be much nicer. Regardless, I think it's possible (even if ill-advised) to alter or extend the available options, so this might be needlessly restrictive.
- π¦πΊAustralia dpi Perth, Australia
I don't think we have that anywhere else in core
Psalm / Stan syntax have pretty much been unofficially approved by the PHP community.
Next steps would be to convert to enum and use that in the @param documentation.
This is a quick fix, the enum issue is certainly more involved than this.
I must say I don't particularly like this, the enum would be much nicer.
This doesnt change anything, and I agree the enum is the next step.
Regardless, I think it's possible (even if ill-advised) to alter or extend the available options, so this might be needlessly restrictive.
This is an underscore-function, so its designated internal. It has a list of allowed values, its just not enforced. Adding restrictions would involve actually enforcing these values, which this issue does not propose. The other enum issue however, would actually enforce the values. So perhaps its worth adding your 2c there.
- Status changed to Postponed
about 1 month ago 9:48am 7 August 2024 - π«π·France nod_ Lille
In my opinion, this issue is an uncontroversial nonbreaking change.
I agree that it's de-facto how it's done everywhere else. It doesn't change the fact that our coding standards needs to be updated to allow it before making the change. So we need a new issue over at https://www.drupal.org/project/coding_standards β and I'm happy to +1 on it
- π¦πΊAustralia dpi Perth, Australia
Why would coding standards need an update?
- π«π·France nod_ Lille
technically it's "API documentation and comment standards > Indicating data types in documentation" that is under our PHP coding standards that needs the change.
A list of values is not a data type per the description in https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... β
- π¦πΊAustralia dpi Perth, Australia
PHPstan does the checking to see if it is well formed, and validates i/o.
Coding standards applies coding standards. If after a while we find some kind of variants in how this syntax is written, then we go through the process of implementing sniffs.
Approving all data types in coding standard upfront slows down progress.
FWIW, string and pipe is already allowed. I believe from convention our standard doesnt allow spaces-between types. So it is merely the literal-string syntax I understand as the only disagreement.
- π³πΏNew Zealand quietone New Zealand
To answer the question above, here is what it would look like
- π«π·France nod_ Lille
Thanks, I tried in PhpStorm and there is no autocompletion available with that type param declaration that I can see.
I tried to add a call to _user_mail_notify with a wrong $op value but phpstan didn't complain when running the commit checks. how do you make it work so that phpstan complains about it?
- π¦πΊAustralia dpi Perth, Australia
PHPStan level 5 is required for it to show up.
This is what you should expect to see:
phpstan: Parameter $xyz of method Class::method() expects 'blue'|'green'|'red', 'purple' given.
- π«π·France nod_ Lille
ok I see it, not hard but not trivial to set up so that it show up. I'm not sold on the benefits in this particular case, it doesn't help humans reading or writing code and the benefits to automated tools require a non-default setup.
It looks like working on the enum patch would be faster than changing the doc standards and committing this temporary solution