Font Library: return null if a font collection is not registered

[matiasbenedetto]

What?

Font Library: return null if a font collection is not registered

Why?

• improves code quality
• address feedback from: Font Library: Add all REST API endpoints. wordpress-develop#6034 (comment)

How?

returning null when a font collection is not registered

Testing Instructions

• Run unit tests

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core SVN

Core Committers: Use this line as a base for the props when committing in SVN:

Props mmaattiiaass, grantmkin, youknowriad.

GitHub Merge commits

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>
Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/compat/wordpress-6.5/fonts/class-wp-font-library.php
❔ lib/compat/wordpress-6.5/fonts/class-wp-rest-font-collections-controller.php
❔ phpunit/tests/fonts/font-library/wpRestFontCollectionsController.php

[creativecoder]

This looks good to me. I see other Core functions vary on whether they return null or false in this case, but I think returning null seems most appropriate, as you tried to get something, but there's nothing there.

I see that phpunit tests are failing, I think because the WP_Font_Library class has already been committed to WordPress trunk, that class is overriding here, and it doesn't yet have the updated method that returns null? @youknowriad @getdave does that seem right? Any advice on how to get CI passing?

[youknowriad]

I see that phpunit tests are failing, I think because the WP_Font_Library class has already been committed to WordPress trunk, that class is overriding here, and it doesn't yet have the updated method that returns null?

I think at this point, we could consider removing the phpunit tests from Gutenberg, because from now on, they will be maintained on Core. That's how we approached this kind of things in the past.

[matiasbenedetto]

A PR removing the tests from the already merged classes: #58752 that we would need to merge before this one.

[creativecoder]

Do we need to remove all tests right now?

Gutenberg supports the current and previous release of WordPress, so having tests that cover the 6.5 compat code seems to have some value until 6.4 isn't supported any longer.

What do you think of just removing that one test, for now, until WP trunk is in sync with these changes?

[youknowriad]

Gutenberg supports the current and previous release of WordPress, so having tests that cover the 6.5 compat code seems to have some value until 6.4 isn't supported any longer.

you're right, I think the compat tests should stay in Gutenberg because the code they cover will remain in Gutenberg but for the classes that moved to Core, we should remove their tests.

[matiasbenedetto]

What do you think of just removing that one test, for now, until WP trunk is in sync with these changes?

Removing just some tests makes things difficult to track and could give us confidence that things are being appropriately tested when they could not.

you're right, I think the compat tests should stay in Gutenberg because the code they cover will remain in Gutenberg

In that case we would we need to find a way to run the tests loading the classes tested from Gutenberg and not from core. I'm not sure how to do that.

[github-actions]

Flaky tests detected in 3b338b1aaf886a50c21f46e30f8296cd23063865.
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/7805690881
📝 Reported issues:

• [Flaky Test] aria-disabled is correctly hydrated for different values #54370 in /test/e2e/specs/interactivity/directive-bind.spec.ts

[matiasbenedetto]

In that case we would we need to find a way to run the tests loading the classes tested from Gutenberg and not from core. I'm not sure how to do that.

Should we add a _Gutenberg suffix to the name of the classes that were already merged to core to be able to test them? Example: WP_Font_Library_Gutenberg

[creativecoder]

I think the compat tests should stay in Gutenberg because the code they cover will remain in Gutenberg

I think @youknowriad might be referring to font code and tests that aren't being ported to Core. The only thing I'm aware of are the fonts bc-layer code, as well as gutenberg_convert_legacy_font_family_format and the related test file. Doesn't look like those are being removed, so this shouldn't be a problem.

[youknowriad]

@coderkevin Yes, I was indeed referring to these files :)

Now for testing the ones that are already on Core, to be honest I don't see a value. What would be good would be to check that they're kept in sync with core classes but we don't have any of that so far and it's not specific to font library.

[creativecoder]

I've merged #58752 and have rebased this branch, waiting for CI to pass.

[creativecoder]

There's still some weirdness here, even with removing the phpunit tests already committed to Core.

WP_Font_Library::get_font_collection returns WP_Error on current WP trunk, but null here. We need to account for both in the controller, since unit tests are run against WP trunk.

For now I've added a check for a WP_Error instance in the controller, which we should remove once WP trunk is updated with the code in this PR.

[creativecoder]

CI is finally passing, so this is now ready to merge.