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

Type declarations #102

Merged
merged 1 commit into from
Aug 28, 2022
Merged

Type declarations #102

merged 1 commit into from
Aug 28, 2022

Conversation

dingo-d
Copy link
Member

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

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

The PR adds rules about the type declaration rules 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.

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
@dingo-d dingo-d requested a review from GaryJones August 20, 2022 11:24
@dingo-d
Copy link
Member Author

dingo-d commented Aug 20, 2022

I've added all the types currently available that cannot be used in core (still), but since the wording in the alert now excludes core I've reworded the phrase until the minimum version is raised to until the minimum PHP version is ... or higher. Because it didn't really make sense (as there is no mention of WordPress, so where the PHP minimum required version comes from wasn't really clear to me in the sentence context).

I'm open to rewording these statements if they read weird in any way.

@dingo-d dingo-d force-pushed the type-declaratons branch 3 times, most recently from b3609ef to 7193559 Compare August 20, 2022 12:36
@dingo-d dingo-d mentioned this pull request Aug 20, 2022
@dingo-d dingo-d marked this pull request as ready for review August 26, 2022 08:25
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.

Gone through it with a fine toothcomb....

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 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
@dingo-d dingo-d requested a review from jrfnl August 26, 2022 11:35
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
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.

See inline comments + the remark about the order of information.

Co-authored-by: Juliette <[email protected]>
Co-authored-by: Gary Jones <[email protected]>
@jrfnl jrfnl force-pushed the type-declaratons branch from f1bd412 to d24ab04 Compare August 28, 2022 02:06
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.

Approving. There is a stray space in line 425 "code[space][space]new to Core", which I will remove before merging.

Merging this now as @GaryJones is AFK for the next few weeks. If needs be, additional textual tweaks can still be made in a follow up commit.

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