WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 15 months ago

Last modified 4 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 2 years ago.
Allow for multibyte filenames in attachment unit tests.
43170.diff (86.0 KB) - added by jorbin 16 months ago.
43170.2.diff (66.1 KB) - added by jorbin 16 months ago.
WIP
43170.3.diff (46.5 KB) - added by jorbin 16 months ago.

Download all attachments as: .zip

Change History (18)

@Viper007Bond
2 years ago

Allow for multibyte filenames in attachment unit tests.

#1 @Viper007Bond
2 years 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
2 years ago

  • Component changed from General to Media

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

#3 @pento
18 months ago

#43138 was marked as a duplicate.

#4 @jorbin
16 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
16 months ago

#5 @desrosj
16 months ago

  • Keywords early added

#6 @joemcgill
16 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
16 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
16 months ago

WIP

@jorbin
16 months ago

#8 @jorbin
16 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.


16 months ago

#10 @jorbin
16 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.


16 months ago

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


15 months ago

#13 @jorbin
15 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.