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

Font Library: load collection JSON data from a URL in the collection config #54067

Merged
merged 71 commits into from
Sep 22, 2023

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Aug 30, 2023

What?

Font Library: load collection JSON data from a URL in the collection config.

Why?

To avoid loading Google Fonts collection data from a file in the repo.

How?

  • Remove the data_json_file property from the font collection config and replace it by src property.
  • src property can have a value of a URL or a file path.
  • In both cases, if src is a URLor a file path it will be read by the backend and the data will be sent to the client.

Closes #53992

Testing instructions:

Use the UI to load Google Fonts collection:
image

Depends on:

Disclaimer

We are using this URL to load the google fonts collection data. When that file is hosted in wordpress.org that URL will be replaced.

Tracking issue:

matiasbenedetto and others added 30 commits August 18, 2023 12:35
Co-authored-by: Tonya Mork <[email protected]>
Co-authored-by: Tonya Mork <[email protected]>
* Ensures each required param is of the right data type.
* Improves each param check error to include expected data type.
* Rechecking if "data_json_file" exists in the $config property shouldn't be necessary,
  as it's checked in the constructor.

* If the file does not exist, bail out immediately as there's nothing more to do.

* Adds a check and WP_Error for file_get_contents():
file_get_contents() returns false on failure.
If there's nothing in the file, an empty string is returned.
This change checks for both of these conditions and returns a WP_Error if either happens.

* Internationalizes WP_Error error message for consistency in Core.
Co-authored-by: Tonya Mork <[email protected]>
…n, remove try catch and raise the error if needed
@github-actions
Copy link

github-actions bot commented Sep 20, 2023

Flaky tests detected in a047eb7.
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/6253897039
📝 Reported issues:

@matiasbenedetto
Copy link
Contributor Author

matiasbenedetto commented Sep 20, 2023

This PR was updated with the latest trunk changes. It seems ready to review.
I think that adding one more test case for data loading from URLs is the only thing that needs to be added. ✔️ Test added.

@ironprogrammer
Copy link
Contributor

Works for me 👍🏻

Test Report

Steps to Test

  1. Open the font management dialog and select the "Install Fonts" tab.
  2. Compare the fonts listed, before and after patch.
  3. Compare the response for the default-font-collection before and after patch (.../wp-json/wp/v2/fonts/collections/default-font-collection?_locale=user).

Environment

  • OS: macOS 13.5.2
  • Browser: Safari 16.6
  • Server: nginx/1.25.2
  • PHP: 7.4.33
  • WordPress: 6.4-alpha-56267-src
  • Theme: twentytwentythree v1.2
  • Active Plugins:
    • gutenberg v16.7.0-rc.1 (compared trunk and this PR)

Actual Results

  • ✅ Fonts displayed on the "Install Fonts" tab are consistent.
  • ✅ The response data for default-font-collection is the same (used Postman to query and persist results to disk, then diffed in terminal).

@mikachan mikachan added the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 21, 2023
@anton-vlasenko anton-vlasenko self-requested a review September 21, 2023 19:56
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

Please find my code review below.

matiasbenedetto and others added 3 commits September 21, 2023 17:46
Co-authored-by: Anton Vlasenko <[email protected]>
Co-authored-by: Anton Vlasenko <[email protected]>
Co-authored-by: Anton Vlasenko <[email protected]>
@matiasbenedetto
Copy link
Contributor Author

@anton-vlasenko I committed your code changes.
Thank you for the review.

Copy link
Contributor

@pbking pbking left a comment

Choose a reason for hiding this comment

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

🚢

