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.

Sign up for GitHub

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
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/IterableCodeExtractor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

@dac514 dac514 Mar 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@schlessera schlessera Apr 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/"}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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' ].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, indeed! My bad.

) {
return true;
}
Expand Down