Skip to content
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

Jump to bottom

Use metadata.name only as the hint for pattern overrides #59956

Closed
wants to merge 3 commits into from
Closed

Use metadata.name only as the hint for pattern overrides #59956

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f17f82d
Use name only as the hint for pattern overrides
kevin940726 Mar 18, 2024
e9858fe
Add check for metadata.name
kevin940726 Mar 20, 2024
7fba5e9
Add bindings attribute
kevin940726 Mar 25, 2024
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
51 changes: 51 additions & 0 deletions lib/compat/wordpress-6.6/blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php
/**
* Temporary compatibility shims for block APIs present in Gutenberg.
*
* @package gutenberg
*/

/**
* Process the block bindings attribute for pattern overrides.
*
* @param string $block_content Block Content.
* @param array $parsed_block The full block, including name and attributes.
* @param WP_Block $block_instance The block instance.
*/
function gutenberg_process_pattern_overrides_bindings( $block_content, $parsed_block, $block_instance ) {
$pattern_overrides_source = get_block_bindings_source( 'core/pattern-overrides' );

// Return early if the pattern overrides source is not registered.
if ( null === $pattern_overrides_source ) {
return $block_content;
}

$supported_block_attrs = array(
'core/paragraph' => array( 'content' ),
'core/heading' => array( 'content' ),
'core/image' => array( 'id', 'url', 'title', 'alt' ),
'core/button' => array( 'url', 'text', 'linkTarget', 'rel' ),
);

// If the block isn't one of the supported block types or isn't inside a pattern, return.
if (
! isset( $supported_block_attrs[ $block_instance->name ] ) ||
! isset( $block_instance->context['pattern/overrides'] )
) {
return $block_content;
}
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved

$modified_block_content = $block_content;
foreach ( $supported_block_attrs[ $block_instance->name ] as $attribute_name ) {
$source_value = $pattern_overrides_source->get_value( array(), $block_instance, $attribute_name );

// If the value is not null, process the HTML based on the block and the attribute.
if ( ! is_null( $source_value ) ) {
$modified_block_content = gutenberg_block_bindings_replace_html( $modified_block_content, $block_instance->name, $attribute_name, $source_value );
}
}

return $modified_block_content;
}

add_filter( 'render_block', 'gutenberg_process_pattern_overrides_bindings', 20, 3 );
Copy link
Contributor

@youknowriad youknowriad Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This hook feels like it recreates part of the block binding framework. Maybe another way to achieve the same is to actually "inject" the block bindings attributes in the render callback of the core/block block before calling do_blocks. That way the logic of the block bindings continue to apply just today, we'll just be "not persisting" the binding.

Copy link
Member Author

Choose a reason for hiding this comment

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

This hook is actually mutually exclusive to the original gutenberg_process_block_bindings hook because that hook only checks when there's a metadata.bindings attribute. I think we can merge them together in Core if it makes sense too.

Copy link
Contributor

@talldan talldan Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm intrigued by @youknowriad's suggestion of generating the binding attributes at runtime though, as it avoids changing the block bindings code at all. That makes sense to me given block bindings is the broader API, while pattern overrides are a single use case of that API.

I guess a downside is it might be more fragile, as it relies on a specific order of operations, but then there are tests for that.

Could be something to explore.

Copy link
Member

Choose a reason for hiding this comment

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

There are no hooks used in WP core for block bindings integration, so I don’t think there would be any issue with updating the attributes in a proper place in WP_Block class.

Copy link
Member Author

Choose a reason for hiding this comment

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

In 7fba5e9, I updated the code to inject the bindings instead. Not sure if it's what you mean here?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep that matches what I had in mind basically.

