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.

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

Transforms: Shortcode: Support isMatch predicate #18459

Merged
merged 4 commits into from
Nov 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,24 @@ transforms: {
```
{% end %}

In the case of shortcode transforms, `isMatch` receives shortcode attributes per the [Shortcode API](https://codex.wordpress.org/Shortcode_API):

{% codetabs %}
{% ES5 %}
```js
isMatch: function( attributes ) {
return attributes.named.id === 'my-id';
},
```
{% ESNext %}
```js
isMatch( { named: { id } } ) {
return id === 'my-id';
},
```
{% end %}


To control the priority with which a transform is applied, define a `priority` numeric property on your transform object, where a lower value will take precedence over higher values. This behaves much like a [WordPress hook](https://codex.wordpress.org/Plugin_API#Hook_to_WordPress). Like hooks, the default priority is `10` when not otherwise set.

A file can be dropped into the editor and converted into a block with a matching transform.
Expand Down
20 changes: 19 additions & 1 deletion packages/blocks/src/api/raw-handling/shortcode-converter.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ import { createBlock, getBlockTransforms, findTransform } from '../factory';
import { getBlockType } from '../registration';
import { getBlockAttributes } from '../parser';

function segmentHTMLToShortcodeBlock( HTML, lastIndex = 0 ) {
function segmentHTMLToShortcodeBlock( HTML, lastIndex = 0, excludedBlockNames = [] ) {
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
// Get all matches.
const transformsFrom = getBlockTransforms( 'from' );

const transformation = findTransform( transformsFrom, ( transform ) => (
excludedBlockNames.indexOf( transform.blockName ) === -1 &&
transform.type === 'shortcode' &&
some( castArray( transform.tag ), ( tag ) => regexp( tag ).test( HTML ) )
) );
Expand All @@ -32,6 +33,7 @@ function segmentHTMLToShortcodeBlock( HTML, lastIndex = 0 ) {
const transformTag = find( transformTags, ( tag ) => regexp( tag ).test( HTML ) );

let match;
const previousIndex = lastIndex;

if ( ( match = next( transformTag, HTML, lastIndex ) ) ) {
const beforeHTML = HTML.substr( 0, match.index );
Expand All @@ -49,6 +51,22 @@ function segmentHTMLToShortcodeBlock( HTML, lastIndex = 0 ) {
return segmentHTMLToShortcodeBlock( HTML, lastIndex );
}

// If a transformation's `isMatch` predicate fails for the inbound
// shortcode, try again by excluding the current block type.
//
// This is the only call to `segmentHTMLToShortcodeBlock` that should
// ever carry over `excludedBlockNames`. Other calls in the module
// should skip that argument as a way to reset the exclusion state, so
// that one `isMatch` fail in an HTML fragment doesn't prevent any
// valid matches in subsequent fragments.
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
if ( transformation.isMatch && ! transformation.isMatch( match.shortcode.attrs ) ) {
return segmentHTMLToShortcodeBlock(
HTML,
previousIndex,
[ ...excludedBlockNames, transformation.blockName ],
);
}

const attributes = mapValues(
pickBy( transformation.attributes, ( schema ) => schema.shortcode ),
// Passing all of `match` as second argument is intentionally broad
Expand Down
40 changes: 39 additions & 1 deletion test/integration/blocks-raw-handling.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
getBlockContent,
pasteHandler,
rawHandler,
registerBlockType,
serialize,
} from '@wordpress/blocks';
import { registerCoreBlocks } from '@wordpress/block-library';
Expand All @@ -24,6 +25,38 @@ describe( 'Blocks raw handling', () => {
// Load all hooks that modify blocks
require( '../../packages/editor/src/hooks' );
registerCoreBlocks();
registerBlockType( 'test/gallery', {
title: 'Test Gallery',
category: 'common',
attributes: {
ids: {
type: 'array',
default: [],
},
},
transforms: {
from: [
{
type: 'shortcode',
tag: 'gallery',
isMatch( { named: { ids } } ) {
return ids.indexOf( 42 ) > -1;
Copy link
Member

Choose a reason for hiding this comment

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

Is this your answer to everything? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😄

},
attributes: {
ids: {
type: 'array',
shortcode: ( { named: { ids } } ) =>
ids.split( ',' ).map( ( id ) => (
parseInt( id, 10 )
) ),
},
},
priority: 9,
},
],
},
save: () => null,
} );
} );

it( 'should filter inline content', () => {
Expand Down Expand Up @@ -248,12 +281,17 @@ describe( 'Blocks raw handling', () => {
'markdown',
'wordpress',
'gutenberg',
'caption-shortcode',
'shortcode-matching',
].forEach( ( type ) => {
it( type, () => {
const HTML = readFile( path.join( __dirname, `fixtures/${ type }-in.html` ) );
const plainText = readFile( path.join( __dirname, `fixtures/${ type }-in.txt` ) );
const output = readFile( path.join( __dirname, `fixtures/${ type }-out.html` ) );

if ( ! ( HTML || plainText ) || ! output ) {
throw new Error( `Expected fixtures for type ${ type }` );
}

const converted = pasteHandler( { HTML, plainText, canUserUseUnfilteredHTML: true } );
const serialized = typeof converted === 'string' ? converted : serialize( converted );

Expand Down
3 changes: 3 additions & 0 deletions test/integration/fixtures/shortcode-matching-in.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<p>[gallery ids="40,41,42"]</p>
<p>[gallery ids="1000"]</p>
<p>[gallery ids="42"]</p>
7 changes: 7 additions & 0 deletions test/integration/fixtures/shortcode-matching-out.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<!-- wp:test/gallery {"ids":[40,41,42]} /-->

<!-- wp:gallery {"ids":[1000],"columns":3,"linkTo":"attachment"} -->
<figure class="wp-block-gallery columns-3 is-cropped"><ul class="blocks-gallery-grid"><li class="blocks-gallery-item"><figure><img data-id="1000" class="wp-image-1000"/></figure></li></ul></figure>
<!-- /wp:gallery -->

<!-- wp:test/gallery {"ids":[42]} /-->
89 changes: 89 additions & 0 deletions test/integration/shortcode-converter.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,60 @@ describe( 'segmentHTMLToShortcodeBlock', () => {
},
save: () => null,
} );
registerBlockType( 'test/broccoli', {
title: 'Test Broccoli',
category: 'common',
attributes: {
id: {
type: 'number',
},
},
transforms: {
from: [
{
type: 'shortcode',
tag: [ 'my-broccoli' ],
attributes: {
id: {
type: 'number',
shortcode: ( { named: { id } } ) => parseInt( id, 10 ),
},
},
isMatch( { named: { id } } ) {
return id < 1000;
},
},
],
},
save: () => null,
} );
registerBlockType( 'test/fallback-broccoli', {
title: 'Test Fallback Broccoli',
category: 'common',
attributes: {
id: {
type: 'number',
},
},
transforms: {
from: [
{
type: 'shortcode',
tag: [ 'my-broccoli' ],
attributes: {
id: {
type: 'number',
shortcode: ( { named: { id } } ) => parseInt( id, 10 ),
},
},
isMatch( { named: { id } } ) {
return id > 1000;
},
},
],
},
save: () => null,
} );
} );

it( 'should convert a standalone shortcode between two paragraphs', () => {
Expand All @@ -64,6 +118,41 @@ describe( 'segmentHTMLToShortcodeBlock', () => {
<p>Bar</p>` );
} );

