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

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

Open
ramonjd opened this issue May 20, 2024 · 10 comments
Labels

Comments

@ramonjd
Copy link
Member

ramonjd commented May 20, 2024

What problem does this address?

When adding a background image or a web font via the site editor global styles tools, the absolute path is saved in the global styles post content to the database.

For example:

Web fonts post theme.json in the database:

"fontFace":[{"src":"http:\/\/localhost:8888\/wp-content\/uploads\/fonts\/esDR31xSG-6AGleN6teukbcHCpE.woff2","fontWeight":"400","fontStyle":"normal","fontFamily":"ABeeZee","preview":"https:\/\/s.w.org\/images\/fonts\/17.7\/previews\/abeezee\/abeezee-400-normal.svg"}]}]}

And background images:

"background":{"backgroundImage":{"url":"http:\/\/localhost:8888\/wp-content\/uploads\/2024\/05\/budgie4.jpg","id":14,"source":"file","title":"budgie4"}}

This means that if the host changes, e.g., when viewing a site via a test portal as with WordPress Studio, the assets will return 404s.

What is your proposed solution?

Remove the host and save relative paths, e.g.,

"background":{"backgroundImage":{"url":"file:.\/wp-content\/uploads\/2024\/05\/budgie4.jpg","id":14,"source":"file","title":"budgie4"}}

Related:

cc @andrewserong @noisysocks @autumnfjeld

@noisysocks
Copy link
Member

cc. @creativecoder for a Font Library expert's perspective but this sounds good to me and will help with theme portability.

@creativecoder
Copy link
Contributor

This is a tricky one! I'm unsure of the best answer, but I can share what I learned when implementing the font library, which is based on media uploads. In the databse, uploads use:

  • Relative file paths
  • Absolute URLs

We followed the same conventions for wp_font_face posts.

I fear changing some urls be relative in the database is only a bandaid that we'll regret later... I think we need a solution for site portability that addresses all urls saved to the database (media, fonts, and even site/home urls).


"background":{"backgroundImage":{"url":"file:.\/wp-content\/uploads\/2024\/05\/budgie4.jpg","id":14,"source":"file","title":"budgie4"}}

Related to the discussions in this issue, in the context of theme.json, I think we should probably keep file:./ as a placeholder for only for the theme path, if possible, rather than expanding it's use to include other path replacements.

@ramonjd
Copy link
Member Author

ramonjd commented Jun 3, 2024

I fear changing some urls be relative in the database is only a bandaid that we'll regret later... I think we need a solution for site portability that addresses all urls saved to the database (media, fonts, and even site/home urls).

Thanks for the feedback @creativecoder

There are some update/migration plugins that handle a site 'domain change' scenario as well I see, i.e., updating all instances of a URL with another in the database. Maybe WordPress needs such a tool 🤷🏻

@threadi
Copy link

threadi commented Jul 20, 2024

I just stumbled across it during a project. I set up and prepared the project without SSL. I also installed fonts via the font library. When I then switched to SSL, the fonts were no longer loaded - http vs. https. I then found the entries in the database in which the URLs were still with http. In the end I was able to solve it using Better Search & Replace. But it's not nice for normal users. Relative paths would be better here or an adjustment of the existing URLs if the main URL of the project changes.

@eric-michel
Copy link

@creativecoder

I fear changing some urls be relative in the database is only a bandaid that we'll regret later... I think we need a solution for site portability that addresses all urls saved to the database (media, fonts, and even site/home urls).

I definitely get this concern (and think it's worth talking about making all references relative in the future), but Global Styles assets present a newer, more immediate challenge than other URLs in the database. As @ramonjd mentioned, there are existing migration tools that handle domain changes, but the way that URLs are stored in the database for Global Styles breaks those tools because the slashes are escaped.

We use WP Local and push/pull sites to/from WP Engine on a regular basis during development. These tools do an automatic search-replace on the Site Address when transferring the database, but font file URLs are not properly replaced during this process because of the escaping \ preventing the search-replace script from finding the full domain name including the https://.

We love the ease of installing fonts via Global Styles, but we're abandoning it in favor of the older theme.json implementation since that is a more durable option for how we develop/deploy sites. Relative references would allow us to use the Global Styles interface to easily install fonts. Either that, or the removal of the escaping \.

@ramonjd
Copy link
Member Author

ramonjd commented Sep 19, 2024

Global Styles assets present a newer, more immediate challenge than other URLs in the database.

Yeah, and here we're explicitly talking about the contents of a global styles CPT.

At present, WordPress will export a theme with absolute URLs if any fonts/background images have been added to global styles by a user.

This is perhaps where we could start: by doing a search and replace on the theme.json assets output before we bundle it.

This would ensure portability, which is one of the promises of theme.json.

_wp_get_attachment_relative_path() could come in handy in that case.

The same functionality could then be tested for the other case — ensuring that styles are portable across hosts — however with a manual trigger, or perhaps some frontend monitoring that compares hosts and would prompt the admin to "update" the current CPT.

All this is highly speculative of course. However, if we try something out for the exporting side at least, maybe it'll spark ideas for a more robust solution.

@eric-michel
Copy link

All this is highly speculative of course. However, if we try something out for the exporting side at least, maybe it'll spark ideas for a more robust solution.

We're definitely supportive of anything that will restore the functionality of long-standing migration tools that have existed for years as a priority. As you mentioned, only URLs stored in the global styles CPT are currently affected, but that prevents us from using a variety of features unless we're ok with complicating our workflow.

@bacoords
Copy link
Contributor

+1 to this issue being solved. When migrating between environments, font URLs are not updated. When running wp search-reaplce to clean up URLs, font URLs are not updated.

I'm not sure the best solution here (regenerating the URLs on a cache clear?), but relying on tricky moves to clean up slashed URLs for common assets like fonts feels hacky.

@ramonjd
Copy link
Member Author

ramonjd commented Oct 24, 2024

Thanks for keeping this one alive, folks.

I guess it'd be not much of a stretch to create a helper class to do string replace and update on wp_global_styles post type records (and, optionally, their revisions).

Theme.json could also contain absolute URLs to external hosts, not just the new site host.

Therefore, I'm thinking it might be safer to specify a path to search for and also the replace value.

Some_Class->some_method( $theme_name, $old_path, $new_path )

I imagine upload paths could change in site migrations too? So it should be able to handle hosts, e.g., get_site_url() and longer paths e.g., wp_upload_dir()['baseurl']

It could also be a user-facing tool in the admin. Making it user friendly is another question 😄

@ramonjd
Copy link
Member Author

ramonjd commented Oct 24, 2024

On a related note, I think it would be rather convenient to have absolute paths to uploaded files converted to theme assets when exporting a theme from the site editor:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants