Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
Add __default binding for pattern overrides
  • Loading branch information
kevin940726 authored and talldan committed May 31, 2024
commit 9d450540489a809ee4d40037ace9b6305b3ed8e3
33 changes: 25 additions & 8 deletions packages/block-editor/src/hooks/use-bindings-attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ const BLOCK_BINDINGS_ALLOWED_BLOCKS = {
'core/button': [ 'url', 'text', 'linkTarget', 'rel' ],
};

const DEFAULT_ATTRIBUTES = '__default';

/**
* Based on the given block name,
* check if it is possible to bind the block.
Expand Down Expand Up @@ -68,11 +70,20 @@ export const withBlockBindingSupport = createHigherOrderComponent(
return;
}

const bindingsEntries = Object.entries( bindings );
const attributes = {};

for ( const [ attributeName, boundAttribute ] of Object.entries(
bindings
) ) {
if ( bindings[ DEFAULT_ATTRIBUTES ] !== undefined ) {
Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand, the #60694 (comment) from @SantosGuillamot is about the changes applied in the general processing of block bindings in the block editor. At the moment, the implementation proposed makes __default a keyword that would work with every possible type of block binding source on the client, but there is no matching logic on the server that would make it universally applicable. What alternatives would be possible to limit it to Pattern Overrides only?

Copy link
Contributor

Choose a reason for hiding this comment

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

With the getValue implementation, I don't think there's any way to move this logic into pattern overrides source itself. At the moment this file checks each attribute individually so it expects the metadata.bindings attribute to use the same attribute name as the block attribute.

If it were getValues instead, I think it might be possible, as perhaps the code would encounter __default in the metadata.bindings, know that there's an active binding of some kind, and defer completely to the getValues implementation to return the matching values for each attribute. Within getValues the code could expand the __default binding to the complete supported attribute list

But probably the easiest implementation is to add some hard-coding in useBindingAttributes so that __default only works when it's set to core/pattern-overrides.

I realise that there's also ongoing efforts to refactor this into the store right now so the implementation might change.

Copy link
Member

Choose a reason for hiding this comment

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

But probably the easiest implementation is to add some hard-coding in useBindingAttributes so that __default only works when it's set to core/pattern-overrides.

Yes, that works for now 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

I've made those changes. Also tried to make the changes to useBindingAttributes a lot more minimal so its easier to port to a different implementation, now the __default binding is handled by one function, and the rest of the code is the same.

const defaultAttributes = BLOCK_BINDINGS_ALLOWED_BLOCKS[ name ];
bindingsEntries.push(
...defaultAttributes.map( ( attributeName ) => [
attributeName,
bindings[ DEFAULT_ATTRIBUTES ],
] )
);
}

for ( const [ attributeName, boundAttribute ] of bindingsEntries ) {
const source = sources[ boundAttribute.source ];
if (
! source?.getValue ||
Expand Down Expand Up @@ -122,14 +133,17 @@ export const withBlockBindingSupport = createHigherOrderComponent(
keptAttributes
) ) {
if (
! bindings[ attributeName ] ||
! canBindAttribute( name, attributeName )
( ! bindings[ attributeName ] ||
! canBindAttribute( name, attributeName ) ) &&
! bindings[ DEFAULT_ATTRIBUTES ]
) {
continue;
}

const source =
sources[ bindings[ attributeName ].source ];
const binding =
bindings[ attributeName ] ??
bindings[ DEFAULT_ATTRIBUTES ];
const source = sources[ binding?.source ];
if ( ! source?.setValue && ! source?.setValues ) {
continue;
}
Expand Down Expand Up @@ -157,12 +171,15 @@ export const withBlockBindingSupport = createHigherOrderComponent(
attributeName,
value,
] of Object.entries( attributes ) ) {
const binding =
bindings[ attributeName ] ??
bindings[ DEFAULT_ATTRIBUTES ];
source.setValue( {
registry,
context,
clientId,
attributeName,
args: bindings[ attributeName ].args,
args: binding.args,
value,
} );
}
Expand Down
3 changes: 2 additions & 1 deletion packages/block-library/src/block/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ import { name as patternBlockName } from './index';
import { unlock } from '../lock-unlock';

const { useLayoutClasses } = unlock( blockEditorPrivateApis );
const { isOverridableBlock } = unlock( patternsPrivateApis );
const { isOverridableBlock, PARTIAL_SYNCING_SUPPORTED_BLOCKS } =
unlock( patternsPrivateApis );

const fullAlignments = [ 'full', 'wide', 'left', 'right' ];

Expand Down
37 changes: 36 additions & 1 deletion packages/block-library/src/block/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,48 @@ function render_block_core_block( $attributes ) {
* filter so that it is available when a pattern's inner blocks are
* rendering via do_blocks given it only receives the inner content.
*/
$has_pattern_overrides = isset( $attributes['content'] );
$has_pattern_overrides = isset( $attributes['content'] ) && null !== get_block_bindings_source( 'core/pattern-overrides' );
if ( $has_pattern_overrides ) {
$filter_block_context = static function ( $context ) use ( $attributes ) {
$context['pattern/overrides'] = $attributes['content'];
return $context;
};
add_filter( 'render_block_context', $filter_block_context, 1 );

// Add bindings for the default pattern overrides source.
$apply_default_pattern_overrides_bindings = static function ( $parsed_block ) {
$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' ),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that right now things are in flux and it's more convenient to hardcode or keep certain code paths private, but what's the plan for these mappings in the future?

  • An attribute property in the schema? { "attributes:" { "content": { "...": "...", "overridable": true } } }
  • A block-supports record? { "supports": { "overrides": [ "content" ] } }?
  • Something entirely different?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we've briefly discussed this before but decided to leave this for the future when we have a clear understanding of what the Block Binding API would look like. I believe the general consensus for the first version of pattern overrides is to use a hard-coded list.


if (
// Return early if the block isn't one of the supported block types,
! isset( $supported_block_attrs[ $parsed_block['blockName'] ] ) ||
// or doesn't have a name,
! isset( $parsed_block['attrs']['metadata']['name'] ) ||
// or doesn't have the default binding.
empty( $parsed_block['attrs']['metadata']['bindings']['__default'] ) ||
// or the default binding isn't the pattern overrides source.
'core/pattern-overrides' !== $parsed_block['attrs']['metadata']['bindings']['__default']['source']
) {
return $parsed_block;
}

$bindings = array();
foreach ( $supported_block_attrs[ $parsed_block['blockName'] ] as $attribute_name ) {
$bindings[ $attribute_name ] = array( 'source' => 'core/pattern-overrides' );
}
$parsed_block['attrs']['metadata']['bindings'] = array_merge(
$bindings,
$parsed_block['attrs']['metadata']['bindings']
);

return $parsed_block;
};
add_filter( 'render_block_data', $apply_default_pattern_overrides_bindings, 10, 1 );
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know about the implications of using a filter instead of changing the bindings process directly, as we are doing in the editor.

@gziolo Any idea if this could cause any issues?

Copy link
Member

Choose a reason for hiding this comment

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

Updating the linked private method would be the preferred approach when integrating in WordPress Core. However that might be really difficult to achieve in the near perspective if we want to test it in the Gutenberg plugin.

By the way, the filter should get removed after executing do_blocks, otherwise, if there are several Synced Patterns on the page, the same filter will get added whenever a block renders. In effect 10 blocks rendered, the same filter registered and executed 10 times.

Copy link
Contributor

@talldan talldan May 31, 2024

Choose a reason for hiding this comment

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

Agree, I'd prefer it if there were some symmetry between the client and server implementations. I'll look into it now, it would make the PHP backport easier if we decide to merge this for WordPress 6.6.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've pushed a commit that moves it to a standalone filter. I'll also work on a PHP backport for it now. Hopefully it's now mostly a copy/paste of that code.

}

