Theme export: convert uploaded URLs to relative path assets #66407

ramonjd · 2024-10-24T11:08:08Z

🚧

Warning

Experimental code - please don't treat it as ready yet. Thank you!

What? How?

This is a first pass, so it's pretty rough and tumble. No tests or defensive coding yet.

This PR checks a theme tree for URIs containing the current site's upload path. If found:

it replaces the paths with a relative equivalent: 'http://domain/wp-content/uploads/1/2/test.gif' >' file:./uploads/test.gif'
fetches the file and adds it to the zip in /uploads

The outcome is that the uploaded files become part of the new theme's asset library, and can therefore be installed anywhere.

Why?

A theme can use uploaded files for:

fonts
background images

The paths to these assets are absolute URIs to the uploaded files.

Currently, when exporting themes these URIs are preserved in the theme.json, which means that the theme isn't "portable" in that relies on those hosted assets.

Testing Instructions

In the site editor, go to global styles and upload a font or two.
Upload some images and use them as background images for the site or for quote/pullquote/group blocks.
Save the styles.
Now export the theme and open the zip.
In theme.json, the absolute paths to uploaded files should be converted to relative file:./ paths
The uploaded files themselves should be available in /uploads in the zip.
Now upload the exported theme and activate it in your site. Check that it all works as expected.

Exported theme.json [BEFORE]

					"fontFace": [
						{
							"fontFamily": "\"Star Jedi\"",
							"fontStyle": "normal",
							"fontWeight": "400",
							"src": "http://localhost:8888/wp-content/uploads/fonts/Starjedi.ttf"
						}
					],

Exported theme.json [AFTER]

					"fontFace": [
						{
							"fontFamily": "\"Star Jedi\"",
							"fontStyle": "normal",
							"fontWeight": "400",
							"src": "file:./uploads/Starjedi.ttf"
						}
					],

Screenshots or screencast

lib/block-template-utils.php

github-actions · 2024-10-24T21:47:09Z

Flaky tests detected in 8646cce.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11512116126
📝 Reported issues:

[Flaky Test] The navigation background color should apply to all navigation links including submenu and overlay backgrounds #64763 in /test/e2e/specs/editor/blocks/navigation-colors.spec.js

lib/block-template-utils.php

ramonjd · 2024-10-25T04:57:20Z

lib/global-styles-and-settings.php

