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

Tag Processor: Remove the shorthand next_tag( $tag_name ) syntax #44595

Closed
wants to merge 8 commits into from

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Sep 30, 2022

A part of #44410

What?

In @dmsnell's words:

My worry is that we say you can search for a given tag name, but then people want a short hand for class name, so we add it, but then if you don't specify a tag you have ->next( null, 'wp-block-group' ) everywhere. Or someone thinks, let's put class name first, and then we have the other problem.

I'd even be for exposing this shorthand only after we see use in practice to see if people even want it, and if so, which convention is more dominant.

Alternatively it's always easy to later add new specialized methods. Suppose we only expose ->next() right now, taking a query object. we can add ->next_tag( 'img' ) and ->next_with_class( 'wp-block-group' ). performance wise this is a "free" option

It resonates with me, so this PR removes the shorthand syntax and renames the long tag_name and class_name keys to short tag and class ones.

Before:

$p->next_tag( 'p' );

After:

$p->next( array( 'tag' => 'p' ) );

Testing Instructions

Confirm the CI passed and that no next_tag, tag_name, and class_name mentions were missed in this refactoring.

cc @dmsnell

@adamziel adamziel changed the title Remove the shorthand next_tag( $tag_name ) syntax Tag Processor: Remove the shorthand next_tag( $tag_name ) syntax Sep 30, 2022
@adamziel adamziel self-assigned this Sep 30, 2022
@adamziel adamziel added [Type] Enhancement A suggestion for improvement. Developer Experience Ideas about improving block and theme developer experience labels Sep 30, 2022
@dmsnell
Copy link
Member

dmsnell commented Sep 30, 2022

Unfortunately there's already a branch named tag-processor so I can't checkout this branch as usual (because the tag name conflicts with the existing tag through the /).

If you also want to check it out locally, rename it.

git checkout -b update/tag-processor-cleanup-api origin/tag-processor/cleanup-api

if ( null !== $query ) {
if ( ! is_array( $query ) || $query === $this->last_query ) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

the nesting catches my attention.

I don't think we need to change this code at all because we only set the query parameters if we have an array with the given keys. isset() is inherently safe to call.

it would be different if we did something to alert a dev of passing a non-array value, but right now there's no difference between passing a non-array and passing an array with the wrong keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, good point! I just reverted this part

@adamziel
Copy link
Contributor Author

Unfortunately there's already a branch named tag-processor so I can't checkout this branch as usual (because the tag name conflicts with the existing tag through the /).

If you also want to check it out locally, rename it.

git checkout -b update/tag-processor-cleanup-api origin/tag-processor/cleanup-api

Oops, sorry about that! Let's close this one and continue in #45082

@adamziel adamziel closed this Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants