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: fix infinite loop when calling wp_get_upload_dir in a function that's used to filter font_dir #58839

Merged
merged 7 commits into from
Feb 28, 2024

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Feb 8, 2024

What?

Fix infinite loop when calling wp_get_upload_dir in a function that's used to filter font_dir.
An alternative approach to: WordPress/wordpress-develop#6198

Why?

This avoids an infinite loop that can occur if wp_upload_dir() is called inside a 'font_dir' callback.
Fix: #58696

Testing instructions

  1. Use wp_get_upload_dir() inside a function used to filter upload_dir as in the following example:
 function alter_wp_fonts_dir( $defaults ) {
	$wp_upload_dir = wp_get_upload_dir();
	$uploads_basedir = $wp_upload_dir['basedir'];
	$uploads_baseurl = $wp_upload_dir['baseurl'];

	$fonts_dir = $uploads_basedir . '/fonts';
	// Generate the URL for the fonts directory from the font dir.
	$fonts_url = str_replace( $uploads_basedir, $uploads_baseurl, $fonts_dir );

	$defaults['path'] = $fonts_dir;
	$defaults['url']  = $fonts_url;

	return $defaults;
}
add_filter( 'font_dir', 'alter_wp_fonts_dir' );
  1. Upload fonts using the font library
  2. Check that the fonts were successfully uploaded to the path set by the filter.
    (wp-content/uploads/fonts in the example provided).

@matiasbenedetto matiasbenedetto marked this pull request as draft February 8, 2024 14:27
Copy link

github-actions bot commented Feb 8, 2024

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.

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: costdev <costdev@git.wordpress.org>
Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: dd32 <dd32@git.wordpress.org>
Co-authored-by: jazzsequence <jazzs3quence@git.wordpress.org>

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

@matiasbenedetto matiasbenedetto changed the title fix infinite loop when calling wp_get_upload_dir in a function thats … Font Library: fix infinite loop when calling wp_get_upload_dir in a function that's used to filter font_dir Feb 8, 2024
Copy link

github-actions bot commented Feb 8, 2024

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-rest-font-faces-controller.php
❔ lib/compat/wordpress-6.5/fonts/fonts.php

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

This works well for me, following the testing instructions to change the fonts folder to be within wp-content/uploads. I was able to install and delete fonts with font files in that directory.


One thing to note is that the very first font I installed, I saw an error (the same one we are tracking in #59023), but couldn't replicate it after the first time. That's not related to this change, so I don't think it should block this PR.

@matiasbenedetto matiasbenedetto merged commit df5dc24 into trunk Feb 28, 2024
59 checks passed
@matiasbenedetto matiasbenedetto deleted the try/fix-infinite-loop-upload-dir branch February 28, 2024 20:25
@github-actions github-actions bot added this to the Gutenberg 17.9 milestone Feb 28, 2024
@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 28, 2024
youknowriad added a commit that referenced this pull request Mar 4, 2024
…unction that's used to filter font_dir (#58839)

* fix infinite loop when calling wp_get_upload_dir in a function thats  used to filter font_dir

* remove not needed parameter

* remove filter after use

* removing not needed parameter

* Add a comment explaining the reasons behind the implementation

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>

* fix spaces

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>

---------

Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>
Co-authored-by: costdev <costdev@git.wordpress.org>
Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: dd32 <dd32@git.wordpress.org>
Co-authored-by: jazzsequence <jazzs3quence@git.wordpress.org>
@youknowriad
Copy link
Contributor

I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: b5f2b33

@youknowriad youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Mar 4, 2024
youknowriad added a commit that referenced this pull request Mar 4, 2024
…unction that's used to filter font_dir (#58839)

* fix infinite loop when calling wp_get_upload_dir in a function thats  used to filter font_dir

* remove not needed parameter

* remove filter after use

* removing not needed parameter

* Add a comment explaining the reasons behind the implementation

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>

* fix spaces

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>

---------

Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>
Co-authored-by: costdev <costdev@git.wordpress.org>
Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: dd32 <dd32@git.wordpress.org>
Co-authored-by: jazzsequence <jazzs3quence@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Font Library [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Font Library: "Fetch error: The response is not a valid JSON response" when filtering font_dir.
4 participants