New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing `\WP_CLI\Utils\normalize_path` function. Using it for ABSPATH constant. #4718

Merged
merged 4 commits into from Mar 18, 2018

Conversation

4 participants
@ericgopak
Contributor

ericgopak commented Mar 4, 2018

As discussed with @gitlost here: wp-cli/checksum-command#39
Introducing normalize_path function (copied it from latest the WP Core :) ), and applied it to the ABSPATH constant, so that path concatenations of it have (more or less) consistent path separators.

@ericgopak

This comment has been minimized.

Show comment
Hide comment
@ericgopak

ericgopak Mar 4, 2018

Contributor

I didn't modify any other functions (like e.g. Utils\get_temp_dir) to make use of this path normalization, since I am not sure it won't break anything for Windows users.

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.

Contributor

ericgopak commented Mar 4, 2018

I didn't modify any other functions (like e.g. Utils\get_temp_dir) to make use of this path normalization, since I am not sure it won't break anything for Windows users.

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.

@danielbachhuber danielbachhuber added this to the 2.0.0 milestone Mar 5, 2018

@danielbachhuber danielbachhuber requested a review from gitlost Mar 5, 2018

@schlessera

Only minor nitpicking.

Show outdated Hide outdated php/utils.php
Show outdated Hide outdated php/utils.php
@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Mar 5, 2018

Contributor

copied it from latest the WP Core

Ah cool, somehow I wasn't aware of this function wp_normalize_path(). There's a trunk version that deals with PHP stream wrappers changeset [42387] but we can ignore that for now I think.

If you could copy the core test also, to tests/test-utils.php, it'd be good.

I didn't modify any other functions

That's fine, will be done in another PR.

I think it's great that we now have this normalization function

Absolutely!

Contributor

gitlost commented Mar 5, 2018

copied it from latest the WP Core

Ah cool, somehow I wasn't aware of this function wp_normalize_path(). There's a trunk version that deals with PHP stream wrappers changeset [42387] but we can ignore that for now I think.

If you could copy the core test also, to tests/test-utils.php, it'd be good.

I didn't modify any other functions

That's fine, will be done in another PR.

I think it's great that we now have this normalization function

Absolutely!

Show outdated Hide outdated php/utils.php
@ericgopak

This comment has been minimized.

Show comment
Hide comment
@ericgopak

ericgopak Mar 5, 2018

Contributor

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

There's a trunk version that deals with PHP stream wrappers changeset [42387] but we can ignore that for now I think.

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

Contributor

ericgopak commented Mar 5, 2018

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

There's a trunk version that deals with PHP stream wrappers changeset [42387] but we can ignore that for now I think.

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

gitlost approved these changes Mar 5, 2018

Good stuff @ericgopak , thanks!

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Mar 6, 2018

Contributor

Just to note the Travis failure for the trunk job is due to this recent change https://core.trac.wordpress.org/ticket/43228

Contributor

gitlost commented Mar 6, 2018

Just to note the Travis failure for the trunk job is due to this recent change https://core.trac.wordpress.org/ticket/43228

@ericgopak

This comment has been minimized.

Show comment
Hide comment
@ericgopak

ericgopak Mar 6, 2018

Contributor

Thanks @gitlost for the note!

Then we're just waiting for @schlessera to approve this PR.

Contributor

ericgopak commented Mar 6, 2018

Thanks @gitlost for the note!

Then we're just waiting for @schlessera to approve this PR.

@gitlost gitlost referenced this pull request Mar 6, 2018

Merged

Fix sed -i option on MacOS #132

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Mar 6, 2018

Contributor

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

Contributor

gitlost commented Mar 6, 2018

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

@schlessera schlessera merged commit 30e55f6 into wp-cli:master Mar 18, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment