WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 12 months ago

Last modified 12 months ago

#22267 closed enhancement (fixed)

Make trailingslashit() and untrailingslashit() work with backslashes

Reported by: knutsp Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version: 2.2
Component: Filesystem API Keywords: has-patch commit needs-unit-tests
Focuses: Cc:

Description

#20778 demonstrates that untrailinhslashit(), hence trailingslashit(), are unable to remove a trailing backslash from file paths on Windows.

As Windows accepts a slash as directory separator, DIRECTORY_SEPARATOR is not needed to make portable code, except when parsing or exploding a path that comes from the system http://alanhogan.com/tips/php/directory-separator-not-necessary. We neeed to tell trailingslashit() and untrailingslashit() that we want to remove the trailing slash a file path from system, not a generated one and not a URL path.

My idea is:

function untrailingslashit( $string, $is_path = false ) {
    if ( $is_path )
        return rtrim( $string, DIRECTORY_SEPARATOR . '/' );
    else
        return rtrim( $string, '/');
}

function trailingslashit( $string, $is_path = false ) {
    if ( $is_path )
        return untrailingslashit( $string, $is_path ) . DIRECTORY_SEPARATOR;
    else
        return untrailingslashit( $string ) . '/';
}

Expected results that is true:

  trailingslashit( '/mypath'  ) == '/mypath/'
  trailingslashit( '/mypath/' ) == '/mypath/'
  trailingslashit( '/mypath//') == '/mypath/'
untrailingslashit( '/mypath/' ) == '/mypath'
untrailingslashit( '/mypath//') == '/mypath'
  trailingslashit( '/usr/tmp/', true ) == '/usr/tmp/' // On non-Windows
  trailingslashit( '/usr/tmp',  true ) == '/usr/tmp/' // On non-Windows
  trailingslashit( '/usr/tmp//',true ) == '/usr/tmp/' // On non-Windows
  trailingslashit( 'C:\TEMP',   true ) == 'C:\TEMP\'  // On Windows
  trailingslashit( 'C://TEMP//',true ) == 'C://TEMP\' // On Windows
  trailingslashit( 'C:/TEMP/',  true ) == 'C:/TEMP\'  // On Windows
