WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.27k stars 4.09k forks source link

Theme.json: create a consistent syntax for relative paths #61804

Open ramonjd opened 3 months ago

ramonjd commented 3 months ago

What problem does this address?

A convention was introduced as part of the web fonts feature, with regards to relative paths to theme assets in theme.json.

The convention is that a prefix of file:./ is required to indicate that a path is relative to the theme root directory.

For example, the following path —

"src": "file:./assets/some/file.ext"

— assumes that /assets/ is top-level directory of the theme.

This rule also applies to style variations, so theme.json files in the /styles folder.

This convention was followed for background image URIs.

See the discussions in:

What is your proposed solution?

As @noisysocks points out:

Relative paths using . as a prefix is misleading because the paths are actually relative to the theme directory and not to where the theme.json file is. It also implies that .. should work which it doesn't. We should make the paths actually relative or remove support for them. This is an issue for fonts as well.

I would propose either:

  1. Communicate that all paths, regardless of the depth, must be relative to the theme root, and perhaps also support "file:" (without dotslash to avoid confusion) by default and have backwards compat handling for "file:./"
  2. Allow paths that are relative to current directory, and then in resolve them in the background with PHP trickery with realpath or something.

As @creativecoder notes, we should update the docs to reflect the current behaviour:

https://developer.wordpress.org/themes/global-settings-and-styles/settings/typography/#registering-web-fonts-font-faces

For example:

The src property is unique in that it allows you to reference a URL that is relative to the theme's root directory, regardless of where the theme.json resides. For example, to reference a font bundled with your theme, you would use the "file:./path/to/file.ext" format in both the theme's main theme.json or in theme variation JSON files in the /styles directory.

ramonjd commented 3 months ago

Also, should we consider only saving relative paths to the database? This applies to fonts as well:

noisysocks commented 3 months ago

I'm not sure what the best solution is given we've put ourselves into a corner re. backwards compatibility 😅

Actually supporting relative paths seems problematic since it implies that .. would work which it doesn't, and I'm not sure that we ever should support .. since we don't want to allow accessing files outside of the theme directory.

So I suppose best to either keep things as they are and document it, or change the prefix to be file: or file:/ instead of file:./.

ramonjd commented 3 months ago

I'm not sure what the best solution is given we've put ourselves into a corner re. backwards compatibility Actually supporting relative paths seems problematic since it implies that .. would work which it doesn't, and I'm not sure that we ever should support .. since we don't want to allow accessing files outside of the theme directory.

Thanks for the feedback! I agree 💯

I think I'd therefore support point 1 in the description ("Communicate that all paths, regardless of the depth, must be relative to the theme root, and perhaps also support "file:" (without dotslash to avoid confusion) by default and have backwards compat handling for "file:./"")

creativecoder commented 3 months ago
  1. Communicate that all paths, regardless of the depth, must be relative to the theme root, and perhaps also support "file:" (without dotslash to avoid confusion) by default and have backwards compat handling for "file:./"

I agree that option one seems like the best path here, at least for now. Any changes to support a new/different file syntax and maintain backcompat seem likely to add a lot of complexity, so I don't think there's enough benefit to warrant the added complexity at this time.