Added methods for absolute path handling in Path utility class - #19592
Open
mirko-pagliai wants to merge 1 commit into
Open
Added methods for absolute path handling in Path utility class#19592mirko-pagliai wants to merge 1 commit into
Path utility class#19592mirko-pagliai wants to merge 1 commit into
Conversation
Member
|
As per policy those new methods should target 5.next. |
Contributor
Author
|
Done. Where can I read the policy? I missed it. |
Member
|
https://book.cakephp.org/5.x/release-policy.html#patch-releases We could probably make this a bit more clear for newcomers. |
- Introduced `isAbsolute()` to check for absolute paths. - Added `makeAbsolute()` to construct absolute paths based on a base path. - Included extensive test coverage for the new methods in `PathTest`.
mirko-pagliai
force-pushed
the
5.x-path
branch
from
August 14, 2026 11:48
1c19156 to
a8f7972
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
isAbsolute()to check for absolute paths.makeAbsolute()to construct absolute paths based on a base path.PathTest.Implementation notes / discussion points
Path separators are always normalized, regardless of OS.
isAbsolute()andmakeAbsolute()follow the existing behaviour ofnormalize()/join(): backslashes are always treated as path separators and Windows drive letters are always recognized, independent of the OS the code happens to run on.makeAbsolute()does not resolve./..segments.The result is a plain concatenation of
$fromand$path(see the../configcase intestMakeAbsolute), not a canonical path. This is consistent with the rest of the class: neitherjoin()nornormalize()resolve dot segments either.Flagging this so it reads as a scope decision, not an oversight — a proper canonicalization method would be a separate, larger addition.
$fromis not validated as absolute.Passing a relative
$fromtomakeAbsolute()will silently produce a result that looks absolute but isn't. No other method in this class throws on invalid input, so this PR keeps that behaviour and documents the expectation in the docblock instead of adding validation/exceptions. Happy to add a guard if reviewers prefer stricter behaviour.Edge case: bare drive letter (
"C:", no trailing separator).isAbsolute()requires a separator after the colon ("C:/"), soisAbsolute('C:')isfalse. A bare"C:"conventionally refers to the current directory on that drive rather than its root, so treating it as non-absolute seemed the safer default — open to discussion if a different convention is preferred.Test coverage suggestions, if useful:
makeAbsolute()with an already-absolute$pathcontaining backslashes (e.g.C:\other\file.php), to exercise the "already absolute, normalize separators" branch explicitly.isAbsolute()with a lowercase drive letter and a drive-relative path (C:folder).P.S. I rightly maintained the same "style" in the tests.
However, from now on, I would really encourage the use of the
DataProviderand (especially)TestWithattributes whenever possible (I can do another PR for this).