Skip to content

Pattern: process embeds #55979

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
Merged
merged 1 commit into from
Nov 13, 2023
Merged

Pattern: process embeds #55979

merged 1 commit into from
Nov 13, 2023
+6 −1

Conversation

dsas
Copy link
Contributor

@dsas dsas commented Nov 8, 2023 •
edited
Loading

What?

Process embed blocks inside a pattern file so that the media is embedded rather than just outputting a URL.

Why?

Gutenberg lets you create patterns that include embed blocks. When the pattern is included in a template file the embed in the editor properly embeds and shows the embedded item. When the front-end is viewed the embed is not processed and instead it just shows a URL.

The front-end should display what Gutenberg does.

Additionally, this works for template-parts, and for patterns included by template-parts, just not for patterns included by templates.

This addresses #46556

How?

The PHP render_callback function for the core/pattern block now uses WP_Embed::autoembed to expand any autoembeds. I've copied how template-parts does the same thing, but template-parts does a lot of other stuff in addition, I don't think they are needed for embed purposes but maybe they should be applied too?

Testing Instructions

  1. Add a pattern file to emptytheme (or your active theme)
  2. In the pattern file, add an embed block
  3. Add the pattern to an HTML template (not through the Site Editor)
  4. Load the page - on trunk the embed URL is displayed, but not embedded and vice versa on this branch

Here's a git diff to emptytheme you can use to do steps 1-3: test.patch Download it to the root of your gb and then git apply test.patch

Testing Instructions for Keyboard

N/A

Screenshots or screencast

Editor Frontend Before Frontend After
Screenshot 2023-11-08 at 20 33 08 Screenshot 2023-11-08 at 20 34 30 Screenshot 2023-11-08 at 20 33 46

@dsas
Process embeds inside patterns
88c440b 
Embed blocks inside patterns were not being processed when viewing a
site. This focused change addresses that issue.
@dsas dsas requested a review from ajitbohra as a code owner November 8, 2023 20:51
@dsas dsas self-assigned this Nov 8, 2023
@dsas dsas requested review from ockham and jorgefilipecosta and removed request for ajitbohra November 8, 2023 20:52
@dsas dsas added [Type] Bug An existing feature does not function as intended [Block] Pattern Affects the Patterns Block [Package] Block library /packages/block-library labels Nov 8, 2023
tellthemachines
tellthemachines approved these changes Nov 12, 2023
View reviewed changes
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The change is working well for me following the testing steps in #46556.

template-parts does a lot of other stuff in addition

You mean the content processing above the autoembed call? I don't think we need that or I expect it would have been added by now. It's not necessary for embeds to work.

Also to your question in the linked issue

I'm not sure why the embed HTML generation code isn't part of the embed block itself

This is likely because it's not needed for the embed block itself, as this issue only happens when it's added in places like template parts and patterns but perhaps @talldan has more context as he worked on the original workaround for block widgets

@talldan
Copy link
Contributor

talldan commented Nov 13, 2023

template-parts does a lot of other stuff in addition

It might be good to audit whether things like convert_smilies work as expected. Also not sure about shortcodes, some more care has to be a bit more careful about where they're processed to make sure they don't run on user generated content.

I'm not sure why the embed HTML generation code isn't part of the embed block itself

I think it's a historical WordPress thing. Embeds were always processed by the code that renders a post (before blocks existed), and the same approach had to be kept after blocks were introduced to ensure that classic and block embed content worked the same way. Especially given classic and block content can still co-exist in the same post.

The same approach to processing embeds has now expanded to template parts, templates etc, as they're essentially also responsible for rendering a post.

It could be something to revisit, I do like the idea of moving the processing to the block.

@dsas
Copy link
Contributor Author

dsas commented Nov 13, 2023

Thanks @tellthemachines & @talldan

You mean the content processing above the autoembed call? I don't think we need that or I expect it would have been added by now. It's not necessary for embeds to work.

Yep, that's what I meant. Not specifically that it was required for embeds, but that it might have the same problem as embeds.

It looks like convert_smileys and texturize both still work in patterns. I don't think shortcodes do, but i'm not sure how they should be written anyway - I could be doing it wrong. Happy to leave that can of worms out of scope.

I don't have commit permissions btw - I'm not able to merge this change myself.

@tellthemachines
Copy link
Contributor

I don't think shortcodes do, but i'm not sure how they should be written anyway - I could be doing it wrong. Happy to leave that can of worms out of scope.

Yeah no worries, that's fine!

@tellthemachines tellthemachines merged commit 4d3b148 into WordPress:trunk Nov 13, 2023
@github-actions github-actions bot added this to the Gutenberg 17.1 milestone Nov 13, 2023
cbravobernal pushed a commit that referenced this pull request Nov 14, 2023
@dsas @cbravobernal
Process embeds inside patterns (#55979)
dbd5000 
Embed blocks inside patterns were not being processed when viewing a
site. This focused change addresses that issue.
@dsas dsas mentioned this pull request Nov 14, 2023
Closed
@oandregal oandregal changed the title Process embeds inside patterns Pattern: process embeds Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tellthemachines tellthemachines

@ockham ockham

@jorgefilipecosta jorgefilipecosta

Assignees

@dsas dsas

Labels
[Block] Pattern Affects the Patterns Block [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 17.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dsas @talldan @tellthemachines