HTML API: Adjust code styling to Gutenberg's linter's preferences #4918

dmsnell · 2023-07-26T19:18:19Z

In this patch we're adjusting the code style according to the rules that the linting process in Gutenberg requires.

There are only a couple code changes that should have no effect on the runtime:

a missing check to verify that only UTF-8 is supported has been added (brought up because it was identified as an undefined variable)
a few return false; statements have been added to avoid having the linter complain that functions don't return a value despite indicating they return bool. the functions are stubs for coming support and currently throw, so the return statements are unreachable.

costdev

Thanks for the PR @dmsnell!

These changes look good to go! The same type of @see adjustments are needed elsewhere in Core too as they break the WordPress.org parser due to a lack of {}. We have a ticket for those but I don't believe the ones in this PR were covered by its patch. Great to see them in this PR (no pun intended).

dream-encode · 2023-07-26T20:04:21Z

src/wp-includes/html-api/class-wp-html-processor.php

@@ -232,16 +232,16 @@ class WP_HTML_Processor extends WP_HTML_Tag_Processor {
 	 * @return WP_HTML_Processor|null The created processor if successful, otherwise null.
 	 */
 	public static function createFragment( $html, $context = '<body>', $encoding = 'UTF-8' ) {
-		if ( '<body>' !== $context ) {
+		if ( '<body>' !== $context || 'UTF-8' !== $encoding ) {


@dmsnell The PR mentions that there should be no code changes. Is this change intentional?

good eye @dream-encode - I didn't mean to mislead there. this should be the single code change, and when I overlooked it it's because it was always intended to be the case to only support UTF-8 and bail if any others are passed. all the docs omit this argument but it's important to be there as we continue to build support into the class.

hopefully there should be no behavioral change, unless someone was intentionally trying to run this in an unsupported mode (which should not be the case as this only merged into trunk a few days ago).

I've updated the description to better explain

peterwilsoncc · 2023-07-26T22:56:11Z

src/wp-includes/html-api/class-wp-html-open-elements.php

I'm unclear, why are there so many unused variables and unimplemented functions in this file?

Hi @peterwilsoncc! thanks for taking a look.

the reason for this odd-seeming code is mostly to build out the interfaces we know we're going to need, but provide a way to implement HTML compliance without dumping a single massive patch.

we've chosen to build this in stages to give enough opportunity to review each stage and focus on testing each step and the implications of each step. in accordance with the philosophy started with the Tag Processor, we want to always avoid breaking, but we're find aborting. that is, this file and others have the scaffolding in place where we'll need to expand support when we add support for the kinds of HTML tags that make those functions relevant. today, it's not possible to get into a spot where those functions would be called, and where the variables are unused, it''s not possible to get into a spot where they would be meaningful.

they are there because that's what the function interface needs to be, and a large part of this is building a clear correspondence between the implementation code and the HTML specification; this in order to make it easier to expand and maintain and debug. most of these function names and interfaces are lifted directly from the spec.

as we continue to expand support in the HTML Processor, which we'll do at a pace that gives the ability to review and test and vet each additional bit of support, this will gradually fill up until those variables are all used and all the functions are fully implemented. the alternative is a giant code dump and a "let's hope this works and please trust that it does because there's no way we can examine it." 😄

for more insight into where this structure leads you can look at dmsnell#6 and adamziel#2.

For this specific patch we're mostly trying to clear up the obstacles that are preventing the code that has been merged into Core from being backported into Gutenberg.

I understand that it's being built up gradually, and agree it makes sense to, but I don't think it's a good idea to include the unused variables and unimplemented methods until they're required.

Including them locks WP into specifics that may need to change as the implementation is built out. I know this has been speced out quite thoroughly but including the unimplemented skeleton now makes promises to extenders that may not be kept.

Including them locks WP into specifics that may need to change as the implementation is built out. I know this has been spec'd out quite thoroughly but including the unimplemented skeleton now makes promises to extenders that may not be kept.

hm. this is exactly the point of adding them, to guard against updates that might overlook it or against people starting to rely on having those functions with one argument when they'll come back with more. we can provide dummy values, but I don't see that adding much.

also I think within weeks we might have a number of these filled out, so they may not last very long as unused. in fact, I'm hoping to have the use-case ready for the lightbox work in time for 6.4, and I think that will pull in lots of this in the process.

peterwilsoncc · 2023-07-26T23:10:30Z

src/wp-includes/html-api/class-wp-html-active-formatting-elements.php

@@ -149,7 +148,7 @@ public function remove_node( $token ) {
 	 *     > EM -> STRONG -> A ->
 	 *
 	 * To start with the most-recently added element and walk towards the top,
-	 * @see WP_HTML_Active_Formatting_Elements::walk_up
+	 * {@see WP_HTML_Active_Formatting_Elements::walk_up}.


Suggested change

* {@see WP_HTML_Active_Formatting_Elements::walk_up}.

* see WP_HTML_Active_Formatting_Elements::walk_up().

For functions methods, the developer.wordpress.org docs parser will pick up links if brackets are included, {@see is only needed for hooks. See get_posts() for an example:

wordpress-develop/src/wp-includes/post.php

Line 2392 in 444bdf4

* Optional. Arguments to retrieve posts. See WP_Query::parse_query() for all available arguments.

🔢 Applies throughout.

Are you suggesting we remove the brackets? If so, I have a couple questions:

although the docs parser might pick them up, what constitutes a link and how can we tell before the docs are published and updated if we did it right?

IDE support catches the {@see …} syntax as do all normal docblock parsing code. This is lost when removing the brackets. Are you saying you prefer removing the brackets and losing that support?

Are there any particular benefits to removing the brackets and the standard docblock tagging?

lthough the docs parser might pick them up, what constitutes a link and how can we tell before the docs are published and updated if we did it right?

For linking to hooks {@see 'some_hook_name'} constitutes a link (the quotes are required)

For linking to functions and methods function() and Class::method() constitute links (the brackets are required)

You can validate that you've done it correctly but running the docs parser locally. Rather than generating the full docs, it's best to specify the file when running the command to avoid waiting for the full docs to be generated wp parser create /path/to/file.php --user=<id|login>

Are you suggesting we remove the brackets?

Yeah, I'm suggesting removing the braces, and adding the brackets to the method names -- reviewing the WP Code it seems to be the standard. I'm basing this on a ticket opened by a meta team member after I was similarly unclear, see WP#45445. The key quote being:

The documentation standards mention the use of @see for referencing hooks, but could stand to be more explicit about not needing to be used for functions, classes, or class methods.

running the docs parser locally

thank you! I've asked before how I can preview the docs and this is the first time someone has indicated how. I'll poke around with that. seems like it needs to run on a copy of WordPress as a plugin.

I'm suggesting removing the braces, and adding the brackets to the method names

looks like I was confused on the distinction of braces and brackets and parenthesis.

I'm really bummed about removing syntax that works and breaks IDE integrations for that hides the links implicitly. do you know there's a preference not to use the standardized docblock syntax?

if I'm required to remove the curly brackets I'm more inclined to awkwardly rewrite the docblock so it can use a @see tag on its own. I'd be sad if people are prevented from navigating the code because this specific formalization isn't required in order for the docs to display correct.

any thoughts on this? is there wiggle-room to cater towards developers who are reading the code?

I thought there was nothing in the inline documentation standards that indicates "the right way" to do this, but I did see something in this section:

@see | elementname | References another function/method/class the function/method relies on. Should only be used inline for “linking” hooks.
(my emphasis)

There's certainly a difference between "should" and "must", and Core has instances of inline @see referencing functions. I'm not sure there's a solid answer to this one yet. We could probably do with some clarification in the standards.

I do see the very valid point about IDE support. I wonder if there's a reason why we shouldn't use {@see function_name()}, other than it being unnecessary for the docs parser.

FYI: I'd opened a ticket recently where I identified some @see tags broken on the parser. That ticket, and this PR could help standardize a consistent format regardless of what's being referenced, saving some brainpower.

I'm really bummed about removing syntax that works and breaks IDE integrations for that hides the links implicitly. do you know there's a preference not to use the standardized docblock syntax?

I'm really basing this on none of the existing code in core using @see for functions/methods (some external libraries do) and the ticket I linked to above.

FWIW, I think your point about supporting IDEs is a solid argument for starting a discussion on make/core. Apparently I don't have the right extension installed in VSCode for @see linking to work but it makes sense that IDEs would use it.

I'll poke around with that. seems like it needs to run on a copy of WordPress as a plugin.

I recall it being a little fiddly to get set up but worth it for testing potential edge cases.

Are we good to go with the syntax as written and continue considering the special syntax for WordPress in follow-up work that focuses on that PHPDoc tag and whether to allow the inline form?

I'd rather go with the existing standard in WP core, ie see WP_HTML_Active_Formatting_Elements::walk_up() and the equivalent elsewhere. A make/core post can be used to discuss whether we change the standard.

If this PR uses a special case, all that is going to happen is someone will do a follow up commit fixing the coding standards for consistency.

I will update to the existing pattern, though I thought we established it's not truly a standard; more like nothing else in Core has used this legitimate pattern.

Will try and post this quickly so it doesn't hold up the patch. Thanks @peterwilsoncc

updated in cdd785c

the language is awkward with see coming out of nowhere, so I had to smudge a few places and use a normal @see tag to avoid making it confusing, and we've lost the ability to lean on tools provided by the IDE, but the inline syntax is gone.

src/wp-includes/html-api/class-wp-html-token.php

ockham

LGTM; Sergey's suggestion makes sense to me, though.

ockham · 2023-08-07T13:49:59Z

Thanks all! Committed to Core in https://core.trac.wordpress.org/changeset/56363/.

dmsnell force-pushed the html-api/gutenberg-wpcs-issues branch 3 times, most recently from 633ec29 to a2cff5b Compare July 26, 2023 19:33

costdev approved these changes Jul 26, 2023

View reviewed changes

dream-encode reviewed Jul 26, 2023

View reviewed changes

peterwilsoncc reviewed Jul 26, 2023

View reviewed changes

dmsnell force-pushed the html-api/gutenberg-wpcs-issues branch from a2cff5b to 14a0ad8 Compare August 2, 2023 19:57

dmsnell mentioned this pull request Aug 2, 2023

HTML API: add get/set inner/outer markup #4956

Draft

10 tasks

SergeyBiryukov reviewed Aug 5, 2023

View reviewed changes

src/wp-includes/html-api/class-wp-html-token.php Outdated Show resolved Hide resolved

ockham approved these changes Aug 7, 2023

View reviewed changes

dmsnell added 2 commits August 7, 2023 15:26

HTML API: Adjust code stying to Gutenberg's linter's preferences

266012f

Adjust styling to avoid using PHPDoc syntax

1593b89

ockham force-pushed the html-api/gutenberg-wpcs-issues branch from c7c34c5 to 1593b89 Compare August 7, 2023 13:26

ockham changed the title ~~HTML API: Adjust code stying to Gutenberg's linter's preferences~~ HTML API: Adjust code styling to Gutenberg's linter's preferences Aug 7, 2023

ockham closed this Aug 7, 2023

dmsnell deleted the html-api/gutenberg-wpcs-issues branch August 8, 2023 05:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML API: Adjust code styling to Gutenberg's linter's preferences #4918

HTML API: Adjust code styling to Gutenberg's linter's preferences #4918

dmsnell commented Jul 26, 2023 •

edited by ockham

Loading

costdev left a comment

dream-encode Jul 26, 2023

dmsnell Jul 26, 2023

dmsnell Jul 26, 2023

peterwilsoncc Jul 26, 2023

dmsnell Jul 27, 2023

peterwilsoncc Jul 27, 2023

dmsnell Jul 28, 2023

peterwilsoncc Jul 26, 2023

dmsnell Jul 27, 2023

peterwilsoncc Jul 27, 2023

dmsnell Jul 28, 2023

costdev Jul 28, 2023 •

edited

Loading

peterwilsoncc Jul 28, 2023

dmsnell Aug 2, 2023

peterwilsoncc Aug 2, 2023

dmsnell Aug 3, 2023

dmsnell Aug 3, 2023

ockham left a comment

ockham commented Aug 7, 2023

	* {@see WP_HTML_Active_Formatting_Elements::walk_up}.
	* see WP_HTML_Active_Formatting_Elements::walk_up().

HTML API: Adjust code styling to Gutenberg's linter's preferences #4918

HTML API: Adjust code styling to Gutenberg's linter's preferences #4918

Conversation

dmsnell commented Jul 26, 2023 • edited by ockham Loading

costdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costdev Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham left a comment

Choose a reason for hiding this comment

ockham commented Aug 7, 2023

dmsnell commented Jul 26, 2023 •

edited by ockham

Loading

costdev Jul 28, 2023 •

edited

Loading