Skip to content
This repository was archived by the owner on Jul 28, 2023. It is now read-only.

Fix wp_process_directives output#150

Closed
DAreRodz wants to merge 1 commit into
main-wp-directives-pluginfrom
fix/wp-process-directives
Closed

Fix wp_process_directives output#150
DAreRodz wants to merge 1 commit into
main-wp-directives-pluginfrom
fix/wp-process-directives

Conversation

@DAreRodz

Copy link
Copy Markdown
Collaborator

What

Fixes a couple of bugs while processing directives and generating the new HTML.

Why

The HTML output is basically $block_content, unchanged. 😅

How

  1. Returning the updated HTML from the WP_HTML_Tag_Processor.
  2. Getting the directive type from the attribute (type:name) before looking for the directive function.
  3. Using the already implemented wp-context attribute.

I guess we'll have to implement tests for wp_process_directives sooner than later. ☝️

@DAreRodz DAreRodz added the bug Something isn't working label Feb 14, 2023
@DAreRodz DAreRodz requested a review from ockham February 14, 2023 18:34
@ockham

ockham commented Feb 15, 2023

Copy link
Copy Markdown
Collaborator

Good spot, thank you!

Note that #144 changes wp_process_directives a bit: Rather than an HTML string, it will accept a WP_HTML_Tag_Processor object (typically created as $tags = WP_HTML_Tag_Processor( $html );), and return the modified $tags. This means that in order to plug it into the render_block filter (or anything that handles HTML strings really), we have to add a layer of indirection where that $tags object is created (also see #144).

This means that #144 (which has approval from @luisherranz 🎉 ) will conflict with this PR. There's another change in #144 related to the aforementioned indirection that might come in handy for Woo: wp_process_directives now also accepts separate arguments for prefix and lists of tag and attribute directives -- meaning it will be fully agnostic of those.

LMK if it's okay to merge #144 (I can help you carry over any necessary changes to Woo), or if you'd like to proceed differently 😊

@DAreRodz

Copy link
Copy Markdown
Collaborator Author

LMK if it's okay to merge #144

@ockham, sure! We can merge #144 and adapt later this PR's changes (or even close it if it makes more sense).

@ockham

ockham commented Feb 15, 2023

Copy link
Copy Markdown
Collaborator

Great, thank you for confirming @DAreRodz !

@ockham

ockham commented Feb 15, 2023

Copy link
Copy Markdown
Collaborator

Merged #144.

I guess we'll have to implement tests for wp_process_directives sooner than later. ☝️

I forgot to say, #144 also added basic coverage for that function.

@DAreRodz

Copy link
Copy Markdown
Collaborator Author

Great. 😊 @ockham, I'm closing this PR as most of the fixes were already addressed in your PR.

I think the part about getting the directive processors is still broken. My guess is that, if directive attributes have a colon (e.g., wp-bind:value), they won't be found inside $attribute_directives.

https://github.com/WordPress/block-hydration-experiments/blob/b3df431c3436bb5f41fede36b9307cd5d770dd67/src/directives/wp-process-directives.php#L33-L34

I'll take a look and open a new PR if necessary. 🕵️

@DAreRodz DAreRodz closed this Feb 16, 2023
@DAreRodz

Copy link
Copy Markdown
Collaborator Author

I'll take a look and open a new PR if necessary. 🕵️

My guess was right. 😅 I created a bugfix here: #151

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants