Bundle template images into theme assets and make their urls relative

[matiasbenedetto]

Initially discussed on #192, continuing here for a cleaner git history.

What?

If a template or a template part includes images which source is an absolute URL:

• The images are downloaded and bundled in the /assets/images folder of the theme
• The templates or template parts that include these images are converted to a pattern and added to the pattern folder. The template content is replaced by a reference to the pattern recently created.

Currently, we are considering the media content of these blocks:

• Image
• Cover
• Media & text
• Video

Why?

To make the process of launching a theme easier by bundling to theme assets the images inserted using the editor.

To Do:

Include other blocks including images as cover.

How to test:

• Include one or more image blocks in a template or template part using the editor.
• Save the changes
• Export the theme using the plugin
• See if the resulting theme works as expected

Closes: #161

[jffng]

This is testing pretty well!

After a template is transformed to a pattern, I noticed one behavior of template parts — they report missing and are unable to load, because they are missing the theme attribute which seems to be automatically inserted when the template part is initially rendered within a template.

Perhaps we need to address this in Gutenberg.

[matiasbenedetto]

After a template is transformed to a pattern, I noticed one behavior of template parts — they report missing and are unable to load, because they are missing the theme attribute which seems to be automatically inserted when the template part is initially rendered within a template.

Perhaps we need to address this in Gutenberg.

Thanks for testing @jffng !!
I'm not sure what behavior you are pointing out. Could you provide more details, please?
In my tests, I didn't see patterns failing to load but I don't know if you are referring to that.

[jffng]

Sure thing, here are the steps I replicated with the Adventurer theme:

1. Added an image directly to home.html
2. Exported the theme
3. Uploaded and activated it
4. Viewed the template, observe that the footer is missing:

My guess is this has to do with its transformation to being loaded from a pattern.

[matiasbenedetto]

I noticed one behavior of template parts — they report missing and are unable to load

This seems to be working fine now.

[mikachan]

This is awesome! 🔥

I noticed that any alt text on an image comes through correctly in the newly created pattern, but it's not wrapped in esc_attr_e(). I think it would be good to add this as part of this PR, both to escape the text and make it available for translations.

[matiasbenedetto]

Thanks for reviewing @mikachan !

I noticed that any alt text on an image comes through correctly in the newly created pattern, but it's not wrapped in esc_attr_e()

Due to the fact that we are not escaping for translation of any of the texts included in templates and template parts, do you think we can tackle translations in another PR that covers all the text contents of the theme?

[matiasbenedetto]

I created an issue to work on the translations: #221

[mikachan]

Due to the fact that we are not escaping for translation of any of the texts included in templates and template parts, do you think we can tackle translations in another PR that covers all the text contents of the theme?

I think we should tackle translating all text content from theme files and this could be done in a separate PR, but I think it's important to escape any alt text used in images as part of this PR, just for additional security (and we get the bonus of these strings being translatable.)

[matiasbenedetto]

Added the translation and scaping for the alt attribute of Image, Cover and Media&Text blocks.

[matiasbenedetto]

Follow-up issue: When WordPress/gutenberg#42485 is available in WordPress core (6.2), we could remove the fallback implementation of dom parsing using DOMDocument and/or regex introduced in this PR to circumvent the lack of that tool when Gutenberg is not installed.

[jffng]

I'm still getting the issue where template parts inside patterns are not found. Is it just me?

I think this is happening because the template part is not getting the "theme" block attribute injected correctly, e.g.

<!-- wp:template-part {"slug":"footer","tagName":"footer"} /-->

should be

<!-- wp:template-part {"slug":"footer","theme":"themes/adventurer","tagName":"footer"} /-->

Other than that, this PR is working nicely. For maintainability I wonder if we should prioritize refactoring the code into smaller files.

[bgardner]

Question: What happens if a theme does not have an /assets/images folder?

[matiasbenedetto]

Question: What happens if a theme does not have an /assets/images folder?

If that folder doesn't exist it should be created by the plugin to save the assets there.

[matiasbenedetto]

For maintainability I wonder if we should prioritize refactoring the code into smaller files.

I think this PR is already long enough. Adding a refactor of the entire class would make it longer and harder to review. Apart from that the need for refactoring is not a product of this feature but a more global enhancement. For those reasons, I think we can follow up on that task in another PR.

[matiasbenedetto]

I added support to core/video block.

[matiasbenedetto]

I think this is happening because the template part is not getting the "theme" block attribute injected correctly

Yep, that was the cause of the problem loading the template part. When you link a template part in a template you don't need to include the "theme" attribute but when you load the template part from a pattern the "theme" attribute is needed. Should be solved now.

[mikachan]

When you link a template part in a template you don't need to include the "theme" attribute but when you load the template part from a pattern the "theme" attribute is needed. Should be solved now.

I've tested this and it looks like the theme attribute is now included correctly!

However, I've noticed that if the generated theme name includes a hyphen, then this causes the same "Template part not found" error. It looks like the hyphen is removed when it's added to the template part block.

I created a theme called "tt3-clone1", and the exported template parts used "tt3clone1" as the theme attribute:

[matiasbenedetto]

However, I've noticed that if the generated theme name includes a hyphen, then this causes the same "Template part not found" error. It looks like the hyphen is removed when it's added to the template part block.

Good catch! It should be solved with the last commit.

[mikachan]

The theme attribute now looks correct in the template parts, but I'm still seeing some issues caused by the theme slug (but I don't think they've been introduced by this PR).

I'm seeing pattern blocks are using the wrong slug too, e.g. <!-- wp:pattern {"slug":"tt3clone6/post-meta"} /--> instead of <!-- wp:pattern {"slug":"tt3-clone6/post-meta"} /-->.

And the .zip file name of an exported theme also removes the hyphen.

It looks like the function get_theme_slug removes hyphens from the slug, based on this logic: the source theme has a single-word slug but the new theme has a multi-word slug (i.e. I'm cloning twentytwentythree and then naming the new theme tt3-clone).

I think it would be better if we allowed hyphens if they've been specified in the theme name input, as I imagine this is a common way to format a theme name.

Sorry, I feel like I'm blocking this PR with this. Maybe we could merge this now, and create a follow-up issue to address the hyphens in slugs? It's very specific to the "single word source theme, new theme with hyphen in name" use case. 😅

[matiasbenedetto]

Yep, that's the current handling of the name/slug in the plugin. It certainly can be improved but is out of the scope of this PR. I think a follow-up issue would be the way to go here.

[mikachan]

Ok, sounds good to me!