1 change: 1 addition & 0 deletions lib/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ function gutenberg_is_experiment_enabled( $name ) {
require __DIR__ . '/compat/wordpress-6.5/script-loader.php';

// WordPress 6.6 compat.
require __DIR__ . '/compat/wordpress-6.6/blocks.php';
require __DIR__ . '/compat/wordpress-6.6/block-bindings/pattern-overrides.php';

// Experimental features.
Expand Down
12 changes: 2 additions & 10 deletions packages/block-library/src/block/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,7 @@ function hasOverridableAttributes( block ) {
return (
Object.keys( PARTIAL_SYNCING_SUPPORTED_BLOCKS ).includes(
block.name
) &&
!! block.attributes.metadata?.bindings &&
Object.values( block.attributes.metadata.bindings ).some(
( binding ) => binding.source === 'core/pattern-overrides'
)
) && !! block.attributes.metadata?.name
);
}

Expand All @@ -110,11 +106,7 @@ function hasOverridableBlocks( blocks ) {
}

function getOverridableAttributes( block ) {
return Object.entries( block.attributes.metadata.bindings )
.filter(
( [ , binding ] ) => binding.source === 'core/pattern-overrides'
)
.map( ( [ attributeKey ] ) => attributeKey );
return PARTIAL_SYNCING_SUPPORTED_BLOCKS[ block.name ];
}

