Introducing \WP_CLI\Utils\normalize_path function. Using it for ABSPATH constant.#4718
Introducing \WP_CLI\Utils\normalize_path function. Using it for ABSPATH constant.#4718schlessera merged 4 commits intowp-cli:masterfrom
\WP_CLI\Utils\normalize_path function. Using it for ABSPATH constant.#4718Conversation
|
I didn't modify any other functions (like e.g. I think it's great that we now have this normalization function, since everyone can now use it for cross-platform (in other words, Windows-friendly) path operations. |
php/utils.php
Outdated
| /** | ||
| * Normalize a filesystem path. | ||
| * | ||
| * On windows systems, replaces backslashes with forward slashes |
php/utils.php
Outdated
| * On windows systems, replaces backslashes with forward slashes | ||
| * and forces upper-case drive letters. | ||
| * Allows for two leading slashes for Windows network shares, but | ||
| * ensures that all other duplicate slashes are reduced to a single. |
There was a problem hiding this comment.
reduced to a single. => reduced to a single one.
Ah cool, somehow I wasn't aware of this function If you could copy the core test also, to
That's fine, will be done in another PR.
Absolutely! |
php/utils.php
Outdated
| * Allows for two leading slashes for Windows network shares, but | ||
| * ensures that all other duplicate slashes are reduced to a single. | ||
| * Ensures upper-case drive letters on Windows systems. | ||
| * Allows for Windows network shares. |
There was a problem hiding this comment.
To add my nitpicks, can just remove this comment "Allows for WIndows network shares." (taken from the since I see) as it's covered above. Also maybe add Copied from core wp_normalize_path(). or similar, and access public and category System tags as in for instance trailingslashit().
|
Thanks a lot @schlessera and @gitlost for the feedback. I'm very happy to see the high bar for the code quality :) I've addressed all of your feedback above, please review! I've also added a unit test for the new function, where I copied all of the WP Core test cases plus added a couple of new ones (like empty-string test and root-only test).
Yes, I agree, no need to worry about it for now, but in general I like the idea of having a wrapper for paths - if done well, that would alleviate quite a bit of potential path-related headache :) |
gitlost
left a comment
gitlost
left a comment
There was a problem hiding this comment.
Good stuff @ericgopak , thanks!
|
Just to note the Travis failure for the trunk job is due to this recent change https://core.trac.wordpress.org/ticket/43228 |
|
Thanks @gitlost for the note! Then we're just waiting for @schlessera to approve this PR. |
|
@ericgopak Just to note we also have to wait for the trunk issue to be resolved (or if that takes too long workaround it - which is easy to do but better if we don't). |
4 participants
As discussed with @gitlost here: wp-cli/checksum-command#39
Introducing
normalize_pathfunction (copied it from latest the WP Core :) ), and applied it to theABSPATHconstant, so that path concatenations of it have (more or less) consistent path separators.