-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix handling of uploading of font files #6407
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
30225fc
78fc27b
1617d81
48339ed
b583c93
50e91f4
abc2610
c4b4eb5
5c4039b
fe6d3b9
41ec0e8
86612b6
8d9e9d8
d391259
4bad2e3
076854a
d8c1cba
f5b0d01
0a21712
95bb189
4908744
22b071c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2871,6 +2871,9 @@ function _wp_check_existing_file_names( $filename, $files ) { | |
| * @param null|string $deprecated Never used. Set to null. | ||
| * @param string $bits File content | ||
| * @param string|null $time Optional. Time formatted in 'yyyy/mm'. Default null. | ||
| * @param string[] $upload_dir Optional. Array of the uploads directory data as returned by | ||
| * wp_upload_dir() or wp_font_dir(). If not set | ||
| * the function will use wp_upload_dir(). | ||
| * @return array { | ||
| * Information about the newly-uploaded file. | ||
| * | ||
|
|
@@ -2880,7 +2883,7 @@ function _wp_check_existing_file_names( $filename, $files ) { | |
| * @type string|false $error Error message, if there has been an error. | ||
| * } | ||
| */ | ||
| function wp_upload_bits( $name, $deprecated, $bits, $time = null ) { | ||
| function wp_upload_bits( $name, $deprecated, $bits, $time = null, $upload_dir = array() ) { | ||
| if ( ! empty( $deprecated ) ) { | ||
| _deprecated_argument( __FUNCTION__, '2.0.0' ); | ||
| } | ||
|
|
@@ -2894,10 +2897,12 @@ function wp_upload_bits( $name, $deprecated, $bits, $time = null ) { | |
| return array( 'error' => __( 'Sorry, you are not allowed to upload this file type.' ) ); | ||
| } | ||
|
|
||
| $upload = wp_upload_dir( $time ); | ||
| if ( empty( $upload_dir ) ) { | ||
| $upload_dir = wp_upload_dir( $time ); | ||
| } | ||
|
|
||
| if ( false !== $upload['error'] ) { | ||
| return $upload; | ||
| if ( false !== $upload_dir['error'] ) { | ||
| return $upload_dir; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -2919,18 +2924,18 @@ function wp_upload_bits( $name, $deprecated, $bits, $time = null ) { | |
| ) | ||
| ); | ||
| if ( ! is_array( $upload_bits_error ) ) { | ||
| $upload['error'] = $upload_bits_error; | ||
| return $upload; | ||
| $upload_dir['error'] = $upload_bits_error; | ||
| return $upload_dir; | ||
| } | ||
|
|
||
| $filename = wp_unique_filename( $upload['path'], $name ); | ||
| $filename = wp_unique_filename( $upload_dir['path'], $name ); | ||
|
|
||
| $new_file = $upload['path'] . "/$filename"; | ||
| $new_file = $upload_dir['path'] . "/$filename"; | ||
| if ( ! wp_mkdir_p( dirname( $new_file ) ) ) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function attempts to create the directory but it doesn't appear that Given these changes allow developers to define the uploads dir, I think it should either:
If the second approach is taken then the override docs will need to include a note that the directory needs to be created before attempting an upload.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Yes, you're right, there is some repetition in On the other hand Frankly I'm a bit unsure which fix would be better here.
Which do you think is better?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Same to be 100% honest, I could go either way too. If the upload handlers attempt to create the directory then it seems redundant that Let's sleep on it and hopefully our unconscious can decide for us overnight.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Great idea! Unfortunately my subconscious did not take that opportunity :) Joking aside, I tend to very slightly prefer letting plugins handle creation and confirmation of the uploads directory. That would give them a little more freedom in how to do it and whether and how to cache it. Of course they can just use Also tried to figure out if there are any differences between PHPs |
||
| if ( str_starts_with( $upload['basedir'], ABSPATH ) ) { | ||
| $error_path = str_replace( ABSPATH, '', $upload['basedir'] ) . $upload['subdir']; | ||
| if ( str_starts_with( $upload_dir['basedir'], ABSPATH ) ) { | ||
| $error_path = str_replace( ABSPATH, '', $upload_dir['basedir'] ) . $upload_dir['subdir']; | ||
| } else { | ||
| $error_path = wp_basename( $upload['basedir'] ) . $upload['subdir']; | ||
| $error_path = wp_basename( $upload_dir['basedir'] ) . $upload_dir['subdir']; | ||
| } | ||
|
|
||
| $message = sprintf( | ||
|
|
@@ -2961,7 +2966,7 @@ function wp_upload_bits( $name, $deprecated, $bits, $time = null ) { | |
| clearstatcache(); | ||
|
|
||
| // Compute the URL. | ||
| $url = $upload['url'] . "/$filename"; | ||
| $url = $upload_dir['url'] . "/$filename"; | ||
|
|
||
| if ( is_multisite() ) { | ||
| clean_dirsize_cache( $new_file ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -856,15 +856,18 @@ protected function sanitize_src( $value ) { | |
| */ | ||
| protected function handle_font_file_upload( $file ) { | ||
| add_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) ); | ||
| // Filter the upload directory to return the fonts directory. | ||
| add_filter( 'upload_dir', '_wp_filter_font_directory' ); | ||
|
Comment on lines
-859
to
-860
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the back-compat break I am concerned about. If this approach was desired an enhancement should have been opened in June, 2023 WordPress/gutenberg#52704 (comment)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking we have to "agree to disagree" here :) |
||
|
|
||
| // This will create the /fonts directory if it doesn't exist. | ||
| $font_dir = wp_font_dir(); | ||
|
|
||
| $overrides = array( | ||
| 'upload_error_handler' => array( $this, 'handle_font_file_upload_error' ), | ||
| // Not testing a form submission. | ||
| 'test_form' => false, | ||
| // Only allow uploading font files for this request. | ||
| 'mimes' => WP_Font_Utils::get_allowed_font_mime_types(), | ||
| // Upload to the /fonts directory | ||
| 'upload_dir' => $font_dir, | ||
| ); | ||
|
|
||
| // Bypasses is_uploaded_file() when running unit tests. | ||
|
|
@@ -874,7 +877,6 @@ protected function handle_font_file_upload( $file ) { | |
|
|
||
| $uploaded_file = wp_handle_upload( $file, $overrides ); | ||
|
|
||
| remove_filter( 'upload_dir', '_wp_filter_font_directory' ); | ||
| remove_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) ); | ||
|
|
||
| return $uploaded_file; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.