Make WordPress Core

Opened 5 years ago

Closed 13 months ago

Last modified 13 months ago

#43826 closed task (blessed) (fixed)

get_post_galleries() should return gallery blocks

Reported by: pento's profile pento Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.0
Component: Media Keywords: has-patch has-unit-tests commit needs-dev-note needs-docs
Focuses: template Cc:

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 4 years ago.
Add gutenberg block parser for get_post_galleries
43826.2.diff (5.5 KB) - added by tellyworth 4 years ago.
43826.3.diff (9.5 KB) - added by tellyworth 4 years ago.
43826.4.diff (13.1 KB) - added by tellyworth 4 years ago.
43826.5.diff (14.7 KB) - added by tellyworth 4 years ago.
43826.6.diff (15.3 KB) - added by audrasjb 4 years ago.
patch refreshed
43826.diff (16.3 KB) - added by desrosj 4 years ago.

Download all attachments as: .zip

Change History (65)

@Takahashi_Fumiki
4 years ago

Add gutenberg block parser for get_post_galleries

#2 @Takahashi_Fumiki
4 years 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
4 years 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
4 years 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
4 years ago

#5 @tellyworth
4 years 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
4 years ago

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

#7 @birgire
4 years 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 4 years ago by birgire (previous) (diff)

@tellyworth
4 years ago

#8 @tellyworth
4 years 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
4 years ago

#9 @tellyworth
4 years 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 4 years ago by tellyworth (previous) (diff)

#10 @antpb
4 years 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
4 years 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
4 years 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 4 years ago by tellyworth (previous) (diff)

@tellyworth
4 years ago

#13 @tellyworth
4 years 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
4 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#15 @pento
4 years ago

  • Milestone changed from 5.0.2 to 5.0.3

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


4 years ago

#17 @desrosj
4 years 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.


4 years ago

@audrasjb
4 years ago

patch refreshed

#19 @audrasjb
4 years ago

  • Keywords needs-refresh removed

@desrosj I tried to refresh that patch

@desrosj
4 years ago

#20 @desrosj
4 years 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.


4 years ago

#22 @desrosj
4 years 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.


4 years ago

#24 @pento
4 years ago

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

#25 @tellyworth
4 years ago

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

#26 @pento
4 years 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.


4 years ago

#28 @desrosj
4 years 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.


4 years ago

#30 @celloexpressions
4 years 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
4 years ago

  • Milestone changed from 5.1 to 5.2

Still needs more testing, punting to 5.2.

#32 @dway
4 years ago

Hello, any update on this ?

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


4 years ago

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


4 years ago

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


4 years ago

#36 @JeffPaul
4 years 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 years 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 cell is stays 0 when wp_post gets new record or the existing post is edited.

Last edited 3 years ago by russhylov (previous) (diff)

#38 @russhylov
3 years 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 years ago by russhylov (previous) (diff)

#39 @lynk
2 years ago

Will this be fixed add some point? There should be a note in the codex that these functions are currently broken.

Last edited 2 years ago by lynk (previous) (diff)

#40 @celloexpressions
2 years ago

  • Keywords needs-testing added
  • Version set to 5.0

This needs to be fixed. The onus of backwards compatibility should be on core, not themes. And many themes have not taken temporary steps to address this issue. This pushes the work to users, who must rely on the classic editor or insert galleries in classic blocks within the block editor. With dozens of uses in dozens of themes and plugins, there are at minimum hundreds or thousands of people impacted by this breakage.

This ticket was mentioned in PR #1783 on WordPress/wordpress-develop by glendaviesnz.


13 months ago
#41

  • Keywords has-patch added; needs-patch removed

### Description
Currently the get_post_galleries method will only handle gallery shortcodes. A trac ticket has been open for some time to rectify this, and in the meantime a new version of the gallery block is about to be released which uses innerblocks, so the diffs on this ticket will no longer be sufficient.

