Improve documentation of _user_mail_notify

dpi → changed the visibility of the branch 3463923-notif-types-string-param to hidden.

Seems pretty straight forward

Would an Enum fit in here?

Would an Enum fit in here?

Per issue summary, and ✨ Use constants for user email notification types Needs work

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...

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.

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.

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.

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

Why would coding standards need an update?

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-... →

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.

To answer the question above, here is what it would look like

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?

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.

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