Make WordPress Core

#55897 closed enhancement (fixed)

Add tests for path_join()

Reported by: karlijnbk's profile karlijnbk Owned by: karlijnbk's profile karlijnbk
Milestone: 6.1 Priority: normal
Severity: normal Version: 6.1
Component: General Keywords: has-patch has-unit-tests
Focuses: Cc:

Description


Change History (14)

This ticket was mentioned in PR #2774 on WordPress/wordpress-develop by karlijnbok.


21 months ago
#1

  • Keywords has-patch has-unit-tests added

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.

#2 @karlijnbk
21 months ago

  • Keywords dev-feedback added

#3 @SergeyBiryukov
21 months ago

  • Milestone changed from Awaiting Review to 6.1

#4 @SergeyBiryukov
21 months ago

In 53457:

Tests: Add unit tests for path_join().

Props karlijnbk.
See #55897.

#6 @SergeyBiryukov
21 months ago

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.

#7 @SergeyBiryukov
21 months ago

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.

#8 follow-up: @joyously
21 months ago

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?

#9 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov
21 months ago

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.

#10 @joyously
21 months ago

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)

jrfnl commented on PR #2774:


21 months ago
#11

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.

#12 @SergeyBiryukov
21 months ago

In 53461:

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

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

Props joyously.
See #55897.

#13 in reply to: ↑ 9 @SergeyBiryukov
21 months ago

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.

#14 @SergeyBiryukov
19 months ago

  • Keywords dev-feedback removed
  • Resolution set to fixed
  • Status changed from assigned to closed

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

Note: See TracTickets for help on using tickets.