Define order of object method modifiers as in PSR-12

+1 for this.
I'd just change the new text a bit to clearly reflect the non-visibility modifiers are optional:

All methods and properties of classes must specify their visibility: public, protected, or private. abstract and final, if present, must be declared before the visibility; static, if present, must be declared after the visibility. The PHP 4-style "var" declaration must not be used.

Also, not sure what our current stance on this is, but don't we want to capitalize the RFC 2119 verbs, at least when making changes to text?

+1 for standards :)

+1

Late +1 :)

+1

+1

Another +1 here.

This issue is being included in today's coding standards announcement, updating status and issue tags to follow the defined coding standards ratification process.

-1 to "must add public" that's too common to skip it as default in language

-1 to "must add public" that's too common to skip it as default in language

This was already enforced in the coding standards, see also the "old text" in the IS. This is just about adding a consistent order to the other keywords.

+1, and awesome that we can conform to the PSR-standard in this case!

+1

This seems like a good standard since it puts the most important information first. It's also minimally disruptive to adopt:

[ibnsina:drupal | Wed 14:52:10] $ grep -r "static protected" * | grep -v "vendor" | wc -l
      11
[ibnsina:drupal | Wed 14:52:20] $ grep -r "static public" * | grep -v "vendor" | wc -l
       3
[ibnsina:drupal | Wed 14:52:33] $ grep -r "static private" * | grep -v "vendor" | wc -l
       0
[ibnsina:drupal | Wed 14:52:43] $ grep -r "public abstract" * | grep -v "vendor" | wc -l
       0
[ibnsina:drupal | Wed 14:53:19] $ grep -r "protected abstract" * | grep -v "vendor" | wc -l
       0
[ibnsina:drupal | Wed 14:53:27] $ grep -r "public final" * | grep -v "vendor" | wc -l
       0
[ibnsina:drupal | Wed 14:55:00] $ grep -r "protected final" * | grep -v "vendor" | wc -l
       0

Let's do it.

*bump*

This issue has been RTBC for a while and in the interim PSR-2 has been deprecated ans PSR-12 is the recommended alternative. This was discussed at a coding standards meeting, [33294885] where there was agreement to use the text from PSR12, section 4.4 and 4.6. borission_ pointed out that in this issue klausi suggested not using uppercase must/should in the coding standards documentation on d.o. There was no disagreement with that and there is none in this issue. And there is an issue specifically addressing that, #1795750: Revise coding standards to use IETF RFC 2119 standards.

I have updated the IS to show the existing text and the proposed new text. Changing the status to NR.

And update the title.

The new text looks great, +1 for inclusion in the coding standards like this.

Agree, +1 to use the psr12 text.

This was discussed at a coding standards meeting, #3298982: Coding Standards Meeting 2022-08-02 2100 UTC. The suggested changes were agreed to and also commented here.

I think this now puts this issue at Step 6. I think it is OK to tag this as approved and 'needs documentation updates'.

Now for step 7. This needs to move to the core queue and marked RTBC awaiting "core committer signoff".

Issue moved, set to RTBC and now waiting for core committer signoff.

Seems fine to me and uncontroversial since we already do it. What's the next step?

As per https://www.drupal.org/project/coding_standards

Drupal.org coding standards pages are updated for approved proposals. Issues are marked "fixed" and “needs documentation updates” tag removed.

We have the text so let's do it.

Updated coding standard - https://www.drupal.org/docs/develop/standards/object-oriented-code#visib...

Crediting everyone involved.

Thanks for the approval.

@alexpott, thanks for updating the documentation and credit.

I am learning the CS standard process so checking the steps. Step 7 states "If approved by core committers, the issues should be left in RTBC, tagged with “Core Committer Approved” and moved back to the coding standards project queue." So, I am adding the tag and moving back to the Coding standards project queue. Step 8 was done by alexpott.

That leaves 9, "An announcement of all active discussions for the period will be made providing links to updated coding standards docs where appropriate." We are still working on how and where to make announcements. And, as this is already in practice we can probably forego it in this case.

Quote from the new docs (bold added by me for emphasis):

Method and function names must not be declared with space after the method name. The opening brace must go on its own line, and the closing brace must go on the next line following the body. There must not be a space after the opening parenthesis, and there must not be a space before the closing parenthesis.

I really don’t think this was intended or consensus, reading through the comments here and never seeing a single reference to changing this part of our coding standards. Should urgently be revisited, in my opinion. (Probably that whole paragraph should be removed again.)

I agree, that was not intended.

lolz someone tricked me!!!! I just copied and pasted from the issue summary! :D

We also removed

The use of public properties is strongly discouraged, as it allows for unwanted side effects. It also exposes implementation-specific details, which in turn makes swapping out a class for another implementation (one of the key reasons to use objects) much harder. Properties should be considered internal to a class.

Which still seems partially relevant. FWIW I'm a huge fan of readonly public properties but that's a PHP 8.1 only thing.

@drunken monkey, thanks for catching that!

I have updated the IS to remove the text in bold from #35 and to add a 'Properties' section for the re-added paragraph.

The suggested change has "Method names must not be prefixed with a single underscore to indicate protected or private visibility. That is, an underscore prefix explicitly has no meaning." which is already covered in Naming conventions point 4, "Classes should not use underscores in class names unless absolutely necessary to derive names inherited class names dynamically. That is quite rare, especially as Drupal does not mandate a class-file naming match."

Should we keep both?

That is quite rare, especially as Drupal does not mandate a class-file naming match

Well we do actually - that's PSR-0 and PSR-4...

FWIW the naming conventions point 4 are about class names and not method names. So I think the underscore thing and methods is worth keeping.

@alexpott, you are correct about #4. My brain must be getting ready early for my holiday.

Adding a follow up tag for #38.

Just converting to the new template

Match formatting of the PSR.
Considering this is updating documentation to what Drupal has been doing for a long time shall we skip the change record?

Re-evaluated the need for a followup and one isn't needed. Naming documentation is fine.

There was no objection to proceeding with this at the last meeting, #3490058: Coding Standards Meeting Wednesday 2025-01-28 2100 UTC

Documentation has been updated. The changes needed to be reviewed.

@borisson_ replied in #coding-standards that they reviewed the changes.

Therefor I am closing this issue as fixed.

Thanks!