@matiasbenedetto matiasbenedetto merged commit 8a82a37 into trunk Sep 22, 2023
49 checks passed
@matiasbenedetto matiasbenedetto deleted the try/font-collections-url branch September 22, 2023 16:44
@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 22, 2023
mikachan pushed a commit that referenced this pull request Sep 22, 2023
…config (#54067)

* Adding Font Collection class

* php formatting and linting

* Adding tests for collections routes

* Adding tests for WP_Font_Collection constructor

* adding tests for WP_Font_Collection get_data()

* adding tests for WP_Font_Library register_font_collection()

* get font collection tests

* adding 'wp_' prefix to the 'register_font_collection' filter name

* fix callback name

* making class property private

* registering filter from font-library.php file

* removing superfluous comment

* moving files to according changes in trunk

* config without a json file should fail

* fix property name in tests

* name fix

Co-authored-by: Tonya Mork <[email protected]>

* comment update

Co-authored-by: Tonya Mork <[email protected]>

* Adds WP_Error to return type

* Improves contructor error handling.

* Ensures each required param is of the right data type.
* Improves each param check error to include expected data type.

* FontCollection::get_data(): Improves error handling

* Rechecking if "data_json_file" exists in the $config property shouldn't be necessary,
  as it's checked in the constructor.

* If the file does not exist, bail out immediately as there's nothing more to do.

* Adds a check and WP_Error for file_get_contents():
file_get_contents() returns false on failure.
If there's nothing in the file, an empty string is returned.
This change checks for both of these conditions and returns a WP_Error if either happens.

* Internationalizes WP_Error error message for consistency in Core.

* Removes empty space for wpcs

* adding filter in a simpler way

Co-authored-by: Tonya Mork <[email protected]>

* micro-optimization

Co-authored-by: Tonya Mork <[email protected]>

* reuse WP_Error response instead of creating a new one

Co-authored-by: Tonya Mork <[email protected]>

* Eliminates try/catch

* Revert "Eliminates try/catch" commit

This reverts commit 0e6c026.

* Remove wp_register_font_collection and replace it by a global function, remove try catch and raise the error if needed

* adding function comment and guard agains re-declaration

* php format

* fixing docblock coments

* removing param comment

* re-adding parameter comment removed by mistake

* format php

* updating WP_Font_Collection __construct tests

* updating WP_Font_Collection get_data tests

* updating tests

* php format

* array check

Co-authored-by: Tonya Mork <[email protected]>

* Documents config array structure

* adding tests for /fonts/collections endpoint

* adding /fonts/collections/<id> endpoint test

* format

* format

* add test for missing collection

* removing not needed variables

* Add more tests and split the test for WP_REST_Font_Library_Controller

* lint

* try to create dir for tests in CI

* renane config property to src and handle URLs apart from file paths

* Add the ability to load collection JSON data from a URL

* remove unwanted comment

* fix missing merge

* fix merge with trunk

* removing test file after merge with trunk

* removing duplicated test files after merge with trunk

* decode data to make availanle in the client as json

* update google fonts collection source

* format php'

* removing the default font collection (google fonts) json file from repo

* fixing logic to get data from url

* add tests to try the font collection fetching from url

* update test

* format comments

* replacing existing url in test with a mock url

* early return

Co-authored-by: Anton Vlasenko <[email protected]>

* remove not needed filter

Co-authored-by: Anton Vlasenko <[email protected]>

* rewording error message

Co-authored-by: Anton Vlasenko <[email protected]>

---------

Co-authored-by: Tonya Mork <[email protected]>
Co-authored-by: Anton Vlasenko <[email protected]>
@mikachan
Copy link
Member

I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: ead73c8

@mikachan mikachan removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 22, 2023
@anton-vlasenko
Copy link
Contributor

Oops, this has already been merged.
I didn’t notice this during my initial code review, but

if ( false !== strpos( $this->config['src'], 'http' ) && false !== strpos( $this->config['src'], '://' ) ) {

could be replaced with

if ( str_contains( $this->config['src'], 'http' ) && str_contains( $this->config['src'], '://' ) ) {

for better readability.
But this is more of a nitpick.

@matiasbenedetto
Copy link
Contributor Author

could be replaced with

if ( str_contains( $this->config['src'], 'http' ) && str_contains( $this->config['src'], '://' ) ) {

Thank yo so much for the new review @anton-vlasenko !

str_contains() was introduced in PHP 8, so I don't think it is the right move to use it here. https://www.php.net/manual/en/function.str-contains.php because it's not available on PHP < 8.

@ironprogrammer
Copy link
Contributor

@matiasbenedetto, this should be okay, as a polyfill for str_contains was introduced to Core in https://core.trac.wordpress.org/ticket/49652.

@matiasbenedetto
Copy link
Contributor Author

@ironprogrammer Good to know. Changed here: #54832

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Font Library: load collection data from a URL
6 participants