function applyInitialContentValuesToInnerBlocks(
Expand Down
21 changes: 1 addition & 20 deletions packages/editor/src/hooks/pattern-overrides.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { store as editorStore } from '../store';
import { unlock } from '../lock-unlock';

const {
useSetPatternBindings,
ResetOverridesControl,
PATTERN_TYPES,
PARTIAL_SYNCING_SUPPORTED_BLOCKS,
Expand All @@ -38,7 +37,6 @@ const withPatternOverrideControls = createHigherOrderComponent(
return (
<>
<BlockEdit { ...props } />
{ isSupportedBlock && <BindingUpdater { ...props } /> }
{ props.isSelected && isSupportedBlock && (
<ControlsWithStoreSubscription { ...props } />
) }
Expand All @@ -47,15 +45,6 @@ const withPatternOverrideControls = createHigherOrderComponent(
}
);

function BindingUpdater( props ) {
const postType = useSelect(
( select ) => select( editorStore ).getCurrentPostType(),
[]
);
useSetPatternBindings( props, postType );
return null;
}

// Split into a separate component to avoid a store subscription
// on every block.
function ControlsWithStoreSubscription( props ) {
Expand All @@ -66,18 +55,10 @@ function ControlsWithStoreSubscription( props ) {
[]
);

const bindings = props.attributes.metadata?.bindings;
const hasPatternBindings =
!! bindings &&
Object.values( bindings ).some(
( binding ) => binding.source === 'core/pattern-overrides'
);

const shouldShowResetOverridesControl =
! isEditingPattern &&
!! props.attributes.metadata?.name &&
blockEditingMode !== 'disabled' &&
hasPatternBindings;
blockEditingMode !== 'disabled';

return (
<>
Expand Down
120 changes: 0 additions & 120 deletions packages/patterns/src/components/use-set-pattern-bindings.js
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions packages/patterns/src/private-apis.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
import RenamePatternModal from './components/rename-pattern-modal';
import PatternsMenuItems from './components';
import RenamePatternCategoryModal from './components/rename-pattern-category-modal';
import useSetPatternBindings from './components/use-set-pattern-bindings';
import ResetOverridesControl from './components/reset-overrides-control';
import { useAddPatternCategory } from './private-hooks';
import {
Expand All @@ -34,7 +33,6 @@ lock( privateApis, {
RenamePatternModal,
PatternsMenuItems,
RenamePatternCategoryModal,
useSetPatternBindings,
ResetOverridesControl,
useAddPatternCategory,
PATTERN_TYPES,
Expand Down
25 changes: 9 additions & 16 deletions test/e2e/specs/editor/various/pattern-overrides.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,7 @@ test.describe( 'Pattern Overrides', () => {
name: 'core/paragraph',
attributes: {
content: 'This paragraph can be edited',
metadata: {
name: editableParagraphName,
bindings: {
content: {
source: 'core/pattern-overrides',
},
},
},
metadata: { name: editableParagraphName },
},
},
{
Expand Down Expand Up @@ -225,7 +218,7 @@ test.describe( 'Pattern Overrides', () => {
const paragraphId = 'paragraph-id';
const { id } = await requestUtils.createBlock( {
title: 'Pattern',
content: `<!-- wp:paragraph {"metadata":{"id":"${ paragraphId }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
content: `<!-- wp:paragraph {"metadata":{"name":"${ paragraphId }"}} -->
<p>Editable</p>
<!-- /wp:paragraph -->`,
status: 'publish',
Expand Down Expand Up @@ -257,7 +250,7 @@ test.describe( 'Pattern Overrides', () => {
name: 'core/paragraph',
attributes: {
content: 'edited Editable',
metadata: undefined,
metadata: { name: paragraphId },
},
},
] );
Expand All @@ -274,7 +267,7 @@ test.describe( 'Pattern Overrides', () => {
const { id } = await requestUtils.createBlock( {
title: 'Button with target',
content: `<!-- wp:buttons -->
<div class="wp-block-buttons"><!-- wp:button {"metadata":{"name":"${ buttonName }","bindings":{"text":{"source":"core/pattern-overrides"},"url":{"source":"core/pattern-overrides"},"linkTarget":{"source":"core/pattern-overrides"},"rel":{"source":"core/pattern-overrides"}}}} -->
<div class="wp-block-buttons"><!-- wp:button {"metadata":{"name":"${ buttonName }"}} -->
<div class="wp-block-button"><a class="wp-block-button__link wp-element-button" href="http://wp.org" target="_blank" rel="noreferrer noopener nofollow">Button</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons -->`,
Expand Down Expand Up @@ -384,14 +377,14 @@ test.describe( 'Pattern Overrides', () => {
const headingName = 'Editable heading';
const innerPattern = await requestUtils.createBlock( {
title: 'Inner Pattern',
content: `<!-- wp:paragraph {"metadata":{"name":"${ paragraphName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
content: `<!-- wp:paragraph {"metadata":{"name":"${ paragraphName }"}} -->
<p>Inner paragraph</p>
<!-- /wp:paragraph -->`,
status: 'publish',
} );
const outerPattern = await requestUtils.createBlock( {
title: 'Outer Pattern',
content: `<!-- wp:heading {"metadata":{"name":"${ headingName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
content: `<!-- wp:heading {"metadata":{"name":"${ headingName }"}} -->
<h2 class="wp-block-heading">Outer heading</h2>
<!-- /wp:heading -->
<!-- wp:block {"ref":${ innerPattern.id },"content":{"${ paragraphName }":{"content":"Inner paragraph (edited)"}}} /-->`,
Expand Down Expand Up @@ -501,10 +494,10 @@ test.describe( 'Pattern Overrides', () => {
const paragraphName = 'Editable paragraph';
const { id } = await requestUtils.createBlock( {
title: 'Pattern',
content: `<!-- wp:heading {"metadata":{"name":"${ headingName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
content: `<!-- wp:heading {"metadata":{"name":"${ headingName }"}} -->
<h2 class="wp-block-heading">Heading</h2>
<!-- /wp:heading -->
<!-- wp:paragraph {"metadata":{"name":"${ paragraphName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
<!-- wp:paragraph {"metadata":{"name":"${ paragraphName }"}} -->
<p>Paragraph</p>
<!-- /wp:paragraph -->`,
status: 'publish',
Expand Down Expand Up @@ -596,7 +589,7 @@ test.describe( 'Pattern Overrides', () => {
);
const { id } = await requestUtils.createBlock( {
title: 'Pattern',
content: `<!-- wp:image {"metadata":{"name":"${ imageName }","bindings":{"id":{"source":"core/pattern-overrides"},"url":{"source":"core/pattern-overrides"},"title":{"source":"core/pattern-overrides"},"alt":{"source":"core/pattern-overrides"}}}} -->
content: `<!-- wp:image {"metadata":{"name":"${ imageName }"}} -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->`,
status: 'publish',
Expand Down
Loading