untrailingslashit( 'C:\TEMP\',  true ) == 'C:\TEMP'   // On Windows

On Windows, for making a path to $subdir one may use either

$path = trailingslashit( $path, true ) . $subdir;
$path = untrailingslashit( $path, true ) . '/' . $subdir;

according to preference in the situation, knowing that '/' is a valid separator in any case.

The added DIRECTORY_SEPARATOR in trailingslashit() code suggestion above could be '/', but it might produce an unexpected result, some someone when deliberately wants to make a true native path for the system.

The second parameter to rtrim() in untrailingslashit() should instruct rtrim() to remove both characters, as we cannot assume a Windows file path is always delimited or trailed with "\".

I will make the patch, and will test the all the expected results on Windows and Linux. That is, if I am not convinced this is no way to go (adding optional parameters and "messing" with an old formatting function).

Alternatives

We could make untrailingslashpath() and trailingslashpath() which assumes a path string, and no extra argument. This would require, at least softly, a replace all over the code, where a path is to be (un)trailingslashed.

We could make untrailingbackslashit() and trailingbackslashit() instead, but this would force us to check either DIRECTORY_SEPARATOR or $is_win before selecting which function to use, bloating code all over the place. A good helper function should be a real help, especially in edge cases.

Attachments (5)

22267-a.patch (1.4 KB) - added by knutsp 2 years ago.
Patch A: Introduces optional arg, removes / and \, adds \ to win file paths
22267.b.patch (1.4 KB) - added by knutsp 2 years ago.
Patch B: Introduces optional arg, removes / and \, but adds / to win file paths as a standard
22267.c.patch (1.2 KB) - added by knutsp 2 years ago.
Patch C: Removes both / and \, adds / regardless, Changes default behaviour.
22267.c2.diff (1.1 KB) - added by knutsp 2 years ago.
Patch C: Removes both / and \, adds / regardless, Replacement is now written as "/ "
22267.d.diff (2.7 KB) - added by willmot 12 months ago.
22267.d.diff updates the docs for trailingslashit and adds 2 new unit tests to cover working with backslashes.

Download all attachments as: .zip

Change History (24)

comment:1 @knutsp2 years ago

  • Component changed from Formatting to Filesystem

Just to precise, by

On Windows, for making a path to $subdir one may use either ...

I meant when in a code block like:

$is_win = ( 'WIN' === strtoupper( substr( PHP_OS, 0, 3 ) ) )
if ( $is_win ) { // one may use either:
    $path = trailingslashit( $path, true ) . $subdir;  // this works
    $path = untrailingslashit( $path, true ) . '/' . $subdir;  // also works
}
Version 0, edited 2 years ago by knutsp (next)

comment:2 @dd322 years ago

to just pull one example out of the above:

trailingslashit( 'C://TEMP//',true ) == 'C://TEMP\' // On Windows

There's no technical reason why when trailingslashit we need to use DIRECTORY_SEPARATOR , for most intents, using / will work perfectly fine as you've pointed out.

But, I'd rather we didn't use DIRECTORY_SEPARATOR at all and standardised on / like we already have. In some parts of the code, we already do a str_replace( '\\', '/', $path) to work around windows paths being different.

There are few times when we need a native "windows style" path, and given windows will accept either form (including within the address bar in windows explorer) it seems safe to just use *nix paths throughout.

I think we could potentially also make the assumption that untrailingslashit() should always remove trailing / and \ from the given string instead of adding complexity to it.

comment:3 @knutsp2 years ago

I think we could potentially also make the assumption that untrailingslashit() should always remove trailing / and \ from the given string instead of adding complexity to it.

Yes, that was my first thought, but I didn't have the guts to propose a change to the default behaviour of a function introduced many years ago, when it's actually not broken. untrailingslashit($str) removes trailing slashes from $str and that's it. Count on it to do just that. And tralingslashit() depends on it, so likewise.

The functions may be used in plugins and themes for things that wasn't expected at the time it was introduced. The added complexity is just extending it a bit. Fine to have some (predictable and well-thought) complexity in a helper function to avoid complexity in code in general.

I'd rather we didn't use DIRECTORY_SEPARATOR at all and standardised on / like we already have.

The alternative is to use both '/' and '\' for removal in untrailingslashit(), safely assuming that there are only two possible separators.

For adding a separator in trailingslashit(), in case of $is_path is given and true, it's fine with me to standardize on '/'. My suggestion has, as I mentioned, just to do with a reasonably expected behaviour on Windows after introducing an optional argument.

@knutsp2 years ago

Patch A: Introduces optional arg, removes / and \, adds \ to win file paths

@knutsp2 years ago

Patch B: Introduces optional arg, removes / and \, but adds / to win file paths as a standard

@knutsp2 years ago

Patch C: Removes both / and \, adds / regardless, Changes default behaviour.

comment:4 @knutsp2 years ago

For next revisions of the two first patches I will replace leading space with tab and
change $is_path to $is_file_path as this is more explicit. The essential is file.

More opinions on these three alternatives (patch a,b,c) wanted.

comment:5 @scribu2 years ago

  • Summary changed from Add optional boolean argument to trailingslashit() and untrailingslashit() to Make trailingslashit() and untrailingslashit() work with backslashes

Positional boolean arguments are hard to read. You could have a $separator = '/' instead.

And, in general, when opening a ticket, describe the benefit not the solution.

comment:6 @SergeyBiryukov2 years ago

I guess 22267.c.patch might actually be acceptable. At least it doesn't break any of our current unit tests.

Perhaps explicit \\ notation would be more readable than chr( 92 ).

@knutsp2 years ago

Patch C: Removes both / and \, adds / regardless, Replacement is now written as "/
"

comment:7 @knutsp2 years ago

  • Keywords has-patch added; 2nd-opinion removed

comment:8 @dd322 years ago

I'd like to revert [22331] when this ticket is fixed.

comment:9 @Ov3rfly2 years ago

  • Cc Ov3rfly added

comment:10 @SergeyBiryukov16 months ago

  • Keywords commit 3.9-early added
  • Milestone changed from Awaiting Review to Future Release

Just tested 22267.c2.diff to confirm that it would have prevented #26091. Looks good to me.

comment:11 @knutsp13 months ago

Milestone 3.9?

comment:12 @samuelsidler13 months ago

  • Milestone changed from Future Release to 3.9

comment:13 @nacin13 months ago

  • Keywords needs-unit-tests added; 3.9-early removed

The docs for trailingslashit() should also be updated. I'm OK with this but let's get some new unit tests in here.

@willmot12 months ago

22267.d.diff updates the docs for trailingslashit and adds 2 new unit tests to cover working with backslashes.

comment:14 @willmot12 months ago

I've added some unit tests in 22267.d.diff, apparently I have to add a comment as well as uploading the file so that people get notified...

comment:15 @nacin12 months ago

Those tests look great, willmot. It might be good to have an additional test case for mixed forward and back slashes.

comment:16 @willmot12 months ago

Good call, I'll get that added tomorrow.

comment:17 @nacin12 months ago

Whether used for paths or URLs, this should be OK. I'm going to go ahead and commit this. But, be prepared for it to be reverted during 3.9 beta if it breaks something we didn't think about.

comment:18 @nacin12 months ago

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

In 27344:

Strip backslashes, not just forward slashes, from untrailingslashit().

trailingslashit() will now remove any forward or backslashes from the end of a string before appending a forward slash.

props knutsp, willmot.
fixes #22267.

comment:19 @nacin12 months ago

If not clear, [27344] reverts [22331].

Note: See TracTickets for help on using tickets.