Gallery Block: Support gaps that define column/row gaps to avoid PHP Warning/Fatal #41125
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dd32
added
[Type] Bug
|
Pinging a few folks you worked on the block gap feature recently. |
There was a problem hiding this comment.
Thanks for the PR @dd32!
The Gallery block hasn't opted-in to the split row / column support for blockGap, so I'm not quite sure how the users managed to create their patterns (though they could have manually edited the markup to try to achieve the result). Still, it'll be very good to get this fix in, much appreciated!
It's looking mostly good to me, just left a comment where I think we need to use the column value instead of the combined value.
We probably also need to update the JS-equivalent place where this logic is used:
We can always look at that separately if you need to get this PR in quickly. I'm just wrapping up for the day now, but happy to take a closer look tomorrow.
For testing, here's the block markup I'm using:
Test gallery block markup
<!-- wp:gallery {"linkTo":"none","className":"columns-2","style":{"spacing":{"blockGap":{"top":"10px","left":"2px"}}}} -->
<figure class="wp-block-gallery has-nested-images columns-default is-cropped columns-2"><!-- wp:image {"id":27,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://a-toule-site.local/wp-content/uploads/2022/02/IMG_3878-768x1024.jpg" alt="" class="wp-image-27"/></figure>
<!-- /wp:image -->
<!-- wp:image {"id":26,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://a-toule-site.local/wp-content/uploads/2022/02/IMG_3880-768x1024.jpg" alt="" class="wp-image-26"/></figure>
<!-- /wp:image -->
<!-- wp:image {"id":25,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://a-toule-site.local/wp-content/uploads/2022/02/IMG_3875-1024x1024.jpg" alt="" class="wp-image-25"/></figure>
<!-- /wp:image --></figure>
<!-- /wp:gallery -->To manually change split column / row gap in the Gallery block, for anyone testing, you can update this line of the Gallery block's block.json file to the following:
"blockGap": [ "horizontal", "vertical" ],
And CC: @glendaviesnz for a second opinion 😀
Comment on lines
70
to
76
| if ( is_array( $gap_value ) ) { | ||
| $gap_row = isset( $gap_value['top'] ) ? $gap_value['top'] : '0.5em'; | ||
| $gap_column = isset( $gap_value['left'] ) ? $gap_value['left'] : '0.5em'; | ||
| $gap_value = $gap_row === $gap_column ? $gap_row : $gap_row . ' ' . $gap_column; | ||
| } | ||
|
|
||
| $style = '.' . $class . '{ --wp--style--unstable-gallery-gap: ' . $gap_value . '; gap: ' . $gap_value . '}'; |
There was a problem hiding this comment.
I think it looks like --wp--style--unstable-gallery-gap is used for calculating the width of columns, so this value always needs to be a single column value, not the combined value if I'm reading this correctly. (@glendaviesnz might be able to correct me on this).
I think that might mean that on line 73, we should set that to always be the column value. E.g.
$gap_value = $gap_column;
There was a problem hiding this comment.
I think it looks like --wp--style--unstable-gallery-gap is used for calculating the width of columns
Yep, this definitely should always be the single column value or the calculation of the column widths will fail.
|
Thanks for finding this @dd32
Just to add context, it is indeed an internal plugin change where we allowed split values for The value comes from the block gap editor control, but should only be an array where the block opts into split values as @andrewserong says. So it's not expected. I'll poke around to make sure this is the case. |
ramonjd
reviewed
May 18, 2022
Good call @andrewserong This works for me: --- a/packages/block-library/src/gallery/gap-styles.js
+++ b/packages/block-library/src/gallery/gap-styles.js
@@ -8,9 +8,12 @@ export default function GapStyles( { blockGap, clientId } ) {
const styleElement = useContext( BlockList.__unstableElementContext );
// --gallery-block--gutter-size is deprecated. --wp--style--gallery-gap-default should be used by themes that want to set a default
// gap on the gallery.
- const gapValue = blockGap
- ? blockGap
- : `var( --wp--style--gallery-gap-default, var( --gallery-block--gutter-size, var( --wp--style--block-gap, 0.5em ) ) )`;
+ let gapValue = `var( --wp--style--gallery-gap-default, var( --gallery-block--gutter-size, var( --wp--style--block-gap, 0.5em ) ) )`;
+ // Check for the possibility of split block gap values. See: https://github.com/WordPress/gutenberg/pull/37736
+ if ( !! blockGap ) {
+ gapValue = typeof blockGap === 'string' ? blockGap : blockGap?.left;
+ }
+
|
ramonjd
reviewed
May 19, 2022
Co-authored-by: Ramon <[email protected]>
Ah interesting! Users can copy-paste patterns in from elsewhere, so if they created it outside of the editor that might explain it. Avoiding errors just because of invalid block settings is still required obviously though :) I tracked down a pattern that triggers it: https://wordpress.org/patterns/pattern/fullwidth-gallery-section-with-half-framed-title/ - here's the source: https://gist.github.com/dd32/5a2db374548d50f5ec8574553c6169a2 I've looked through the revisions, and it looks like it might have been from a copy-paste, however, an older revision does have a |
…pport gap value is not a string
|
Updated so that the 'left' (representing gap column) axial value for gap will be used (if any)
|
|
Size Change: -3.06 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
Thanks for progressing the PR @ramonjd (and for getting it all started, Dion!) Hope you don't mind, Ramon, I've pushed an update in 46a7e40 — I noticed that the split vertical / horizontal values weren't being output (it was using the column value for the
Here's the markup I'm using: Gallery block markup<!-- wp:gallery {"columns":2,"linkTo":"none","className":"columns-2","style":{"spacing":{"blockGap":{"top":"20px","left":"2px"}}}} -->
<figure class="wp-block-gallery has-nested-images columns-2 is-cropped"><!-- wp:image {"id":27,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://a-toule-site.local/wp-content/uploads/2022/02/IMG_3878-768x1024.jpg" alt="" class="wp-image-27"/></figure>
<!-- /wp:image -->
<!-- wp:image {"id":26,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://a-toule-site.local/wp-content/uploads/2022/02/IMG_3880-768x1024.jpg" alt="" class="wp-image-26"/></figure>
<!-- /wp:image -->
<!-- wp:image {"id":25,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://a-toule-site.local/wp-content/uploads/2022/02/IMG_3875-1024x1024.jpg" alt="" class="wp-image-25"/></figure>
<!-- /wp:image --></figure>
<!-- /wp:gallery -->Feel free to revert / make any further changes if I missed anything! 🙂 |
Mind? I love it! Thank you 🙇 I went at it like a bit of a bulldozer and it didn't actually occur to me that the gallery block might support Good call! |
ramonjd
approved these changes
May 19, 2022
glendaviesnz
approved these changes
May 19, 2022
There was a problem hiding this comment.
This worked as advertised for me.
I have added an issue to add the ability to set both in the UI now that it is supported in the backend.
andrewserong
approved these changes
May 19, 2022
andrewserong
added
the
Backport to WP 6.7 Beta/RC
|
I've added the Backport label — if possible, I think it'd be good to get this fix into 6.0.1 potentially, just in case. |
|
I've added a follow-up PR to opt-in to the controls in the UI in: #41175 |
adamziel
pushed a commit
that referenced
this pull request
May 20, 2022
…Warning/Fatal (#41125) * Gallery Block: Support gaps that define column/row gaps. * Fix a variable typo Co-authored-by: Ramon <[email protected]> * Checking for and using a 'left', or gap column, value if the block support gap value is not a string * Use column gap for CSS variable and combined gap for gap property * Remove unnecessary fallback * Tidy whitespace slightly Co-authored-by: Ramon <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: Andrew Serong <[email protected]>
|
I just cherry-picked this PR to the wp/6.0 branch to get it included in the next release: 71a1b4a |
adamziel
removed
the
Backport to WP 6.7 Beta/RC
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR adds array-notation support to the Gallery Block, as currently that causes a PHP Warning under PHP <8, or a Fatal error in PHP 8.1.
Similar to #39927.
Why?
To resolve a PHP Warning/Fatal error.
This PR may be invalid, if an array for
blockGapis not expected here, I'm not sure though of the expectations here. I'm not sure if this is being caused by something specific on WordPress.org as I'm unable to locate where the value is coming from.Hopefully someone more familiar with the Block Gap implementation can shed some light here?
cc @ryelle since this could actually be Pattern Directory specific, I'm really not sure.
How?
The logic has been copied from the Layout support, where this style of blockGap is handled
gutenberg/lib/block-supports/layout.php
Lines 176 to 185 in 1eee465
gutenberg/lib/block-supports/layout.php
Lines 104 to 108 in 1eee465
Testing Instructions
I'm unsure how to trigger this, a number of Patterns in the WordPress.org Pattern Directory trigger it, but I'm unsure which ones they are as it's occurring during a generic API request.
Screenshots or screencast