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

Try: parse shortcode blocks outside the content #37545

Merged
merged 3 commits into from
Dec 22, 2021

Conversation

carolinan
Copy link
Contributor

@carolinan carolinan commented Dec 21, 2021

Description

Adds do_shortcode() to parse the shortcode when it is placed outside the content (block templates, block patterns).

Closes #35258. Partially #37312

How has this been tested?

Sample shortcode block and smilies:

<!-- wp:shortcode -->
[caption align="aligncenter" width="300"]<img src="https://pd.w.org/2021/12/46561c16fc18dc371.43208413.jpeg" width="300"
	height="205" class="size-medium " />Testing[/caption]
<!-- /wp:shortcode -->

<!-- wp:paragraph --><p>:) ;) :D</p><!-- /wp:paragraph -->

;)

Tested manually by:

  1. Activating Twenty Twenty-Two
  2. Placing the sample code in the home.html template of Twenty Twenty-Two.
  3. Placing the sample code in a post, using the block editor code editor mode.
  4. Viewing the front and confirming both shortcodes are rendered.

Screenshots

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

Adds do_shortcode() to parse the shortcode outside the content (block templates, block patterns)
@carolinan carolinan requested a review from ajitbohra as a code owner December 21, 2021 08:17
@carolinan carolinan added the [Feature] Shortcodes Related to shortcode functionality label Dec 21, 2021
@carolinan carolinan marked this pull request as draft December 21, 2021 08:19
@carolinan carolinan marked this pull request as ready for review December 21, 2021 12:31
@Mamaduka
Copy link
Member

Hi, @carolinan

I think we should resolve this by adding do_shorcode inside gutenberg_get_the_template_html, maybe after the wp_filter_content_tags.

It is how we handle it for Template Parts.

A related conversation - #17626 (comment)

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

It works as expected. Thanks, @carolinan.

P.S. I've updated the PR description, so it correctly links the issue.

@carlomanf
Copy link

Don't we need to run shortcode_unautop before do_shortcode?

@Mamaduka
Copy link
Member

Good point, @carlomanf. I can see empty p tags around shortcode without shortcode_unautop.

@carolinan
Copy link
Contributor Author

OK, how about now, I added both convert_smilies and shortcode_unautop.

@Mamaduka
Copy link
Member

@carolinan

I'm assuming you're using convert_smilies to fix the Emoji issue, but that function is pretty much the legacy way WP used to handle simple emojis in comments. So I don't think we need it here.

Also, let's run shortcode_unautop right be before do_shortcode unless you get some unwanted results when doing so.

@carolinan
Copy link
Contributor Author

Well, it matches what's in the template part:
https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/template-part/index.php#L130

@Mamaduka
Copy link
Member

Good call. Feel free to merge once all checks are green.

Thanks again for working on this, @carolinan.

@carolinan carolinan merged commit 59fbccf into trunk Dec 22, 2021
@carolinan carolinan deleted the try/shortcode-outside-content branch December 22, 2021 09:17
@github-actions github-actions bot added this to the Gutenberg 12.3 milestone Dec 22, 2021
@Mamaduka
Copy link
Member

@noisysocks, should we backport this to WP 5.9?

@Mamaduka Mamaduka added the [Type] Bug An existing feature does not function as intended label Dec 30, 2021
@noisysocks
Copy link
Member

Missed this, sorry. I think it's too late for WP 5.9 as RC 1 was released this morning.

Since it isn't landing in WP 5.9 we should move the changes out of lib/compat/wordpress-5.9 so that it isn't accidentally deleted.

@carlomanf
Copy link

After some thought, I am doubtful if the approach here is sound. The reason is that many of the filters here (wptexturize, convert_smilies, do_shortcode etc) will end up applied twice or more on the same content. They are applied once on a template part or post content block, and then applied again when processing the entire filled-in template.

This is inefficient at best and could cause unexpected behaviour at worst. I imagine that if you double-wrap a shortcode to escape it (e.g. [[shortcode]]) it will end up getting accidentally evaluated, although I didn't test for sure.

What might be a better approach is to add a condition to some of the default filters to only be enqueued if a block template is not active, and if a block template is active, apply them just once to the entire filled-in template.

By the way, another issue has been opened: https://core.trac.wordpress.org/ticket/54759

@Mamaduka
Copy link
Member

Mamaduka commented Jan 8, 2022

@carlomanf, I agree there's some issue with this approach in general (not with this PR). The wptexturize already causes some problems - #37754 (comment)

@carlomanf
Copy link