This PR adds support for both versions of the gallery block

### To test:

  • Check out this PR to local wp dev env along with the latest version of the gutenberg plugin
  • Add a theme that makes use of the get_post_galleries method, eg. Arbutus
  • Add several posts using the Gallery post type - add a short code, gallery block, and v2 gallery block (turn on gallery experiment to add a v2 block)
  • The the homepage of the site and check that all 3 galleries display as expected

https://i0.wp.com/user-images.githubusercontent.com/3629020/138993311-71de119d-cba5-42c1-b165-d03c7fadbc70.png

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

#42 @glendaviesnz
13 months ago

  • Keywords needs-patch added; has-patch removed

Since this was last looked at we now have a new version of the Gallery block that is about to be released that uses innerBlocks. I have created a PR (https://github.com/WordPress/wordpress-develop/pull/1783) that refactors the get_post_galleries method to account for both these formats, and also splits some of the parsing out to separated methods as the nesting was getting a bit deep/complex to account for a second block format.

glendaviesnz commented on PR #1783:


13 months ago
#43

Once there is agreement on the way forward with this I will add some additional unit tests

This ticket was mentioned in PR #1788 on WordPress/wordpress-develop by glendaviesnz.


13 months ago
#44

  • Keywords has-patch added; needs-patch removed

This is an alternative to #1783 - it makes for a much longer and harder to grok get_post_galleries method, but avoids adding extra methods to global namespace.

### Description
Currently the get_post_galleries method will only handle gallery shortcodes. A trac ticket has been open for some time to rectify this, and in the meantime a new version of the gallery block is about to be released which uses innerblocks, so the diffs on this ticket will no longer be sufficient.

This PR adds support for both versions of the gallery block

### To test:

  • Check out this PR to local wp dev env along with the latest version of the gutenberg plugin
  • Add a theme that makes use of the get_post_galleries method, eg. Arbutus
  • Add several posts using the Gallery post type - add a short code, gallery block, and v2 gallery block (turn on gallery experiment to add a v2 block)
  • The the homepage of the site and check that all 3 galleries display as expected

https://i0.wp.com/user-images.githubusercontent.com/3629020/138993311-71de119d-cba5-42c1-b165-d03c7fadbc70.png

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

ramonjd commented on PR #1788:


13 months ago
#46

This is testing well for me.

I tried with Classic Block/short code, v1, v2 galleries

Before
https://i0.wp.com/user-images.githubusercontent.com/6458278/139416506-a7dfc4c1-9836-47bd-927f-4d993f96527a.png

After
https://i0.wp.com/user-images.githubusercontent.com/6458278/139416528-a30c824e-e54e-4905-a48b-04d281438de5.png

#47 @hellofromTonya
13 months ago

  • Milestone changed from Future Release to 5.9
  • Owner changed from tellyworth to hellofromTonya
  • Status changed from assigned to reviewing

In talking with @glendaviesnz, PR 1788 is needed:

to update an existing method to cope with the new gallery block format that will be merged from gutenberg into core

the method has never worked with gallery blocks anyway, but this fix will make it work for both old and new gutenberg gallery block formats

See Gutenberg issue 35907 for context and more details.

Moving this ticket into 5.9 and self-assigning for test and review.

glendaviesnz commented on PR #1788:


13 months ago
#48

Have added initial unit tests - still need to add some tests for the new gallery format

This ticket was mentioned in PR #1864 on WordPress/wordpress-develop by costdev.


13 months ago
#49

  • Keywords has-unit-tests added

Galleries in Gutenberg don't use the [gallery] shortcode, so aren't returned by get_post_galleries().

This change makes get_post_galleries() also check for Gallery blocks and return them.

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

#50 @glendaviesnz
13 months ago

To test https://github.com/WordPress/wordpress-develop/pull/1788:

Apply the patch to a 5.9 build in local wp dev env

  • 5.9 includes the version of Gutenberg needed (11.9) so no need for plugin
  • Add a gallery post (post type can be set in editor right settings panel) with v1 version of the gallery blocks - to do this set the use_balanceTags option to 1, eg. wp option set use_balanceTags 1. With this set add a new post of type Gallery, add a gallery block to it and check that it is v1 block - you can tell by clicking on an image in the gallery and seeing that it is the move icons within the image
  • Once you have a gallery post with a v1 gallery block set wp option set use_balanceTags 0 and add a new gallery post with a v2 gallery block - you can tell it is v2 by selecting an individual image and it will have its own block tool bar with image editing, duotone, etc
  • Add a third gallery post with a gallery short code. You can do this by adding a Classic block, and using the media option in the classic block toolbar to add a gallery
  • Visit the site homepage and all three types of gallery should display a gallery excerpt as below:

https://user-images.githubusercontent.com/3629020/138993311-71de119d-cba5-42c1-b165-d03c7fadbc70.png

Without the patch applied only the classic and v1 block will display the gallery excerpt in the home page. The v1 block only displays because Arbutus has a fallback that parses the v1 blocks when get_post_galleries fails to return anything.

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

glendaviesnz commented on PR #1864:


13 months ago
#51

This tested well for me on a site running the Arbutus theme which uses get_post_galleries to generate gallery excerpts. This updated call worked for a v1 and v2 gallery block, a gallery blocked nested 2 deep in group blocks, and also for a short code gallery:
https://i0.wp.com/user-images.githubusercontent.com/3629020/141964472-08e662a0-108b-44c1-aa98-e150b164ce81.png

LGTM once the failing tests are sorted.

hellofromtonya commented on PR #1788:


13 months ago
#52

Closing in preference to PR #1864.

#53 @hellofromTonya
13 months ago

  • Keywords commit added; needs-testing removed

PR 1864 is ready for commit. It handles both v1 and v2 gallery block. Performance has been considered, analyzed, and tuned.

There are holdups with getting the migration from v1 to v2 gallery block content working (see GB PR 36191. Per @glendaviesnz,

This hold up has been brought about by the need to block access to sites that have use_balanceTags enabled, but are running WordPress versions < 5.9, and complications with trying to set a flag to alert for the presence of the use_BalanceTags at a point in desktop and mobile before gallery deprecations run.

The migration testing work should not affect the v2 structure which is handled in this ticket and the patch.

If by RC the v2 gallery block is reverted (removed from core), then this ticket's patch can be revisited to determine if the v2 portion should also be reverted.

As today is 5.9 Beta 1, marking this commit.

#54 @dway
13 months ago

Nice work, thx !

#55 @hellofromTonya
13 months ago

Preparing the commit.

#56 @hellofromTonya
13 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 52190:

Media: Add support for v1 and v2 gallery block in get_post_galleries().

The get_post_galleries() function only handled galleries from the [gallery] shortcode. It did not process gallery blocks.

Introducing v1 and v2 gallery block support in get_post_galleries() including support for innerblock nesting.

There are no changes to how the function is called. It detects if the post content has one or more gallery blocks. If detected, it parses the blocks and then processes to add each gallery block's HTML to the array of galleries before being passed through the filter and returned.

Includes integration tests.

Follow-up to [24682], [43309], [48262], [52042].

Props glendaviesnz, costdev, antpb, audrasjb, birgire, celloexpressions, desrosj, hellofromTonya, jeffpaul, lynk, pento, ramonopoly, russhylov, takahashi_fumiki, tellyworth.
Fixes #43826.

#58 @hellofromTonya
13 months ago

  • Keywords needs-dev-note needs-docs added

Gallery block support (for v1 and v2) has arrived in get_post_galleries(). Thank you everyone for your contributions! This will ship with 5.9. Cheers

Marking for dev-note and docs update.

Note: See TracTickets for help on using tickets.