it( 'should convert a shortcode to a block type with a passing `isMatch`', () => {
const original = `<p>[my-broccoli id="42"]</p>`;

const transformed = segmentHTMLToShortcodeBlock( original, 0 );
const expectedBlock = createBlock( 'test/broccoli', { id: 42 } );
expectedBlock.clientId = transformed[ 1 ].clientId;
expect( transformed[ 1 ] ).toEqual( expectedBlock );
} );

it( 'should not convert a shortcode to a block type with a failing `isMatch`', () => {
const original = `<p>[my-broccoli id="1000"]</p>`;

const transformed = segmentHTMLToShortcodeBlock( original, 0 );
const expectedBlock = createBlock( 'core/shortcode' );
expectedBlock.clientId = transformed[ 1 ].clientId;
expect( transformed[ 1 ] ).toEqual( expectedBlock );
} );

it( 'should not blindly exclude a transform in subsequent shortcodes after a failed `isMatch`', () => {
const original = `<p>[my-broccoli id="1001"]</p>
<p>[my-broccoli id="42"]</p>
<p>[my-broccoli id="1000"]</p>`;

const transformed = segmentHTMLToShortcodeBlock( original );
Copy link
Member

Choose a reason for hiding this comment

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

Could we add some integration tests as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix, a few of questions:

  1. How would they differ from the current ones? It's not like any core blocks are using isMatch, nor that there are multiple core blocks interested in the same shortcode tag.
  2. Looking at blocks-raw-handling.test.js, I notice that there are now three "areas" for testing:
  • individual tests under Blocks raw handling
  • the fixture-based tests under pasteHandler
  • individual tests under rawHandler

Seems like we could revisit this? I personally don't really know where to start adding tests for this.

  1. That readFile helper silently bails if the provided file path doesn't exist, meaning that types in pasteHandler can just silently pass even when there are missing fixtures. Case in point: caption-shortcode no longer runs. Seems like something to fix too, I don't see a reason not to throw an error if a file is missing.

Copy link
Member

Choose a reason for hiding this comment

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

It's not like any core blocks are using isMatch, nor that there are multiple core blocks interested in the same shortcode tag.

Interestingly, this could be changed. An example off the top of my head is that the [video] shortcode in Core explicitly supports YouTube and Vimeo URLs in the src attribute. These shortcodes could be transformed into their respective core-embed block, rather than the core/video block.

Copy link
Member

@ellatrix ellatrix Nov 18, 2019

Choose a reason for hiding this comment

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

  1. How would they differ from the current ones? It's not like any core blocks are using isMatch, nor that there are multiple core blocks interested in the same shortcode tag.

That's true, but we could add a test block?

Seems like we could revisit this? I personally don't really know where to start adding tests for this.

Yeah, we should clean that up :) Ideally, if applicable, tests should be run for both the raw and paste handler.

That readFile helper silently bails if the provided file path doesn't exist, meaning that types in pasteHandler can just silently pass even when there are missing fixtures. Case in point: caption-shortcode no longer runs. Seems like something to fix too, I don't see a reason not to throw an error if a file is missing.

Me neither. Sounds good to throw an error.

const firstExpectedBlock = createBlock( 'test/fallback-broccoli', { id: 1001 } );
firstExpectedBlock.clientId = transformed[ 1 ].clientId;
const secondExpectedBlock = createBlock( 'test/broccoli', { id: 42 } );
secondExpectedBlock.clientId = transformed[ 3 ].clientId;
const thirdExpectedBlock = createBlock( 'core/shortcode' );
thirdExpectedBlock.clientId = transformed[ 5 ].clientId;
expect( transformed[ 1 ] ).toEqual( firstExpectedBlock );
expect( transformed[ 3 ] ).toEqual( secondExpectedBlock );
expect( transformed[ 5 ] ).toEqual( thirdExpectedBlock );
} );

it( 'should convert two instances of the same shortcode', () => {
const original = `<p>[foo one]</p>
<p>[foo two]</p>`;
Expand Down