Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#33265 closed enhancement (fixed)

Normalize drive letter in paths

Reported by: rarst's profile Rarst Owned by: dd32's profile dd32
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.3
Component: Filesystem API Keywords: has-patch commit
Focuses: Cc:

Description

Windows platform mostly treats paths as case-insensitive. However WordPress tends to use case-sensitive comparisons for paths.

Introduction of wp_normalize_path() gave an API way to deal with slashes consistently.

Another issue it could handle is consistent case of drive letter.

Due to varied possible sources of paths (PHP, web server, user configuration) mismatch in case of drive letter is relatively common (anecdotally I have encountered it multiple times in different contexts), ruins case-sensitive paths comparisons, and is hard to notice and debug.

I would suggest to uppercase drive letter in wp_normalize_path().

Upper case would be consistent with PHP constants (such as __FILE__) and historical Windows conventions (as far as I am aware there is no formal rule which is "correct" case).

Attachments (3)

33265.diff (1.7 KB) - added by dd32 9 years ago.
33265-test.diff (1.1 KB) - added by tyxla 9 years ago.
Adding tests for wp_normalize_path(): general and uppercase drive letter cases included.
33265.2.diff (958 bytes) - added by tyxla 9 years ago.
Improved @dd32's patch by making the drive letter capitalization shorter and more efficient.

Download all attachments as: .zip

Change History (13)

#1 @dd32
9 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to 4.4

I'm of two minds here

  • All paths are case-insensitive on windows systems, only upper-casing the drive letter seems like altering 1 character of many
  • Drive letters are uppercase by convention, paths are usually lower-case (unless a system path), leaving the drive letter as the only character that needs upper-casing.

Due to the PHP constants always returning uppercase drive letters, I'm inclined to go that way through.

33265.diff implements that, still not 100% sure which way though.

@dd32
9 years ago

This ticket was mentioned in Slack in #core by sergey. View the logs.


9 years ago

#3 @SergeyBiryukov
9 years ago

  • Component changed from General to Filesystem API
  • Keywords has-patch commit added
  • Owner set to dd32
  • Status changed from new to assigned

33265.diff looks good to me.

#4 @SergeyBiryukov
9 years ago

  • Keywords needs-unit-tests added

#5 @tyxla
9 years ago

Looks like this is a great moment to add general tests for wp_normalize_path() in addition to a test for the uppercase drive letters. Patch with tests coming up.

@tyxla
9 years ago

Adding tests for wp_normalize_path(): general and uppercase drive letter cases included.

#6 @tyxla
9 years ago

  • Keywords 2nd-opinion needs-unit-tests removed

And by the way, I suggest using ucfirst() for capitalizing the drive letter. It seems to be a more efficient and short way to do that IMHO. Patch coming up for that as well.

Last edited 9 years ago by tyxla (previous) (diff)

@tyxla
9 years ago

Improved @dd32's patch by making the drive letter capitalization shorter and more efficient.

#7 @dd32
9 years ago

  • Status changed from assigned to accepted

@tyxla Good calls!

#8 @dd32
9 years ago

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

In 34104:

When running on windows systems, normalise the capitalisation of the drive letter for more reliable string comparisons.

Props tyxla
Fixes #33265

#9 @bobbingwide
9 years ago

Could be even quicker if you don't bother to check for the colon.
just do ucfirst()

Assumption is that on nix systems the first letter is a '/' which doesn't get changed.

Are there cases where the $path does validly start with a letter that's not a drive letter
and should therefore not be uppercased.

#10 @dd32
9 years ago

While it's a good suggestion, I'm personally worried that people will be passing relative-paths somewhere, for example, $file = ABSPATH . wp_normalize_path( $file );

Note: See TracTickets for help on using tickets.