[wp-trac] [WordPress Trac] #60835: Fix and improve handling of uploading of font files

[WordPress Trac]

#60835: Fix and improve handling of uploading of font files
----------------------------------------------------+---------------------
 Reporter:  azaozz                                  |       Owner:  (none)
     Type:  defect (bug)                            |      Status:  new
 Priority:  high                                    |   Milestone:  6.6
Component:  Upload                                  |     Version:  6.5
 Severity:  normal                                  |  Resolution:
 Keywords:  needs-testing has-patch has-unit-tests  |     Focuses:
----------------------------------------------------+---------------------

Comment (by peterwilsoncc):

 > Can you openly and honestly say why it is sooo important that this code
 remains unfixed, and what will WP gain by keeping it unfixed?

 I think the code can be improved (it's not buggy, in that font uploads
 work as expected), I believe I said as much on the original PR.

 Pascal's comment on the original PR was that a make post was needed to
 discuss how such directories were to be handled in the future. This is to
 allow for a general approach rather than a font specific approach and why
 I asked for it in my first comment. (Pascal is on leave at the moment,
 thus no mention.)

 > Not sure about you but I think this is a really bad advice... Basically
 you're saying "do not attempt to make WP better!", isn't it?

 This is why I have been reviewing the PR and requesting a different
 approach. I don't think your PR is an improvement as it introduces a new
 API that allows the `upload_dir` filter to be bypassed by plugin authors.
 This introduces the very issue to another filter you are trying to avoid.

 I've marked this as a enhancement (it refactors code), Jorbin's moved it
 off the milestone, Joe has requested clarity and Olga (triage lead) has
 also moved it off the milestone.

 I (and I presume others) are trying to work with you to improve the patch
 but hitting roadblocks again and again.

 > Do you think the changes in 60835.diff are more "inappropriate" than the
 changes in [57868]? Could you please review 60835.diff and if you find it
 "inappropriate" share here what exactly is inappropriate about it?

 The patch introduces code to `remove_filter()` targeting a specific filter
 added and removed by core functions. At a glance it is clear this would
 cause problems for `wp_font_dir()`. At a fundamental level introducing
 such code to the plugins API is overreach regardless of the filter.

 It's not really my responsibility to write tests for your patches but I
 have done so in [https://github.com/WordPress/wordpress-develop/pull/6995
 PR#6995]. The tests should probably be committed as part of test
 improvements for 6.7.

 > I can't tell what the purpose of this PR is anymore, is it just to fix a
 potential filter issue and not actually change the core functionality of
 the fonts directory location? Or does this PR ALSO change the default
 directory of fonts from the uploads directory to move it outside of the
 uploads directory again by default?

 @elrae It does not move the font directory, that will continue to the live
 in `uploads` or `uploads/site/siteid` on multisite installs. The patch
 refactors the uploads code and introduces a new API.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/60835#comment:76>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform