Add tests for path_join()

Adds unit tests for path_join in build/wp-includes/functions.php

Trac ticket: https://core.trac.wordpress.org/ticket/55897#ticket

Two things I'd like to mention about the path_join method:

1. Maybe some verification or checks should be added to make sure $base and $path are both indeed strings.
2. The ltrim for the path variable is redundant, because if the path variable starts with slashes, it will be considered absolute, thus entering the if statement and returning $path. It will only reach the ltrim( $path, '/' ) if $path is not absolute, i.e. does not start with a slash, so nothing needs to be trimmed.

[SergeyBiryukov]

In 53457:

Tests: Add unit tests for path_join().

Props karlijnbk.
See #55897.

Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/53457.

[SergeyBiryukov]

Hi there, welcome to WordPress Trac! Thanks for the ticket and the PR, this looks great.

Keeping the ticket open for now to address the notes from your PR.

[SergeyBiryukov]

In 53460:

General: Remove redundant ltrim() from path_join().

If the path starts with a slash, it will be considered absolute and returned as is earlier in the function.

It it's not absolute, then it does not start with a slash, so there is nothing to trim.

This change is covered by existing unit tests.

Follow-up to [6984], [53457].

Props karlijnbk.
See #55897.

[joyously]

Remove redundant ltrim() from path_join()

Isn't it only redundant in the second return in path_join? It seems like the ltrim should be used in the first return. There is no test case for a path with multiple slashes.
Also, the path_is_absolute function checks for / or \, so should these paths be normalized first, before joining them?

[SergeyBiryukov]

Replying to joyously:

Isn't it only redundant in the second return in path_join? It seems like the ltrim should be used in the first return.
There is no test case for a path with multiple slashes.
Also, the path_is_absolute function checks for / or \, so should these paths be normalized first, before joining them?

Good suggestions, thanks! Yes, it was only redundant in (and is now removed from) the second return.

Looks like you're right about including it in the first return, and we should include a test case for a path with multiple slashes.

I'm not sure about normalizing the paths though. Looking at how path_join() is used in core, I don't see any immediate issues with mixed slashes there. On second thought, I don't see any harm in that, as it will indeed provide more consistent behavior.

[joyously]

Regardless of the function, this ticket is about test cases, and the case of mixed slashes is not there. (not sure if it's in some existing test case)

Two things I'd like to mention about the path_join method:

1. Maybe some verification or checks should be added to make sure $base and $path are both indeed strings. Newer php versions might break otherwise (PHP8 for instance can give a fatal error).
2. The ltrim for the path variable is redundant, because if the path variable starts with slashes, it will be considered absolute, thus entering the if statement and returning $path. It will only reach the ltrim( $path, '/' ) if $path is not absolute, i.e. does not start with a slash, so nothing needs to be trimmed.

Well done @karlijnbok 👍🏻 and I concur on both your observations.

[SergeyBiryukov]

In 53461:

Tests: Add some test cases for path_join() with Windows paths.

Follow-up to [6984], [53457], [53460].

Props joyously.
See #55897.

[SergeyBiryukov]

Replying to SergeyBiryukov:

Looks like you're right about including it in the first return, and we should include a test case for a path with multiple slashes.

A test case was added in [53461].

Thinking about it some more, I believe the current behavior is correct: Windows supports any combination of two or more leading slashes or backslashes in network shares:

• \\host\share
• \\host/share
• //host\share
• //host/share
• \\\host\share
• \\\host/share
• ///host\share
• ///host/share

So using return '/' . ltrim( $path, '/' ) in case of an absolute path would break it, keeping only a single slash or introducing a mix of slashes. We could limit the leading slashes or backslashes to two, but that would make the function a bit more complicated without an obvious benefit.

There are a few further enhancements that could be made here:

• Making sure that $base and $path are both indeed strings. This could use some discussion on how to handle and what to return in case of an invalid input.
• Normalizing the slashes and adding test cases for mixed slashes.
• Limiting the leading slashes to two and adding test cases for more than two slashes.

I think they should be explored in a new ticket, as this one was focused on adding some tests for the current behavior.

[SergeyBiryukov]

Closing as fixed for now, based on the comment above. See [53457], [53460], and [53461].