#56791 closed task (blessed) (fixed)
Coding Standards fixes for WP 6.2
Reported by: | desrosj | Owned by: | |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch |
Focuses: | coding-standards | Cc: |
Attachments (11)
Change History (54)
This ticket was mentioned in PR #2435 on WordPress/wordpress-develop by @kebbet.
2 years ago
#2
- Keywords has-patch added
#7
follow-up:
↓ 8
@
2 years ago
In references to ticket:56969#comment:33 and ticket:56969#comment:35 (pinging @peterwilsoncc and @adamsilverstein), I've attached a patch to replace a strpos()
check with modern (and polyfilled) str_contains()
.
The question that arises: There are about 60 false === strpos( ... )
and 150 false !== strpos( ... )
in Core that could be replaced by ! str_contains( ... )
and str_contains( ... )
. Should these be batch-replaced or rather left alone until the respective area of code is touched anyways? Likewise, possible str_starts_with()
and str_ends_with()
usage could be introduced.
#8
in reply to:
↑ 7
@
2 years ago
Replying to TobiasBg:
The question that arises: There are about 60
false === strpos( ... )
and 150false !== strpos( ... )
in Core that could be replaced by! str_contains( ... )
andstr_contains( ... )
. Should these be batch-replaced or rather left alone until the respective area of code is touched anyways? Likewise, possiblestr_starts_with()
andstr_ends_with()
usage could be introduced.
I am in favor of making a batch replacement to use str_contains()
, str_starts_with()
, and str_ends_with()
where appropriate, for readability and consistency.
#10
follow-up:
↓ 11
@
2 years ago
While updating the deprecated functions list in WordPressCS, I noticed a typo in the call to the _deprecated_function()
function for the _filter_query_attachment_filenames()
function.
I've attached a patch to fix it.
The issue was introduced in [54524] /cc @audrasjb
Even though it is a small fix, it is misleading, so I wonder if the patch should be backported ?
#11
in reply to:
↑ 10
;
follow-up:
↓ 13
@
2 years ago
Replying to jrf:
Even though it is a small fix, it is misleading, so I wonder if the patch should be backported ?
Thanks for the patch!
It looks like the issue is only in these two commits: [54524], [54534], i.e. in trunk and the 6.0 branch. Backports to other branches have the correct version already.
Backporting to the 6.0 branch makes sense to me.
#13
in reply to:
↑ 11
@
2 years ago
Replying to SergeyBiryukov:
It looks like the issue is only in these two commits: [54524], [54534], i.e. in trunk and the 6.0 branch. Backports to other branches have the correct version already.
Backporting to the 6.0 branch makes sense to me.
Ah, it was before 6.1 was branched, so it should also be backported to the 6.1 branch now :)
#14
@
2 years ago
Thanks @SergeyBiryukov And yes, I concur with your assessment - backports to 6.0 and 6.1 only (issue didn't exist before that).
@
2 years ago
Follow up to previous efforts to make sure visibility is declared on all methods. Note: this will be enforced by WPCS 3.0.0.
@
2 years ago
Follow up to previous efforts to make sure visibility is declared on all properties. Note: this will be enforced by WPCS 3.0.0.
@
2 years ago
Follow up to previous efforts for the same. Note: this will be enforced by WPCS 3.0.0.
@
2 years ago
This fixes three out of the four currently flagged issues for in_array()
usage. These three all do comparisons with strings, so all the more reason why it is imperative that a strict comparison is used.
@
2 years ago
Note: if I'm honest, this whole function declaration should be changed as it is now declared as a function in the global namespace, while only intended for one test, but that's outside the scope of this patch.
#20
@
2 years ago
I've just uploaded 9 patches to fix a variety of coding standards issues. Most a very simple and easy merges. The only "big" ones are the "always use parentheses when instantiating an object" and "always declare visibility for methods" patches.
This ticket was mentioned in PR #3873 on WordPress/wordpress-develop by @SergeyBiryukov.
2 years ago
#34
Trac ticket: https://core.trac.wordpress.org/ticket/56791
#38
in reply to:
↑ 37
;
follow-up:
↓ 39
@
22 months ago
Replying to SergeyBiryukov:
While matching the database field of the same name, the
$comment_ID
variable did not follow the WordPress coding standards...
I like this change, seems to make sense imho. However there is nothing about this in the WP coding standards (as far as I see). Seems the implementation of the standards in WPCS is wrong :)
#39
in reply to:
↑ 38
@
22 months ago
Replying to azaozz:
Replying to SergeyBiryukov:
While matching the database field of the same name, the
$comment_ID
variable did not follow the WordPress coding standards...
I like this change, seems to make sense imho. However there is nothing about this in the WP coding standards (as far as I see). Seems the implementation of the standards in WPCS is wrong :)
From the handbook:
Use lowercase letters in variable, action/filter, and function names (never camelCase). Separate words via underscores. Don’t abbreviate variable names unnecessarily; let the code be unambiguous and self-documenting.
Ref: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#naming-conventions
Note: this phrase has been in the handbook since as long as I remember. Also see the original import to the Git repo from five years ago in which the exact same phrase is included.
Conclusion: WPCS follows the handbook to the letter and is doing nothing wrong here.
#41
@
22 months ago
- Resolution set to fixed
- Status changed from new to closed
The last schedule 6.3 beta happened today and next week is RC1. I'm closing this ticket for the cycle. However, please feel to reopen if coding standards fixes are needed for 6.2.
#57839 is now open for the continuation of work in the 6.3 cycle.
Thank you everyone for your contributions :)
@SergeyBiryukov commented on PR #2435:
4 months ago
#42
Thanks for the PR! Merged in r58888.
@SergeyBiryukov commented on PR #3873:
3 months ago
#43
This will need tests before it can be merged:
Indeed, this is basically still a draft where I cherry-pick certain changes after a closer review, making sure they are covered by new or existing tests. I will investigate anything related to wp_parse_args()
and WP_Query
, thanks!
Maybe this PR can be included in 6.2, after being overseen in 6.0 and 6.1.
https://core.trac.wordpress.org/ticket/56791