@@ -30,6 +30,8 @@ function gutenberg_get_global_stylesheet( $types = array() ) {
 	$tree = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data();
 	$tree = WP_Theme_JSON_Resolver_Gutenberg::resolve_theme_file_uris( $tree );

+	var_dump( WP_Theme_JSON_Resolver_Gutenberg::get_migrated_relative_theme_uris( $tree ) );


Just to test the output

ramonjd · 2024-10-25T04:58:09Z

lib/class-wp-theme-json-resolver-gutenberg.php

-	 */
-	public static function get_resolved_theme_uris( $theme_json ) {
-		$resolved_theme_uris = array();
+	private static function process_theme_uris( $theme_json, $options = array() ) {


Everything here and below is a major refactor. Just testing things out for now.

Basically so we have a single source of truth about where these types of URIs live.

andrewserong

Just a drive-by comment to say I love this idea, feels like a quality of life improvement that'll make exporting feel that much smoother 👍

In terms of the directory where we save the file, at first I saw "uploads" as a directory and wondered if "assets" would be better. Now that I see it's been updated to "assets", I'm wondering if that could conflict with how a theme has structured its other... assets?

Naming and defaults are tricky to figure out, but I was wondering what the trade-offs might be between storing the uploaded files somewhere explicit so that they don't conflict (or aren't mixed up with) files that live with the theme (uploads) OR attempting to place them somewhere that makes sense as a permanent home for the files (assets) 🤔

I was half wondering if we might need an export modal at some point for configuration of the output zip — with this PR there'd only be a single field, though, so maybe not very useful just yet 😄

Anyway, mostly just wanted to say thanks for working on this!

ramonjd · 2024-10-25T05:26:27Z

Thank you @andrewserong!

I'm a bit obsessed with it now - I want to get it working 🤣 With fonts working, it's a pretty good quality of life upgrade!

Now that I see it's been updated to "assets", I'm wondering if that could conflict with how a theme has structured its other... assets?

I'm dumping the files under assets in the zip, but I guess there could be a slight risk of overwriting existing files where assets already exists. Then again, there's that risk with any name we choose.

Maybe /assets/_external or /assets/_custom would mitigate it? I don't know. This PR is now so messy, I'm sure we'll think of something by the time I clean it up and write tests. 😄

I was half wondering if we might need an export modal at some point for configuration of the output zip

Good idea for down the road!

youknowriad · 2024-10-25T06:46:07Z

I like "assets", I think it's good to set a convention there.

I don't understand much how we'll be overwriting anything though? We're exporting a new theme right and we control all its content no?

ramonjd · 2024-10-25T06:52:36Z

I don't understand much how we'll be overwriting anything though? We're exporting a new theme right and we control all its content no?

Might have been paranoia. Just in case an uploaded file has the same name as a theme file already under /assets, if that's even likely.

carolinan · 2024-10-25T08:35:02Z

Thank you for working on this.
I assume you have already compared with how it is solved in Create Block Theme?

I don't understand much how we'll be overwriting anything though? We're exporting a new theme right and we control all its content no?

I am not sure I understood the question correctly.
The Export from the Site Editor includes all the existing folders and files in the active theme. Plus the user's changes.

youknowriad · 2024-10-25T08:48:00Z

The Export from the Site Editor includes all the existing folders and files in the active theme.

I see I didn't know about this. Personally, I'm not entirely sure this is a good idea. I'd rather "compute" the full theme.

ramonjd · 2024-10-25T21:36:31Z

Thanks for the feedback!

I assume you have already compared with how it is solved in Create Block Theme?

For fonts, this PR does something very similar to CBT_Theme_Zip::add_activated_fonts_to_zip() (src), however Create Block Theme doesn't handle the migration of background urls, so it will export theme.json like this:

	"styles": {
		"background": {
			"backgroundImage": {
				"id": 7,
				"source": "file",
				"title": "default_man_sitting_alone_eating_lunch_thinking_about_WordPressXRhXpu",
				"url": "http://localhost:8888/wp-content/uploads/2024/10/default_man_sitting_alone_eating_lunch_thinking_about_WordPressXRhXpu.png"
			}
		},

The goal of this PR is for the exported theme.json to look more like this:

	"styles": {
		"background": {
			"backgroundImage": {
				"url": "file:./assets/default_man_sitting_alone_eating_lunch_thinking_about_WordPressXRhXpu.png"
			}
		},

The image file is fetched and dumped in /assets/ in the zip.

I'd rather "compute" the full theme.

What would that look like do you think?

In this PR, I don't think it's much of a stretch to check for duplicate names in the destination directory via locateName() and then do some renaming faffery. 🤷🏻

lib/block-template-utils.php

Ensure to strip background image properties before setting relative path

ramonjd · 2024-10-28T04:01:29Z

lib/block-template-utils.php

+				$href   = array(
+					'url' => $href,
+				);
+			}


I can test this in the Core patch since Core tests have access to ZipArchive.

E.g., https://github.com/WordPress/wordpress-develop/blob/de38e8eeecf80a7ca5ee870ec68a59897216b33b/tests/phpunit/tests/block-template-utils.php#L379

carolinan · 2024-11-21T11:01:21Z

The issue for this PR is: #41798

First try at converting all uploaded URLs to relative path assets

691e477

ramonjd self-assigned this Oct 24, 2024

ramonjd added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Experimental Experimental feature or API. labels Oct 24, 2024

ramonjd mentioned this pull request Oct 24, 2024

User global styles: should we save relative paths to the database for uploaded assets? #61809

Open

ramonjd commented Oct 24, 2024

View reviewed changes

lib/block-template-utils.php Outdated Show resolved Hide resolved

lib/block-template-utils.php Outdated Show resolved Hide resolved

lib/block-template-utils.php Outdated Show resolved Hide resolved

lib/block-template-utils.php Show resolved Hide resolved

ramonjd changed the title ~~First try at converting all uploaded URLs to relative path assets~~ Theme export: convert uploaded URLs to relative path assets Oct 24, 2024

ramonjd added the [Status] In Progress Tracking issues with work in progress label Oct 24, 2024

ramonjd added 3 commits October 25, 2024 08:10

Annotate

75b89bf

Use assets dir instead

1d18b83

$is_assets_folder_created is superfluous thanks to $zip->locateName

1d8e790

The linter of DOOM

d68d58f

youknowriad reviewed Oct 24, 2024

View reviewed changes

lib/block-template-utils.php Outdated Show resolved Hide resolved

No regex - move everything to theme resolver.

8646cce

ramonjd commented Oct 25, 2024

View reviewed changes

andrewserong reviewed Oct 25, 2024

View reviewed changes

Cleaning up, trying to make things logical and abstracted.

a04bb11

Add some tests, clean up a bit.

ac3fa04

ramonjd added 2 commits October 25, 2024 17:53

Okay, linter, you win.

7889c4c

Naming is hard

d153229

Added TODO

bb2bf77

ramonjd commented Oct 25, 2024

View reviewed changes

lib/block-template-utils.php Outdated Show resolved Hide resolved

Move callbacks into body of resolver functions

ee8c257

Ensure to strip background image properties before setting relative path

ramonjd commented Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme export: convert uploaded URLs to relative path assets #66407

Theme export: convert uploaded URLs to relative path assets #66407

ramonjd commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading

ramonjd Oct 25, 2024

ramonjd Oct 25, 2024

andrewserong left a comment

ramonjd commented Oct 25, 2024

youknowriad commented Oct 25, 2024

ramonjd commented Oct 25, 2024

carolinan commented Oct 25, 2024 •

edited

Loading

youknowriad commented Oct 25, 2024

ramonjd commented Oct 25, 2024

ramonjd Oct 28, 2024

carolinan commented Nov 21, 2024

Theme export: convert uploaded URLs to relative path assets #66407

Are you sure you want to change the base?

Theme export: convert uploaded URLs to relative path assets #66407

Conversation

ramonjd commented Oct 24, 2024 • edited Loading

What? How?

Why?

Testing Instructions

Exported theme.json [BEFORE]

Exported theme.json [AFTER]

Screenshots or screencast

github-actions bot commented Oct 24, 2024 • edited Loading

ramonjd Oct 25, 2024

Choose a reason for hiding this comment

ramonjd Oct 25, 2024

Choose a reason for hiding this comment

andrewserong left a comment

Choose a reason for hiding this comment

ramonjd commented Oct 25, 2024

youknowriad commented Oct 25, 2024

ramonjd commented Oct 25, 2024

carolinan commented Oct 25, 2024 • edited Loading

youknowriad commented Oct 25, 2024

ramonjd commented Oct 25, 2024

ramonjd Oct 28, 2024

Choose a reason for hiding this comment

carolinan commented Nov 21, 2024

ramonjd commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading

carolinan commented Oct 25, 2024 •

edited

Loading