Backport: Add filter for duotone to account for gutenberg_restore_image_outer_container in classic themes #6607

scruffian · 2024-05-22T16:42:06Z

Trac ticket: https://core.trac.wordpress.org/ticket/61271

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

…ge_outer_container in classic themes

github-actions · 2024-05-22T16:42:24Z

Trac Ticket Missing

This pull request is missing a link to a Trac ticket. For a contribution to be considered, there must be a corresponding ticket in Trac.

To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. More information about contributing to WordPress on GitHub can be found in the Core Handbook.

github-actions · 2024-05-22T16:42:27Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props scruffian, isabel_brison, ajlende.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-05-22T16:55:55Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

tellthemachines

Hmm, this changeset doesn't match the one from WordPress/gutenberg#59764, which adds a new restore_image_outer_container function to the duotone class. Is that intentional?

I also see in testing that this is removing the outer wrapper on aligned images, instead of moving the duotone classname to it (which is what happens in the plugin after WordPress/gutenberg#59764)

ajlende · 2024-06-03T23:13:28Z

@tellthemachines I have the correct backport at #6719

scruffian · 2024-06-03T23:38:59Z

@tellthemachines you're right I had done this wrongly. I have updated the PR with the correct approach (I hope!)

I had incorrectly read the original PR and thought it was removing the restore_image_outer_container filter, but it was actually removing the Duotone specific filter.

ajlende · 2024-06-03T23:48:10Z

src/wp-includes/block-supports/duotone.php

@@ -57,3 +57,7 @@

 // Migrate the old experimental duotone support flag.
 add_filter( 'block_type_metadata_settings', array( 'WP_Duotone', 'migrate_experimental_duotone_support_flag' ), 10, 2 );
+
+// Restore duotone classes to the image wrapper div.
+add_filter( 'render_block_core/image', array( 'WP_Duotone', 'restore_image_outer_container' ), 11, 2 );


Suggested change

add_filter( 'render_block_core/image', array( 'WP_Duotone', 'restore_image_outer_container' ), 11, 2 );

add_filter( 'render_block_core/image', array( 'WP_Duotone', 'restore_image_outer_container' ), 10, 1 );

I think we might have to keep the higher priority because the filter isn't working on your other PR @ajlende, but it's working on this one.

I forgot that this was weird in the PR. Things have to be loaded in a very specific order. WordPress/gutenberg#59764 (comment)

When this gets backported to core, the same should happen with the core version of the functions. Although, it looks like the imports will have to be reordered during the core backport (WordPress/wordpress-develop@9a616a5/src/wp-settings.php#L371).

In my opinion, with the way WordPress code is written with side effects in many files, the way that we mostly centralize imports, the size of the codebase, and the number of contributors, it makes more sense for use to just preserve the order of imports when backporting things to not have to worry so much about priority levels. It seems better to me to maintain that order than force extenders to use extra high priorities to override core behavior.

@tellthemachines I don't have permissions to push to this branch, but I updated the import order to match Gutenberg in #6719 and that seems to work for me.

Cool we can go with that one then!

ajlende · 2024-06-03T23:48:35Z

src/wp-includes/class-wp-duotone.php

+	 * Fixes the issue with our generated class name not being added to the block's outer container
+	 * in classic themes.
+	 *
+	 * @since 6.5.0


Suggested change

* @since 6.5.0

* @since 6.6.0

ajlende · 2024-06-03T23:48:49Z

src/wp-includes/class-wp-duotone.php

+		return $tags->get_updated_html();
+	}
+
+


Suggested change

ajlende · 2024-06-03T23:49:04Z

src/wp-includes/block-supports/duotone.php

+
+// Restore duotone classes to the image wrapper div.
+add_filter( 'render_block_core/image', array( 'WP_Duotone', 'restore_image_outer_container' ), 11, 2 );
+


Suggested change

tellthemachines · 2024-06-04T01:48:06Z

Closing this one in favour of #6719.

mukeshpanchal27 · 2024-06-04T04:08:21Z

src/wp-includes/class-wp-duotone.php

+	 * @since 6.5.0
+	 *
+	 * @param  string $block_content Rendered block content.
+	 * @return string                Filtered block content.


@tellthemachines @return string Filtered block content. space should be removed.

Fixed! https://core.trac.wordpress.org/changeset/58316

Backport: Add filter for duotone to account for gutenberg_restore_ima…

7555c24

…ge_outer_container in classic themes

scruffian mentioned this pull request May 22, 2024

PHP backports for WP 6.6 WordPress/gutenberg#61226

Closed

tellthemachines reviewed May 29, 2024

View reviewed changes

restore the old function and add a new one

725ff24

ajlende reviewed Jun 3, 2024

View reviewed changes

src/wp-includes/class-wp-duotone.php

return $tags->get_updated_html();

}

Copy link

ajlende Jun 3, 2024

Choose a reason for hiding this comment

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

Suggested change

ajlende reviewed Jun 3, 2024

View reviewed changes

tellthemachines closed this Jun 4, 2024

mukeshpanchal27 reviewed Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport: Add filter for duotone to account for gutenberg_restore_image_outer_container in classic themes #6607

Backport: Add filter for duotone to account for gutenberg_restore_image_outer_container in classic themes #6607

scruffian commented May 22, 2024 •

edited

Loading

github-actions bot commented May 22, 2024

github-actions bot commented May 22, 2024 •

edited

Loading

github-actions bot commented May 22, 2024

tellthemachines left a comment

ajlende commented Jun 3, 2024

scruffian commented Jun 3, 2024

ajlende Jun 3, 2024

tellthemachines Jun 3, 2024

ajlende Jun 4, 2024

ajlende Jun 4, 2024

tellthemachines Jun 4, 2024

ajlende Jun 3, 2024

ajlende Jun 3, 2024

ajlende Jun 3, 2024

tellthemachines commented Jun 4, 2024

mukeshpanchal27 Jun 4, 2024

tellthemachines Jun 4, 2024

	add_filter( 'render_block_core/image', array( 'WP_Duotone', 'restore_image_outer_container' ), 11, 2 );
	add_filter( 'render_block_core/image', array( 'WP_Duotone', 'restore_image_outer_container' ), 10, 1 );


		// Restore duotone classes to the image wrapper div.
		add_filter( 'render_block_core/image', array( 'WP_Duotone', 'restore_image_outer_container' ), 11, 2 );

Backport: Add filter for duotone to account for gutenberg_restore_image_outer_container in classic themes #6607

Backport: Add filter for duotone to account for gutenberg_restore_image_outer_container in classic themes #6607

Conversation

scruffian commented May 22, 2024 • edited Loading

github-actions bot commented May 22, 2024

Trac Ticket Missing

github-actions bot commented May 22, 2024 • edited Loading

github-actions bot commented May 22, 2024

Test using WordPress Playground

Some things to be aware of

tellthemachines left a comment

Choose a reason for hiding this comment

ajlende commented Jun 3, 2024

scruffian commented Jun 3, 2024

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

tellthemachines commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scruffian commented May 22, 2024 •

edited

Loading

github-actions bot commented May 22, 2024 •

edited

Loading