Skip to content

Image Block: Add data-id attribute on server side render for core galleries #35975

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.

Sign up for GitHub

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

Merged
adamziel merged 8 commits into from
Nov 5, 2021
Merged

Image Block: Add data-id attribute on server side render for core galleries #35975

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b0cddec
Adding a server render_callback in order to add a data-id attribute t…
ramonjd Oct 27, 2021
a0b5948
Switch to using the_post instead of server side block render
Nov 1, 2021
6af28ef
Change order of attribs and check for innerBlocks
Nov 2, 2021
48975cd
Reinstating render_callback
ramonjd Nov 3, 2021
15e8532
This commit adds a data-id attribute to every image innerblock of a g…
ramonjd Nov 3, 2021
fcd872f
NEWSFLASH! Array keys must be surrounded by spaces unless they cont…
ramonjd Nov 3, 2021
a958531
Adding a render function so that block based themes render the galler…
ramonjd Nov 4, 2021
ff4ba58
Add guards for undefined indexes
Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Change order of attribs and check for innerBlocks
  • Loading branch information
@ramonjd
Glen Davies authored and ramonjd committed Nov 5, 2021
commit 6af28ef59ba53e53dedb17f678fec76250eab37f
10 changes: 5 additions & 5 deletions packages/block-library/src/image/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
/**
* Adds a data-id attribute to the core Image block when nested in a Gallery block.
*
* @param WP_Post $post The block attributes.
* @return string Returns the block content with the data-id attribute added.
* @param WP_Post $post The WP post.
* @return string Returns the post content with the data-id attribute added to gallery images.
*/
function get_block_core_image_post_content( $post ) {
if ( is_admin() ) {
Copy link
Member Author

@ramonjd ramonjd Nov 2, 2021

Choose a reason for hiding this comment

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

Suggested change
if ( is_admin() ) {
if ( is_admin() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ) {

Is there are case where we'd want to run this code for rest requests?

I'm thinking it's safe to return early if we're targeting post data on the frontend, unless I'm missing a use case.

	if ( is_admin() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ) {
		return;
	}

For example, add a var_dump('I LOVE THE BAND KISS!') or some other logging after this if statement, and try to save a post. It doesn't have to contain a gallery.

The POST response will contain invalid, but possibly true, content: string(21) "I LOVE THE BAND KISS!" {id: 115, date: "2021-11-02T03:18:06", date_gmt: "2021-11-02T03:18:06",…}

Pinging a WP API endpoint that contains gallery post content still works for me with this check in place:

E.g., http://localhost:4759/wp-json/wp/v2/posts/104
Screen Shot 2021-11-02 at 2 05 43 pm

Copy link
Member

@TimothyBJacobs TimothyBJacobs Nov 2, 2021

Choose a reason for hiding this comment

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

I'd be somewhat wary about using is_admin() in a condition check here. All it means is that the context when this post is setup is happening without wp-admin, it doesn't actually mean that the post is being edited or the like. Somewhat could just as easily be expecting to get this data over an admin-ajax.php call or simply trying to render their gallery in an admin page.

The same goes for REST_REQUEST. It can both be used in the Block Editor. But it can also be used by someone making an API call to grab data for somewhere on the front-end of their website where they would expect the ids to be present.

Copy link
Member Author

@ramonjd ramonjd Nov 2, 2021

Choose a reason for hiding this comment

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

Thanks for the advice @TimothyBJacobs 🙇

I think @glendaviesnz was trying to find ways to isolate the logic to frontend rendering to avoid block validation errors, given that data-id isn't output by the save method of the image block.

🤔 Assuming block validation wasn't an issue, do you see any issues with removing the if check here entirely?

Cheers!

Copy link
Member

@TimothyBJacobs TimothyBJacobs Nov 2, 2021

Choose a reason for hiding this comment

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

Assuming block validation wasn't an issue, do you see any issues with removing the if check here entirely?

No, I think removing the check entirely would be ideal.

Expand All @@ -19,14 +19,14 @@ function get_block_core_image_post_content( $post ) {
$content = $post->post_content;
$blocks = parse_blocks( $content );
foreach ( $blocks as $block ) {
if ( 'core/gallery' === $block['blockName'] ) {
if ( 'core/gallery' === $block['blockName'] && ! empty( $block['innerBlocks'] ) ) {
foreach ( $block['innerBlocks'] as $inner_block ) {
if ( 'core/image' === $inner_block['blockName'] ) {
if ( isset( $inner_block['attrs']['id'] ) ) {
$image_id = esc_attr( $inner_block['attrs']['id'] );
$data_id_attribute = 'data-id="' . $image_id . '"';
$class_string = 'class="wp-image-' . $image_id . '"';
$content = str_replace( $class_string, $class_string . ' ' . $data_id_attribute, $content );
$content = str_replace( $class_string, $data_id_attribute . ' ' . $class_string, $content );
}
}
}
Expand All @@ -36,4 +36,4 @@ function get_block_core_image_post_content( $post ) {
$post->post_content = $content;
}

add_action( 'the_post', 'get_block_core_image_post_content', 10, 2 );
add_action( 'the_post', 'get_block_core_image_post_content', 10, 1 );