Make trailingslashit() and untrailingslashit() work with backslashes

[knutsp]

#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.

[knutsp]

Just to be 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
}

[dd32]

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.

[knutsp]

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.

[knutsp]

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

[knutsp]

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

[knutsp]

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

[knutsp]

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.

[scribu]

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.

[SergeyBiryukov]

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 ).

[knutsp]

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

[dd32]

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

[SergeyBiryukov]

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

[knutsp]

Milestone 3.9?

[nacin]

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

[willmot]

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

[willmot]

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...

[nacin]

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

[willmot]

Good call, I'll get that added tomorrow.

[nacin]

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.

[nacin]

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.

[nacin]

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