$content = do_blocks( $content );
Copy link
Member

Choose a reason for hiding this comment

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

The new filter should be removed after rendering blocks, similar to how it happens to render_block_context:

remove_filter( 'render_block_data', $apply_default_pattern_overrides_bindings );

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, I've pushed a commit that does that 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Though I've now removed this code entirely as well. 😄

Expand Down
11 changes: 9 additions & 2 deletions packages/editor/src/bindings/pattern-overrides.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { _x } from '@wordpress/i18n';
import { store as blockEditorStore } from '@wordpress/block-editor';

const CONTENT = 'content';
const DEFAULT_ATTRIBUTES = '__default';

export default {
name: 'core/pattern-overrides',
Expand All @@ -19,10 +20,16 @@ export default {
true
);

const overridableValue =
const overriddenAttributes =
getBlockAttributes( patternClientId )?.[ CONTENT ]?.[
currentBlockAttributes?.metadata?.name
]?.[ attributeName ];
];
const overridableValue = overriddenAttributes?.[ attributeName ];
const defaultValue = overriddenAttributes[ DEFAULT_ATTRIBUTES ];

if ( defaultValue !== undefined ) {
return defaultValue;
}

// If there is no pattern client ID, or it is not overwritten, return the default value.
if ( ! patternClientId || overridableValue === undefined ) {
Expand Down
31 changes: 23 additions & 8 deletions packages/patterns/src/components/pattern-overrides-controls.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {

function removeBindings( bindings, syncedAttributes ) {
let updatedBindings = {};
// Back-compat: Remove existing duplicate bindings.
for ( const attributeName of syncedAttributes ) {
// Omit any bindings that's not the same source from the `updatedBindings` object.
if (
Expand All @@ -37,12 +38,17 @@ function removeBindings( bindings, syncedAttributes ) {
}

function addBindings( bindings, syncedAttributes ) {
const updatedBindings = { ...bindings };
const updatedBindings = {
...bindings,
__default: { source: PATTERN_OVERRIDES_BINDING_SOURCE },
};
// Back-compat: Remove existing duplicate bindings.
for ( const attributeName of syncedAttributes ) {
if ( ! bindings?.[ attributeName ] ) {
updatedBindings[ attributeName ] = {
source: PATTERN_OVERRIDES_BINDING_SOURCE,
};
if (
updatedBindings[ attributeName ]?.source ===
PATTERN_OVERRIDES_BINDING_SOURCE
) {
delete updatedBindings[ attributeName ];
}
}
return updatedBindings;
Expand All @@ -60,9 +66,18 @@ function PatternOverridesControls( { attributes, name, setAttributes } ) {
( attributeName ) =>
attributes.metadata?.bindings?.[ attributeName ]?.source
);
const isConnectedToOtherSources = attributeSources.every(
( source ) => source && source !== 'core/pattern-overrides'
);
const defaultBindings = attributes.metadata?.bindings?.__default;
const allowOverrides =
defaultBindings?.source === PATTERN_OVERRIDES_BINDING_SOURCE ||
attributeSources.some(
( source ) => source === PATTERN_OVERRIDES_BINDING_SOURCE
);
const isConnectedToOtherSources =
( defaultBindings?.source &&
defaultBindings.source !== PATTERN_OVERRIDES_BINDING_SOURCE ) ||
attributeSources.every(
( source ) => source && source !== PATTERN_OVERRIDES_BINDING_SOURCE
);

function updateBindings( isChecked, customName ) {
const prevBindings = attributes?.metadata?.bindings;
Expand Down
16 changes: 8 additions & 8 deletions test/e2e/specs/editor/various/pattern-overrides.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ test.describe( 'Pattern Overrides', () => {
metadata: {
name: editableParagraphName,
bindings: {
content: {
__default: {
source: 'core/pattern-overrides',
},
},
Expand Down Expand Up @@ -234,7 +234,7 @@ test.describe( 'Pattern Overrides', () => {
const paragraphName = 'paragraph-name';
const { id } = await requestUtils.createBlock( {
title: 'Pattern',
content: `<!-- wp:paragraph {"metadata":{"name":"${ paragraphName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
content: `<!-- wp:paragraph {"metadata":{"name":"${ paragraphName }","bindings":{"__default":{"source":"core/pattern-overrides"}}}} -->
<p>Editable</p>
<!-- /wp:paragraph -->`,
status: 'publish',
Expand Down Expand Up @@ -324,7 +324,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 }","bindings":{"__default":{"source":"core/pattern-overrides"}}}} -->
<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 @@ -434,14 +434,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 }","bindings":{"__default":{"source":"core/pattern-overrides"}}}} -->
<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 }","bindings":{"__default":{"source":"core/pattern-overrides"}}}} -->
<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 @@ -535,10 +535,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 }","bindings":{"__default":{"source":"core/pattern-overrides"}}}} -->
<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 }","bindings":{"__default":{"source":"core/pattern-overrides"}}}} -->
<p>Paragraph</p>
<!-- /wp:paragraph -->`,
status: 'publish',
Expand Down Expand Up @@ -694,7 +694,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 }","bindings":{"__default":{"source":"core/pattern-overrides"}}}} -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->`,
status: 'publish',
Expand Down