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

Don’t try to extract anything when there are no PHP files #24

Merged
merged 3 commits into from Mar 6, 2018

Conversation

2 participants
@swissspidy
Collaborator

swissspidy commented Mar 6, 2018

This is commonly the case when adding a child theme where only strings from style.css should get extracted but nothing else.

Without this change, \Gettext\Extractors\Extractor::getFiles() throws an InvalidArgumentException when being passed an empty array.

Now, the function is not being called in that scenario. Plus, any exceptions will be caught an printed by WP_CLI::error().

Don’t try to extract anything when there are no PHP files
This is commonly the case when adding a child theme where only strings from `style.css` should get extracted but nothing else.

@swissspidy swissspidy requested a review from wp-cli/committers Mar 6, 2018

Show outdated Hide outdated features/makepot.feature
try {
if ( ! empty( $files ) ) {
static::fromFile( $files, $translations, $options );

This comment has been minimized.

@schlessera

schlessera Mar 6, 2018

Member

Unrelated to this PR, but makes me wonder whether public static function fromFile( $file, ... ) should not be named public static fromFiles( $files, ... ) instead (plural vor name and argument), as it loops over multiple files.
Alternatively, the loop could be extracted out of fromFile() and put into the surrounding code. Might make more sense, conceptually. In that case, the above would turn from an if into a simple foreach.

@schlessera

schlessera Mar 6, 2018

Member

Unrelated to this PR, but makes me wonder whether public static function fromFile( $file, ... ) should not be named public static fromFiles( $files, ... ) instead (plural vor name and argument), as it loops over multiple files.
Alternatively, the loop could be extracted out of fromFile() and put into the surrounding code. Might make more sense, conceptually. In that case, the above would turn from an if into a simple foreach.

This comment has been minimized.

@swissspidy

swissspidy Mar 6, 2018

Collaborator

I totally agree that it's not ideal.

Just note that this method implements ExtractorInterface::fromFile() where the doc block says @param array|string $file A path of a file or files. The abstract Extractor class it extends does the same, so I tried to keep the same behavior.

@swissspidy

swissspidy Mar 6, 2018

Collaborator

I totally agree that it's not ideal.

Just note that this method implements ExtractorInterface::fromFile() where the doc block says @param array|string $file A path of a file or files. The abstract Extractor class it extends does the same, so I tried to keep the same behavior.

This comment has been minimized.

@schlessera

schlessera Mar 6, 2018

Member

Okay, let's keep it as is for now then.

@schlessera

schlessera Mar 6, 2018

Member

Okay, let's keep it as is for now then.

@schlessera schlessera merged commit 90f3f8e into master Mar 6, 2018

1 check passed

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

@schlessera schlessera added this to the 0.1.0 milestone Mar 6, 2018

@schlessera schlessera deleted the child-theme branch Mar 6, 2018

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