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

[CMSP-695] Filter WP font_dir #29

Merged
merged 61 commits into from
Feb 13, 2024
Merged

Conversation

jazzsequence
Copy link
Contributor

@jazzsequence jazzsequence commented Feb 6, 2024

This PR adds our own handling for the WP Font Library.

This is designed to filter the font_dir (added in WP 6.5 / Gutenberg 17.6.0) to use an alternate path by default that, on Pantheon, is a writeable filesystem (meaning they can install fonts on production without having to commit them to the codebase). This behavior can be overridden by filtering the pantheon_modify_fonts_dir value.

Note that this is blocked by WordPress/gutenberg#58696. Behaviorally speaking, while the tests pass and the font directory is filtered appropriately, testing manually returns an invalid JSON response error (see Loom).

That doesn't mean that we can't merge this PR (we might decide we don't want to, or default the pantheon_modify_fonts_dir to false until WordPress/gutenberg#58696 is fixed). A version_compare determines whether to even load the namespace handling our WP Font Library modifications, so anyone using < WP 6.5 will remain unaffected even if we merge this and do a release anyway.

An approach to add our own filter to define the font directory was considered and decided against, since the user could just use the core font_dir filter. Instead, I made our filter fire earlier (priority 9) so a default font_dir filter will override our filter.

We're storing the value of wp_get_upload_dir into a global ($_pantheon_upload_dir) so we do not create an infinite loop caused by filtering inside of a filter (font_dir and upload_dir), see WordPress/gutenberg#58696 (comment).

TODO: Documentation for the new filter (and the WP filter) and descriptions of the behavior on Pantheon's environment. This will be done in a PR against the docs repo separately.

we're loading after pantheon-updates because that's where the get_current_wordpress_version helper is defined
this allows us to test things that are in gutenberg but haven't been merged into core yet
check wp version and then maybe pull font library out of gutenberg
we don't need a filter if users can just filter it themselves.
this means that if a customer adds the filter with the default priority, it will override our modification.
@jazzsequence jazzsequence requested a review from a team as a code owner February 6, 2024 18:19
@jazzsequence jazzsequence self-assigned this Feb 6, 2024
@pwtyler
Copy link
Member

pwtyler commented Feb 6, 2024

That doesn't mean that we can't merge this PR (we might decide we don't want to, or default the pantheon_modify_fonts_dir to false until WordPress/gutenberg#58696 is fixed)

With enough decisions at play here, will flip this to draft for now.

A version_compare determines whether to even load the namespace handling our WP Font Library modifications, so anyone using < WP 6.5 will remain unaffected even if we merge this and do a release anyway.

I considered for a minute if we need a means to support this for users using newest versions of g'berg on Core versions <6.4 but I think that can be documented with alternative means instead, if at all.

@pwtyler pwtyler marked this pull request as draft February 6, 2024 18:24
@jazzsequence jazzsequence marked this pull request as ready for review February 8, 2024 18:07
Copy link
Member

@pwtyler pwtyler left a comment

Choose a reason for hiding this comment

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

  • Lots of repetition could be cleaned up
  • fonts.php I think could be simplified but maybe I'm wrong

bin/install-local-tests.sh Outdated Show resolved Hide resolved
bin/phpunit-test.sh Outdated Show resolved Hide resolved
bin/phpunit-test.sh Outdated Show resolved Hide resolved
inc/fonts.php Outdated Show resolved Hide resolved
tests/phpunit/test-fonts.php Outdated Show resolved Hide resolved
tests/phpunit/test-fonts.php Show resolved Hide resolved
tests/phpunit/test-fonts.php Outdated Show resolved Hide resolved
inc/fonts.php Outdated Show resolved Hide resolved
@jazzsequence
Copy link
Contributor Author

Tests are passing on the add-more-helpers branch of wpunit-helpers. Once that's merged, this will be ready for re-review.

@jazzsequence jazzsequence requested a review from pwtyler February 12, 2024 16:26
@jazzsequence jazzsequence merged commit 7d1d919 into main Feb 13, 2024
7 checks passed
@jazzsequence jazzsequence deleted the cmsp-695-filter-wp-font-dir branch February 13, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants