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

Add backslash to the regex for matching Windows paths correctly #39

Merged
merged 2 commits into from Mar 2, 2018

Conversation

2 participants
@ericgopak
Contributor

ericgopak commented Feb 28, 2018

This solves the issue on Windows.
It's funny how backslash should be escaped, but here's a reference why that's so: https://www.developwebsites.net/match-backslash-preg_match-php/

@gitlost

This comment has been minimized.

Contributor

gitlost commented Mar 2, 2018

Thanks for the PR @ericgopak . I think though a better way to fix the issue would be to normalize the directory separators first in Checksum_Base_Command::get_files() as the wp-admin/ and wp-includes/ checks in Checksum_Core_Command::filter_file() won't work on Windows and extraneous files there will be missed.

There should be a Utils\normalize_directory_separators() function to do this but in the meantime you could just add it to Checksum_Base_Command eg

	/**
	 * Normalizes directory separators to slashes.
	 *
	 * @param string $path Path to convert.
	 *
	 * @return string Path with all backslashes replaced by slashes.
	 */
	public static function normalize_directory_separators( $path ) {
		return str_replace( '\\', '/', $path );
	}

and then call it in Checksum_Base_Command::get_files() at Checksum_Base_Command.php#L45:

$pathname = self::normalize_directory_separators( substr( $file_info->getPathname(), strlen( $path ) ) );

@gitlost gitlost added this to the 1.0.9 milestone Mar 2, 2018

@ericgopak

This comment has been minimized.

Contributor

ericgopak commented Mar 2, 2018

Thanks a lot for the review @gitlost ! You are totally right, I haven't noticed that get_files function was using filter_file which is overridden in Checksum_Core_Command subclass. Applied your suggestions - please review :)

Apparently my previous solution didn't work at all, although it did get rid of error messages :/
Now I've interactively debugged it via PHPStorm with XDebug enabled, and everything seems to work - all the files are listed and checked correctly, because the directory separators are now normalized.

@ericgopak

This comment has been minimized.

Contributor

ericgopak commented Mar 2, 2018

I agree that the normalization function should be moved up to \WP_CLI\Utils sometime in the future. Also, probably all the paths should be normalized as soon as they are retrieved from "external sources" (e.g. syscall, HTTP request, etc.). Currently the ABSPATH variable still contains non-normalized path separators, but I think it's out of the scope of this issue #35.

@gitlost

gitlost approved these changes Mar 2, 2018

@gitlost gitlost merged commit fc00421 into wp-cli:master Mar 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gitlost

This comment has been minimized.

Contributor

gitlost commented Mar 2, 2018

Thanks very much @ericgopak for the superquick response and feedback!

Also, probably all the paths should be normalized as soon as they are retrieved from "external sources" (e.g. syscall, HTTP request, etc.).

Yes, you're right, they should be.

Currently the ABSPATH variable still contains non-normalized path separators, but I think it's out of the scope of this issue

Yes, this should be addressed in the framework wp-cli/wp-cli when setting it here WP_CLI/Runner.php#L235.

If you have the time and you'd like to submit a PR to add the normalization function to \WP_CLI\Utils and use it when defining ABSPATH (and it could also be used in Utils\get_temp_dir()) it would be most welcome. Thanks again!

@ericgopak

This comment has been minimized.

Contributor

ericgopak commented Mar 2, 2018

Thanks @gitlost !
Yes, I would love to help. I think I'll have time for that during the weekend.

Should we create a new issue for that?

I am new to WP-CLI repo and to contributing to WP in general, so any guidance is greatly appreciated :)

For example, how should I move the normalization function up to \WP_CLI\Utils? Should I create two separate pull requests?

@gitlost

This comment has been minimized.

Contributor

gitlost commented Mar 2, 2018

Good stuff!

Should we create a new issue for that?

A straight PR referencing this PR will suffice in this case.

For example, how should I move the normalization function

For the moment just do the PR on wp-cli/wp-cli please and then when that's merged we can come back and DRY up this code.

Ta!

@schlessera schlessera changed the title from Fixing issue #35 - adding backslash to the regex for matching Windows paths correctly to Add backslash to the regex for matching Windows paths correctly Apr 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment