Skip to content
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

Property and method modifier order #108

Merged
merged 3 commits into from
Sep 25, 2022

Conversation

dingo-d
Copy link
Member

@dingo-d dingo-d commented Aug 20, 2022

This PR depends on #107. Once that is merged, this PR should be rebased to the master branch.

The PR adds rules about the order of property and method modifiers and is the continuation of the additions of 'modern' PHP code in the WordPress PHP Coding Standards handbook based on the make post by Juliette Reinders Folmer.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Should we mention the readonly keyword (PHP 8.1 properties) and property type declarations (PHP 7.4) somewhere in this section ? Or leave it for now until there is sight of a WP minimum PHP version bump ?

Or maybe we should split this into two paragraphs ?

  • Properties (visibility, static, readonly, type)
  • Methods (abstract/final, visibility, static)

@dingo-d dingo-d marked this pull request as ready for review August 26, 2022 08:24
@dingo-d
Copy link
Member Author

dingo-d commented Aug 26, 2022

If we add readonly and we split this to properties and methods, should we have a paragraph for readonly classes as well?

So we'd have:

  • Classes (abstract/final, readonly)
  • Properties (visibility, static, readonly, type)
  • Methods (abstract/final, visibility, static)

@jrfnl @GaryJones any thoughts on this?

@GaryJones
Copy link
Member

I think this needs rebasing to be easier to review.

@dingo-d dingo-d force-pushed the property-and-method-modifier-order branch from d4cce1b to fbb5370 Compare September 9, 2022 08:38
@dingo-d
Copy link
Member Author

dingo-d commented Sep 9, 2022

@GaryJones I've rebased it, should be easier to review now 🙂

@jrfnl
Copy link
Member

jrfnl commented Sep 9, 2022

If we add readonly and we split this to properties and methods, should we have a paragraph for readonly classes as well?

We just discussed this and I agree, I think having a separate paragraph for each would be good. Though IMO that would mean four paragraphs:

  • Classes (abstract/final, readonly) => mind: enums also have a type (but that goes after the enum keyword.
  • Constants (visibility) - not that interesting yet as there is only one type of modifier allowed, but there has been talk about allowing type declarations there too
  • Properties (visibility, static, readonly, type) - note: static/readonly are currently mutually exclusive.
  • Methods (abstract/final, visibility, static)

@dingo-d dingo-d force-pushed the property-and-method-modifier-order branch 3 times, most recently from 5119442 to cce5363 Compare September 9, 2022 11:38
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Reviewed. I hope I caught everything.

Not sure we'd want to have the PHP #.#+ markings inline with the actual order rules, but there were some "if available"s, so either do it with all or don't do it with any of them and just let it be noted in the PHP info block.

wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
wordpress-coding-standards/php.md Show resolved Hide resolved
wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
@dingo-d
Copy link
Member Author

dingo-d commented Sep 9, 2022

I'd leave out the PHP comments in the brackets and just have the [info] shortcode at the end.

@dingo-d dingo-d force-pushed the property-and-method-modifier-order branch from 4381914 to 5bccf26 Compare September 9, 2022 12:23
@dingo-d dingo-d requested a review from GaryJones September 9, 2022 12:23
@dingo-d dingo-d force-pushed the property-and-method-modifier-order branch from 5bccf26 to 7f6c1c1 Compare September 9, 2022 12:24
Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

I wonder if this could all be simplified down as per https://www.php-fig.org/psr/psr-12/#46-abstract-final-and-static, which says:

When present, the abstract and final declarations MUST precede the visibility declaration.

When present, the static declaration MUST come after the visibility declaration.

We'd need to add in readonly, and types, but it seems that it could then be covered in just one or two code snippets.

@dingo-d
Copy link
Member Author

dingo-d commented Sep 12, 2022

I'm fine with having one code example covering all the things, we opted for splitting it into several paragraphs for easier maintenance.

Leave only one code example, add bullet points for the info block.
@dingo-d
Copy link
Member Author

dingo-d commented Sep 23, 2022

@GaryJones I've made some changes based on the discussion I've had with Juliette:

  • Left the specific parts about the order for certain OO constructs
  • Create only one code example with correct and incorrect usage
  • Added bullets to the info block (we have a similar info block for the type declaration)

Does this seem a bit more manageable?

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Just one small comment, other than that: LGTM!

wordpress-coding-standards/php.md Outdated Show resolved Hide resolved
Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

@GaryJones GaryJones merged commit 54ada87 into master Sep 25, 2022
@GaryJones GaryJones deleted the property-and-method-modifier-order branch September 25, 2022 19:46
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.

3 participants