Skip to content
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

Avoid throwing a notice about `strpos(): Empty needle` when going through include paths #149

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@connerbw
Copy link
Contributor

commented Mar 22, 2019

Kept getting this warning when doing --include="*.blade.php", among other things.

Related to #125 and #129

Fix: Warning: strpos(): Empty needle
Kept getting this warning when doing `--include="*.blade.php"`, among other things

Related to #125 and #129

@connerbw connerbw requested a review from wp-cli/committers as a code owner Mar 22, 2019

@@ -176,7 +176,7 @@ protected static function containsMatchingChildren( SplFileInfo $dir, array $mat
// Or the start of the matcher until the first wildcard matches the start of the path.
if (
( '' !== $root_relative_path && 0 === strpos( $base, $root_relative_path ) ) ||
0 === strpos( $root_relative_path, $base )
( '' !== $base && 0 === strpos( $root_relative_path, $base ) )

This comment has been minimized.

Copy link
@schlessera

schlessera Mar 23, 2019

Member

@swissspidy This now saves against the thrown notice, but I'm wondering whether it doesn't now exclude valid matches.

If $base is empty, should a match be possible here?

This comment has been minimized.

Copy link
@connerbw

connerbw Mar 24, 2019

Author Contributor

The old code, without my change, in context, was:

	if (
		( '' !== $root_relative_path && 0 === strpos( $base, $root_relative_path ) ) ||
		0 === strpos( $root_relative_path, $base )
	) {
		return true;
	}
}
return false;

Written another way:

	if (
		$CONDITION_1 ||
		$CONDITION_2
	) {
		return true;
	}
}
return false;

My change only suppresses the warning:

$CONDITION_2 = strpos( 'node_modules', '' );
var_dump($CONDITION_2);
// false, aka:
$CONDITION_2 = 0 === strpos( 'node_modules', '' );
var_dump($CONDITION_2);
// false
$CONDITION_2 = '' !== '' ;
var_dump($CONDITION_2);
// false

whether it doesn't now exclude valid matches. If $base is empty, should a match be possible here?

My change doesn't change how the code works, it only suppresses the PHP Warning.

If a match is possible, and the testing suite doesn't catch it, then that's beyond the scope of my PR IMHO.

Hope this helps.

This comment has been minimized.

Copy link
@schlessera

schlessera Apr 1, 2019

Member

Yes, it was clear how the logic works. However, the PR should ideally be about eliminating the root cause of a bug, not just obfuscating it.

In this case, the root cause is that one possible scenario was apparently not being considered by the current logic. The notice as such is valuable as it shows us we apparently forgot something. If we now just hide the notice, the possible bug might still be in there and forgotten because the symptom is being hidden.

This is why I'd like some feedback by @swissspidy, who built the original code, as I'm unsure what an empty base in this case would signify.

This comment has been minimized.

Copy link
@swissspidy

swissspidy Apr 1, 2019

Member

Hmm I think my initial work was pretty much rewritten in #104. So maybe @herregroen knows more?

The warning implies that $path_or_file is an empty string and I wonder why this happens. If it's because of some edge case thing I don't think just breaking early would cause any troubles.

This comment has been minimized.

Copy link
@connerbw

connerbw Apr 1, 2019

Author Contributor

Maybe this helps? I can reproduce every time by running:

wp i18n make-pot . languages/plugin.pot --include="*.blade.php" --exclude="vendor,symbionts" --headers='{"Report-Msgid-Bugs-To":"https://github.com/namespace/plugin/"}

This comment has been minimized.

Copy link
@schlessera

schlessera Aug 6, 2019

Member

@swissspidy I think you misread the code. The issue is not that $path_or_file is empty, but that current( explode( '*', $path_or_file ) ) returns an empty string. This happens for a simple example like *.php, as it will explode to [ '', '.php' ].

This comment has been minimized.

Copy link
@swissspidy

swissspidy Aug 6, 2019

Member

Ah, indeed! My bad.

@schlessera schlessera requested a review from swissspidy Mar 23, 2019

@schlessera

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I'm not so sure that the logic is actually sound, as we seem to just be hiding the problem with this PR. Nevertheless, it gets rid of the notice, and it doesn't actually break anything that wouldn't already be broken in that case anyway.

So I'll go ahead and merge this for now.

@schlessera schlessera added this to the 2.2.0 milestone Aug 13, 2019

@schlessera schlessera merged commit 502185d into wp-cli:master Aug 13, 2019

2 checks passed

DEP All dependencies are resolved
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schlessera schlessera changed the title Fix: Warning: strpos(): Empty needle Avoid throwing a notice about `strpos(): Empty needle` when going through include paths Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.