Make WordPress Core

Opened 3 weeks ago

Last modified 5 days ago

#60835 new defect (bug)

Fix and improve handling of uploading of font files

Reported by: azaozz's profile azaozz Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Upload Keywords: has-patch dev-feedback needs-testing
Focuses: Cc:

Description

Follow up to #60652 and [57868].

As discussed at https://github.com/WordPress/wordpress-develop/pull/6211 the code that enables uploading of font files by replacing the output of wp_handle_upload() by using the upload_dir filter is not the best solution. In addition it implements nested filters that can be pretty confusing so understand and/or use for plugin and theme authors.

A more straightforward solution is needed for this and similar cases in the future.

Change History (7)

#1 @azaozz
3 weeks ago

Given the constraint of "Do not modifying wp_handle_upload()", the current code in WP 6.5 makes sense. However there is no such constrain when implementing new features in core, hence the current code can be (and imho should be) improved to implement that functionality in a better way. This will also serve as an example of now to resolve similar cases in the future (without imposing such constrains).

#2 @azaozz
3 weeks ago

  • Keywords needs-patch added
  • Summary changed from FIx and improve handling of uploading font files to Fix and improve handling of uploading of font files

#3 follow-up: @peterwilsoncc
3 weeks ago

[57868] may need a follow up but any changes must retain protections against an infinite loop without the need for closures to avoid the risks of future maintainers and extenders introducing bugs. Tests_Fonts_WpFontDir::test_fonts_dir_filters_do_not_trigger_infinite_loop() will need to be retained in some form to ensure against this.

#4 in reply to: ↑ 3 @azaozz
3 weeks ago

Replying to peterwilsoncc:

any changes must retain protections against an infinite loop without the need for closures to avoid the risks of future maintainers and extenders introducing bugs

Yes, definitely. Seems that the nested filters can be avoided if wp_handle_upload() and/or wp_get_upload_dir() are updated/modified. That would completely remove the possibility for an infinite loop caused by using these filters.

This ticket was mentioned in PR #6383 on WordPress/wordpress-develop by @khokansardar.


5 days ago
#5

  • Keywords has-patch added; needs-patch removed

Improve fonts upload dir handling logic.

Trac ticket: #60835

#6 @khokansardar
5 days ago

  • Keywords dev-feedback needs-testing added

@peterwilsoncc commented on PR #6383:


5 days ago
#7

This reintroduces the infinite loop bug fixed in #60652 when filtering the font directory. It looks to be a wider issue than the original as the unit tests are timing out due to entering a loop so the relevant test isn't running.

$ ./vendor/bin/phpunit --filter test_fonts_dir_filters_do_not_trigger_infinite_loop
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

F                                                                   1 / 1 (100%)

Time: 00:05.709, Memory: 214.27 MB

There was 1 failure:

1) Tests_Fonts_WpFontDir::test_fonts_dir_filters_do_not_trigger_infinite_loop
Filtering the uploads directory triggered an infinite loop.

It looks to be a wider issue than the

Note: See TracTickets for help on using tickets.