#59547 closed defect (bug) (fixed)
Rename WP_HTML_Processor::createFragment() to follow the coding standards
Reported by: | SergeyBiryukov | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | 6.4 |
Component: | HTML API | Keywords: | has-patch has-unit-tests commit |
Focuses: | coding-standards | Cc: |
Description
As per the Naming Conventions section:
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.
function some_name( $some_variable ) {}
WP_HTML_Processor::createFragment()
appears to be only method in the HTML API classes that does not follow the snake_case format.
Normally, this would display an error in WPCS checks:
---------------------------------------------------------------------- FOUND 1 ERROR AFFECTING 1 LINE ---------------------------------------------------------------------- 242 | ERROR | Method name "createFragment" in class | | WP_HTML_Processor is not in snake case format, try | | "create_fragment" | | (WordPress.NamingConventions.ValidFunctionName.MethodNameInvalid) ----------------------------------------------------------------------
However, WPCS — or rather PHPCS — is only aware of the current file, so, aside from PHP native classes being extended/interface implemented, WPCS cannot check whether the method is one being overloaded from the parent class/interface or is one native to the child class. To prevent false positives which aren't fixable, WPCS checks if a class extends/implements and bows out if it does. This false positive protection has been in place since WPCS 0.10.0 (2016).
Since WP_HTML_Processor
extends WP_HTML_Tag_Processor
, WPCS does not catch the error in this case, and it should have been caught in code review.
WP_HTML_Processor::create_fragment()
would be the correct method name to match the coding standards.
Follow-up to [56274].
Props to @jrf for a clarification on the WPCS behavior.
Attachments (1)
Change History (13)
#2
@
15 months ago
I wonder:
Is the method being used by any of the node packages?
If no, then it should be safe to rename as @jrf mentions.
If it's being used within the Gutenberg plugin, adjustments will need to be made there too.
cc @dmsnell
#3
@
15 months ago
Thanks all for catching this. I was head's down in the HTML5 spec and copying its idiom. I'll review to make sure there are no missed references, but assuming this passes tests and the code has been changed I approve. I'll come back later once I've ran my assessment, but I don't think this ticket needs to wait to merge on that account.
Thanks for the explanation on WPCS/PHPCS as well. I'll try and pay extra attention to methods in extending classes because of this.
Edit: I meant to say also that I think it's important to get this in ASAP in the 6.4 release so we don't publish it and have to stick with it forever.
#4
follow-up:
↓ 5
@
15 months ago
If it's being used within the Gutenberg plugin, adjustments will need to be made there too.
It is not, @hellofromTonya, and I will handle the backport of this change to Gutenberg so that its class there gets updated accordingly
#5
in reply to:
↑ 4
;
follow-up:
↓ 7
@
15 months ago
- Keywords changes-requested added
Replying to dmsnell:
If it's being used within the Gutenberg plugin, adjustments will need to be made there too.
It is not, @hellofromTonya, and I will handle the backport of this change to Gutenberg so that its class there gets updated accordingly
Awesome. Thanks for confirming @dmsnell :)
Hey @SergeyBiryukov, the tests also need updating.
This ticket was mentioned in PR #5415 on WordPress/wordpress-develop by @SergeyBiryukov.
15 months ago
#6
- Keywords has-unit-tests added
Trac ticket: https://core.trac.wordpress.org/ticket/59547
#7
in reply to:
↑ 5
@
15 months ago
- Keywords changes-requested removed
Replying to hellofromTonya:
Hey @SergeyBiryukov, the tests also need updating.
Thanks! PR 5415 updates the tests as well.
#9
@
14 months ago
Following-up from earlier: everything in the current form of PR 5415 looks correct to me. Much obliged.
#10
@
14 months ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 56790:
@SergeyBiryukov commented on PR #5415:
14 months ago
#11
Thanks everyone! Merged in r56790.
@SergeyBiryukov I've had a quick look at the patch and based on the patch alone (without having done a code search), it looks good.
As this is a new method introduced in WP 6.4.0, i.e. a method which has not been in a published release yet, I think it's okay to rename the method without adding a stub deprecating the original name.
I do wonder if this should be mentioned somewhere in case people have already started building/adjusting code based on the first WP 6.4 beta releases ?