I imagine that if you double-wrap a shortcode to escape it (e.g. [[shortcode]]) it will end up getting accidentally evaluated, although I didn't test for sure.

Now that I think about it, maybe the same could also potentially happen with a double-wrapped shortcode inside a post content block inside a query block inside the_content() on a non-block theme? Hopefully someone can test.

@Mamaduka
Copy link
Member

Mamaduka commented Jan 8, 2022

I will try to do more testing on Monday.

@Mamaduka Mamaduka added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jan 12, 2022
@audrasjb
Copy link
Contributor

@Mamaduka @noisysocks we should probably backport this to WP 5.9, not in the next minor :)

@Mamaduka
Copy link
Member

Mamaduka commented Jan 13, 2022

@audrasjb, I tested shortcode escaping as @carlomanf suggested, and it is not working after this PR I'm getting mixed results.

What do you think about moving wptexturize and shortcode handles before do_blocks?

I'm pro including this into 5.9 if we can resolve these issues.

Example shortcode:

add_shortcode( 'note', function() {
	ob_start();
	?>
		<div class="note" style="margin: 30px 0; padding: 27px 40px; background: #fcf3dc; border-radius: 5px;">
			<p>This is a note!</p>
		</div>
		<?php
	return ob_get_clean();
} );

@carlomanf
Copy link

it is not working after this PR I'm getting mixed results.

What do you think about moving wptexturize and shortcode handles before do_blocks?

Can you elaborate more about the mixed results? I would expect shortcodes directly in the template to escape correctly, but ones in a template part or a post content within the template to not work, because those blocks apply do_shortcode themselves before the template applies it again.

I would also expect moving do_shortcode before do_blocks to cause shortcodes inside reusable blocks to stop working, because reusable blocks don't apply do_shortcode on render.

@carlomanf
Copy link

I think this will require a complex fix and I personally don't see any problem with it being left out of 5.9 because block templates were introduced in 5.8 and have been lacking shortcode support for a full release cycle already.

@Mamaduka
Copy link
Member

Here are my results for escaped shortcut tests:

  • Escaping fails Single post/page template is unmodified.
  • Escaping works when I update the template via Site Editor.

@noisysocks noisysocks added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release labels Jan 13, 2022
@noisysocks
Copy link
Member

OK I trust y'all 🙂

@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jan 17, 2022
noisysocks pushed a commit that referenced this pull request Jan 17, 2022
* Try: parse shortcode blocks outside the content
Adds do_shortcode() to parse the shortcode outside the content (block templates, block patterns)
noisysocks added a commit to noisysocks/wordpress-develop that referenced this pull request Jan 18, 2022
@noisysocks
Copy link
Member

noisysocks commented Jan 18, 2022

I misread some of the conversation here and thought the consensus was to yes to backporting this. Thanks @Mamaduka for pointing out to me that the consensus was actually no. It was included in WordPress/wordpress-develop#2184 which will land in 5.9 RC 3. Sorry about that.

Since it's a PHP-only change, it's very easy to revert. (No need to publish packages.) I think we can leave it in 5.9 RC 3 and if there are any bug reports to do with this then we can revert the changes before the final release next week.

Ping me if you see any such reports. It would also be helpful, of course, to download 5.9 RC 3 when it's published today and test it 😀

@carlomanf
Copy link

Ping me if you see any such reports.

The report was lodged right here above before you asked for it. If this change was included mistakenly, then I don't understand why it wasn't reverted immediately after the mistake was noticed.

To be crystal clear, this is the kind of bug we are talking about:

  1. User is running a non-block theme and is printing an escaped shortcode on their post/page
  2. User activates a block theme
  3. The escaped shortcode now gets evaluated the same as an un-escaped shortcode, and the severity of the unwanted effects is only limited by whatever the shortcode happens to do.

The above is based on my own testing, and although I wasn't able to reproduce the mixed results that @Mamaduka did, the escaping did fail on my first test, which is evidence enough that this is not ready to be released.

@noisysocks
Copy link
Member

Could you open a PR to include in 5.9.1?

@garretthyder
Copy link
Contributor

If Shortcode support can be fixed instead of reverted in 5.9.1 that would be preferred/appreciated by myself and any plugin developers who use shortcodes to render their Block contents. We build our Block & Widget on top of our Shortcode for easier cross-context support. Until WP5.9RC3 was release it was broken in the Site Editor context due to the Shortcode not rendering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Shortcodes Related to shortcode functionality [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FSE templates not rendering shortcodes used in blocks
6 participants