WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 3 months ago

#43826 assigned task (blessed)

get_post_galleries() should return gallery blocks

Reported by: pento Owned by: tellyworth
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Media Keywords: needs-patch
Focuses: template Cc:
PR Number:

Description

Originally reported here: https://github.com/WordPress/gutenberg/issues/5797

Galleries in Gutenberg don't use the [gallery] shortcode, so aren't returned by get_post_galleries(). This function should check for gallery blocks, too.

Attachments (7)

43826.patch (3.2 KB) - added by Takahashi_Fumiki 13 months ago.
Add gutenberg block parser for get_post_galleries
43826.2.diff (5.5 KB) - added by tellyworth 13 months ago.
43826.3.diff (9.5 KB) - added by tellyworth 12 months ago.
43826.4.diff (13.1 KB) - added by tellyworth 12 months ago.
43826.5.diff (14.7 KB) - added by tellyworth 12 months ago.
43826.6.diff (15.3 KB) - added by audrasjb 11 months ago.
patch refreshed
43826.diff (16.3 KB) - added by desrosj 10 months ago.

Download all attachments as: .zip

Change History (45)

@Takahashi_Fumiki
13 months ago

Add gutenberg block parser for get_post_galleries

#2 @Takahashi_Fumiki
13 months ago

I added a patch above.

  • Avoid quick return with checking shortcode existence.
  • Add gut block parser.

Questions

  • I think this needs new tests because no test case for block format strings.
  • if $html attribute is true, simply added block content html. But this output html is apparently different from former shortcode empowered gallery html.
  • I'm afraid that there might be php gutenberg parser in core.

#3 @birgire
13 months ago

@Takahashi_Fumiki

Thanks for the patch.

When I add a gallery I get something like this:

<!-- wp:gallery -->
<ul class="wp-block-gallery columns-3 is-cropped">
	<li class="blocks-gallery-item">
		<figure>
			<img src="https://example.com/wp-content/uploads/2018/11/image.jpg" alt="" data-id="123" data-link="https://example.com/testing/image/" class="wp-image-123"/>
			<figcaption>Test image.</figcaption>
		</figure>
	</li>
        <li>...cut...</li>
        <li>...cut...</li>
</ul>
<!-- /wp:gallery -->

So I assume the task here is to support this format in get_post_galleries() ?


I noticed in 43826.patch the following:

