WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 5 months ago

#43170 closed defect (bug) (fixed)

Replace all usages of basename() with wp_basename() in order to support multibyte filenames

Reported by: Viper007Bond Owned by:
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9.2
Component: Media Keywords: has-patch early
Focuses: Cc:

Description

We still use basename() in a lot of places even though it isn't multibyte compatible out of the box. See http://php.net/basename and https://bugs.php.net/bug.php?id=62119.

Example filename where it can fail: 麻美.jpg

We should replace all uses of basename() with our own wp_basename(). This will probably need a lot of testing, so probably best to avoid one big patch and instead change calls in small chunks.

Related: #43138

Attachments (4)

43170-tests-factory-attachments.patch (1020 bytes) - added by Viper007Bond 19 months ago.
Allow for multibyte filenames in attachment unit tests.
43170.diff (86.0 KB) - added by jorbin 6 months ago.
43170.2.diff (66.1 KB) - added by jorbin 6 months ago.
WIP
43170.3.diff (46.5 KB) - added by jorbin 6 months ago.

Download all attachments as: .zip

Change History (17)

@Viper007Bond
19 months ago

Allow for multibyte filenames in attachment unit tests.

#1 @Viper007Bond
19 months ago

  • Keywords needs-patch added

The above attached patch is just one example where we need to swap it out. I discovered this when writing a unit test for one of my plugins:

https://github.com/Viper007Bond/regenerate-thumbnails/commit/08f5eabd3ff041938aab0b0d1a3a5bde4aaaaca3#diff-35c785b29ba1242d0fbf2db80deb00c8R567

I ended up having to clone self::factory()->attachment->create_upload_object() so that I didn't end up with unnamed-file.jpg.

#2 @SergeyBiryukov
19 months ago

  • Component changed from General to Media

Related: #21217, #23267, #25236, #33227.

#3 @pento
7 months ago

#43138 was marked as a duplicate.

#4 @jorbin
6 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 5.2

I might be feeling a bit YOLO, or having a little too much faith in your unit tests, but I think this is actually something we can fix in one go. The above patch changes every reference to basename to wp_basename except for 2:

1) inside the wp_basename function, we still use basename.
2) inside the unit test bootstrap, we still use basename for the config file.

Unless I hear some objections, I'm going to land this early in the 5.2 cycle (i.e. this week). It can always be reverted if it does break things.

@jorbin
6 months ago

#5 @desrosj
6 months ago

  • Keywords early added

#6 @joemcgill
6 months ago

Thanks for the suggestion, @Viper007Bond and @jorbin for the new patch, which looks good to me. The only nitpick I have is that this includes several changes to docblocks where we're referring to something like a "file basename" and changing it to "wp_basename" seems to mix implementation details with the descriptive noun "basename" we're trying to retrieve.

#7 @ocean90
6 months ago

Some quick notes on 43170.diff:

  • There should be no need to use wp_basename() for __FILE__
  • I also don't think we have to use wp_basename() when we know fore sure that no multibyte-like string is possible, for example $lang_file
  • Third-party libraries shouldn't be changed.
  • Like Joe said, documentation shouldn't be changed to "wp_basename".
  • Maybe focus only on media-related functions first?

@jorbin
6 months ago

WIP

@jorbin
6 months ago

#8 @jorbin
6 months ago

Thanks for the feedback.

https://core.trac.wordpress.org/attachment/ticket/43170/43170.3.diff is much smaller and primarily focuses on media. I'm going to do another pass on the tests since I likely still have some changes in there that I need to not changes, but this is getting closer.

This ticket was mentioned in Slack in #core-committers by jorbin. View the logs.


6 months ago

#10 @jorbin
6 months ago

In 44785:

Replace usages of basename() with wp_basename() in order to support multibyte filenames

This is focused on the pieces of code that touch media files and the tests that support them. basename isn't multibyte compatible out of the box. See http://php.net/basename and https://bugs.php.net/bug.php?id=62119.

See #43170.
Props Viper007Bond.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


5 months ago

This ticket was mentioned in Slack in #core-media by killua99. View the logs.


5 months ago

#13 @jorbin
5 months ago

  • Resolution set to fixed
  • Status changed from new to closed

Based on the lack of bug reports, I'm closing this as fixed for 5.2

Note: See TracTickets for help on using tickets.