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

Investigate include/exclude oddities #175

Merged
merged 10 commits into from Aug 13, 2019

Conversation

@swissspidy
Copy link
Member

commented Jul 23, 2019

I've tried replicating #172 with the similar setup:

- test.php
- vendor/
- vendor/test.php

I ran:

vendor/bin/wp i18n make-pot /tmp/wp-i18n-test --debug --include=/vendor/test.php

And the vendor/test.php file was not included as expected.

Then I added 763941a and the file was now included as expected.

However, I failed at verifying this with a PHPUnit test.

swissspidy added some commits Jul 23, 2019

@swissspidy swissspidy requested a review from wp-cli/committers as a code owner Jul 23, 2019

swissspidy and others added some commits Jul 23, 2019

@schlessera

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I looked into the tests, and for the failing test, the function to extract the files returns an empty array. I'll take a closer look at the code now.

@schlessera

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

From what I can see on my system, this change does nothing, the strings that get passed into that method never contain a leading slash for the tests you are doing...

@schlessera

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@swissspidy Can you take a look at the code change I made to validate?

What I found is that the logic that was in place include directories that had files that might match ... but excluded the actual files that would have matched.

@schlessera

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Okay, it's obviously not enough to have the unit tests pass...

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

@schlessera schlessera merged commit 379d2b0 into master Aug 13, 2019

1 check passed

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

@delete-merged-branch delete-merged-branch bot deleted the try/172-exclude-include branch Aug 13, 2019

@BartvanS

This comment has been minimized.

Copy link

commented Aug 19, 2019

I updated my wp-cli and wp cli i18n. Sadly if i recreate this scenario i still only get the translations from the test.php and not the vendor/test.php.
$ wp cli version -> 2.3.0
I installed the latest version of wp i18n with:
$ wp package install git@github.com:wp-cli/i18n-command.git

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

@BartvanS What command are you running?

@BartvanS

This comment has been minimized.

Copy link

commented Aug 19, 2019

wp i18n make-pot ./ ./test.pot --ignore-domain --include=/vendor/test.php
@schlessera

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@BartvanS Do you maybe have the wp-cli/i18n-command package installed as an external package as well? You can check this via wp package list.

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.