if ( false !== strpos( $post->post_content, '<!-- wp:gallery' ) ) {

We can instead use the has_block() function to check for the gallery block, e.g.:

if ( has_block( 'gallery', $post->post_content ) ) {

There's also the parse_blocks(), that might help to get the innerHTML of the core/gallery block names, instead of using:

 if ( preg_match_all( '/<!-- wp:gallery([^>]*)-->(.*?)<!-- \/wp:gallery -->/s', $post->post_content, $blocks, PREG_SET_ORDER ) ) {

Adding tests for the block format string cases (like above) sounds good.

Here are the current get_post_galleries() tests that would be helpful:

https://core.trac.wordpress.org/browser/tags/4.9.8/tests/phpunit/tests/media.php

#4 @pento
13 months ago

Thank you for the patch, @Takahashi_Fumiki!

There's actually already a block parser in Core, as @birgire mentioned.

Before continuing, however, it's worth noting that the Gallery Block format will be changed before 5.0 (GB11212) to store the image data in the block attibutes, which will make it available in the parse_blocks() output.

@tellyworth
13 months ago

#5 @tellyworth
13 months ago

attachment:43826.2.diff uses parse_blocks() to crudely parse gallery blocks and include the corresponding images in get_post_galleries() and related functions. Unit tests are included.

Currently it still relies on regexes to grab the <img src=".."> values from the gallery block's innerHtml. That could be improved when GB11212 lands, as @pento suggests.

Please note that I have only tested this with phpunit tests. I probably missed and/or broke something as well.

#6 @tellyworth
13 months ago

  • Keywords has-patch has-unit-tests needs-testing added; needs-patch needs-unit-tests removed

#7 @birgire
13 months ago

@tellyworth thanks for the updated patch and tests in 43826.2.diff

Few suggestions for the tests:

It would be nice to have a test case for a content containing both gallery shortcode and a gallery block.

I also wonder if we need more coverage for the $html = false input argument.

We could add @group blocks to the descriptions of the test methods:

  • test_block_inner_post_gallery_images()
  • test_block_post_gallery_images()
  • test_block_post_galleries().

It seems we can reduce some code by combining the two foreach loops with range 1 to 3 and 4 to 6 into a single foreach loop with range 1 to 6. (same for the older test cases).

But it seems benificial to move all the tests for get_post_galleries() under it's own test class, e.g. /tests/phpunit/tests/media/getPostGalleries.php.

Then we could share the fixtures to avoid duplication.

Last edited 13 months ago by birgire (previous) (diff)

@tellyworth
12 months ago

#8 @tellyworth
12 months ago

attachment:43826.3.diff improves the tests as per @birgire's suggestions, specifically:

  • @group blocks added to block tests
  • Added test_mixed_post_gallery_images() to cover mixed shortcode/block case
  • Shortened the tests using single ranges (also for some older tests)

Re $html = false, it seems like all of the tests cover these cases currently; it's true that is not covered. I'll look at adding those next.

Has anyone tested this manually?

@tellyworth
12 months ago

#9 @tellyworth
12 months ago

attachment:43826.4.diff makes further improvements:

  • It now supports json attributes in the gallery block, as per https://github.com/WordPress/gutenberg/pull/11973
  • Added test_block_post_gallery_images_json to cover the <!-- wp:gallery {"ids":1,2,3} --> case
  • Added test_mixed_post_galleries to better cover the $html=false parameter for both shortcodes and blocks (with json)

Couple of things to point out for further discussion:

  1. This continues to support gallery blocks that don't contain the json blob, by extracting img srcs from innerHtml. Should it?
  2. For blocks, get_post_galleries( ..., false ); will now include block attributes in the returned array, in much the same way that it does for shortcodes (see test_mixed_post_galleries for details). I've made sure the ids in the return array is a string, same as for shortcodes. However it's likely that other values in the json blob won't quite match what would be returned from a shortcode, so code that relies on get_post_galleries( ..., false ) might be surprised by the results. What's that going to break?

I've really only unit tested this, as I don't have a good understanding of real world use cases for these functions.

Last edited 12 months ago by tellyworth (previous) (diff)

#10 @antpb
12 months ago

  • Milestone changed from 5.0 to 5.0.1

We should move this to a future milestone as there are still some things to discuss and agree on. Changing the same function to produce different results may not be the best path forward.

One other thing to add, since we are moving this from 5.0, is that the classic-block has the ability to add a gallery with no ids defined. We may want some documentation around that for users that are expecting to use it. New galleries should be accessed like any other block on the server instead of forcing get_post_galleries() to match. I do think later we should support get_post_galleries() but we will need to do it at a later time.

#11 @birgire
12 months ago

To better understand the usage of "get_post_galleries" in plugins and themes:

There are currently 81 matches in 40 plugins in the plugin directory:

https://wpdirectory.net/search/01CWVNTY847CDMRPSX4J899RSY

and 170 matches in 91 themes in the theme directory:

https://wpdirectory.net/search/01CWVPA933JVWHT2HPZVD0S7ZC

#12 @tellyworth
12 months ago

Thanks for those references @birgire. I reviewed the twenty* themes and usage in all of the plugins with > 0 installs, and it looks promising. Most calls use ids only. A few use type or size if present.

I can see no indication there is a common usage idiom that would be tripped up by unexpected keys in the output. Just in case though, that's trivially dealt with by only including ids in the block return.

Last edited 12 months ago by tellyworth (previous) (diff)

@tellyworth
12 months ago

#13 @tellyworth
12 months ago

attachment:43826.5.diff iterates on the previous patch with two changes:

  • Only ids is included in the block attributes returned by get_post_galleries()
  • test_mixed_post_galleries_data tests the behaviour of attributes returned from both shortcode and block galleries

I think that should address the compatibility concern as described in comment:12.

#14 @pento
11 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#15 @pento
11 months ago

  • Milestone changed from 5.0.2 to 5.0.3

This ticket was mentioned in Slack in #core by pbiron. View the logs.


11 months ago

#17 @desrosj
11 months ago

  • Keywords needs-refresh added

43826.5.diff is currently not applying to trunk for me. Can someone refresh this, please? Would love to see this fixed in 5.0.3.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


11 months ago

@audrasjb
11 months ago

patch refreshed

#19 @audrasjb
11 months ago

  • Keywords needs-refresh removed

@desrosj I tried to refresh that patch

@desrosj
10 months ago

#20 @desrosj
10 months ago

43826.6.diff had some missing closing curly braces in the unit tests that was causing some fatal errors. 43826.diff should be a clean refresh against trunk. Working to review now.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


10 months ago

#22 @desrosj
10 months ago

  • Milestone changed from 5.0.3 to 5.1

Going to punt this. Still working through a review. If there is time before the 5.0.3 RC to finish, it can be moved back.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


10 months ago

#24 @pento
10 months ago

Let's leave it for 5.1. @desrosj: Can you finish reviewing it and commit when ready?

#25 @tellyworth
10 months ago

  • Owner set to tellyworth
  • Status changed from new to assigned

#26 @pento
10 months ago

  • Type changed from enhancement to task (blessed)

Converting this to a task. We can land it early during the beta period.

This ticket was mentioned in Slack in #core-media by aaroncampbell. View the logs.


10 months ago

#28 @desrosj
10 months ago

There were some questions when going through this one in Slack recently:

  • Does this have the potential of breaking anything that is using this function _not_ expecting Gutenberg galleries?
  • We would need to know if the content includes Gutenberg blocks, not if the site currently supports it, as those may be separate things.

@tellyworth do you need any help testing this? I didn't want to double efforts working on this one.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


10 months ago

#30 @celloexpressions
10 months ago

  • Version set to 5.0

get_post_gallery_images() and get_post_gallery() also call get_post_galleries(). Usage of these higher-level functions may be more common than 11 indicates. get_post_gallery_images() returns the image srcs, and this should continue working with new blocks so that themes don't have to rely on their own logic to locate classic and block galleries in content.

As long as the returned data format/contents matches classic galleries, themes and plugins would expect this to work equally for classic and block-based galleries. A very common usage is to show gallery excerpts in theme archive views; these are often created by gathering data with this function, then reformatting a custom gallery excerpt with custom markup. It is not tenable to use a different function for block galleries unless all existing content is migrated to gallery blocks, since this usage needs to apply to pre- and post-5.0 galleries equally.

43826.diff appears correct to me; however, I have not tested it.

#31 @pento
10 months ago

  • Milestone changed from 5.1 to 5.2

Still needs more testing, punting to 5.2.

#32 @dway
8 months ago

Hello, any update on this ?

This ticket was mentioned in Slack in #core-media by mike. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 months ago

#36 @JeffPaul
7 months ago

  • Milestone changed from 5.2 to Future Release

Per today's bugscrub, we're punting this as not enough time remains to get this into 5.2. Given the minimal movement during the 5.2 release cycle, I'm punting this to Future Release so that we consciously milestone this for a numbered release when we're ready to take it on.

#37 @russhylov
3 months ago

  • Focuses template added
  • Keywords needs-patch added; has-patch has-unit-tests needs-testing removed
  • Version 5.0 deleted

WP 5.2.2
All this functions ( get_post_gallery, get_post_galleries, get_post_galleries_images, get_attached_media) return empty array or false because post_parent is stays 0 when wp_post gets new record or the existing post is edited.

Version 0, edited 3 months ago by russhylov (next)

#38 @russhylov
3 months ago

Hi all.
Have temporarity solution.

When we insert new gallery into the post content, html view of gallery block look at this
<!-- wp:gallery {"ids":[909,917,916,915,914,913,912,911,910],"linkTo":"media"} -->
<ul class="wp-block-gallery columns-3 is-cropped">....
All of we need its IDs of attachment posts.

If we add to custom meta of post ids block (only numbers in []) with caption "gallery", then we can get custom metadata of the post and get IDs of attachments post which needed.
So, in our script we can get ids of attachments type post and show needed images

<?php

foreach ($posts as $post) {  
    $meta = get_post_meta($post->ID, 'gallery');
    $ids= explode(',', $meta[0]);
    foreach ($ids as $id) { 
      $atp_meta = get_post_meta($id);
      echo "<pre>";
        echo dirname($atp_meta['_wp_attached_file'][0])."<br>";  //  get the upload directory for needed images.  
        var_export(unserialize($atp_meta['_wp_attachment_metadata'][0])); // get array of all intermediate images.
      echo "</pre>";
    }
  }

Done. Profit :)

http://bendera.com.ua/screenshot.jpg

Last edited 3 months ago by russhylov (previous) (diff)
Note: See TracTickets for help on using tickets.