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

Add support for PHP-Blade files #304

Merged
merged 16 commits into from
Mar 13, 2022
Merged

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Feb 21, 2022

This PR adds the code and (simple) tests for extracting translations strings from PHP-Blade template files.

Luckily the 4.x branch of the gettext project already contains a fully working PHP-Blade Extractor,
which can be used very similarly to the PHP extractor for wp i18n.

The IterableCodeExtractor file filter had to be refactored a bit to make it also support filtering for multiple file extensions (as it is the case with blade.php for PHP-Blade template files).

The illuminate/view BladeOne compiler for a standalone PHP-Blade compiler had to be included as composer dependency as the gettext package doesn't require it on its own/expects a PHP-Blade compiler to be available. Edit: Because of current PHP version constraints by i18n, a release of another, standalone and well-maintained PHP-Blade compiler is used instead (BladeOne).

Discussion

Both classes, PhpCodeExtractor and BladeCodeExtractor duplicate some code, $options and $functionsScannerClass:
https://github.com/strarsis/i18n-command/blob/add-blade-extractor/src/PhpCodeExtractor.php#L13-L46
https://github.com/strarsis/i18n-command/blob/add-blade-extractor/src/BladeCodeExtractor.php#L13-L46

Side note: Putting these two aforementioned fields $options and $functionsScannerClass into a PHP trait doesn't work as PHP traits can't override class fields (as I learned by trying to do so).
Both classes have to extend from a class from gettext/gettext, therefore a trivial way for letting both classes share these fields doesn't seem to exist, so I think those duplications (which are quite compact) can be tolerated.

Refactor the IterableCodeExtractor filter so it supports multiple file extensions (`blade.php`).
@strarsis strarsis requested a review from a team as a code owner February 21, 2022 02:46
@strarsis strarsis changed the title Adds code + tests for adding a PHP-Blade extractor Add extractor/support for PHP-Blade files Feb 21, 2022
@strarsis strarsis mentioned this pull request Feb 21, 2022
1 task
@strarsis strarsis changed the title Add extractor/support for PHP-Blade files Add support for PHP-Blade files Feb 21, 2022
@strarsis strarsis mentioned this pull request Feb 21, 2022
1 task
composer.json Outdated Show resolved Hide resolved
@endsection
"""

When I try `wp i18n make-pot example-project result.pot --ignore-domain --debug`
Copy link
Member

Choose a reason for hiding this comment

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

An additional scenario for testing extraction without --ignore-domain would be helpful too.

src/MakePotCommand.php Outdated Show resolved Hide resolved
This was referenced Feb 21, 2022
@swissspidy swissspidy requested a review from a team February 21, 2022 09:56
@swissspidy
Copy link
Member

Thanks for opening this!

For context, Blade support was originally proposed in #147 but was more seen as out of scope. But this might be a reasonable addition after all.

@strarsis
Copy link
Contributor Author

@swissspidy: After adding a test that doesn't ignore the translation domains, I noticed an issue with the PHP function scanner. It probably needs some adjustments. 🐱

@swissspidy
Copy link
Member

@swissspidy: After adding a test that doesn't ignore the translation domains, I noticed an issue with the PHP function scanner. It probably needs some adjustments. 🐱

What kind of issue?

The illuminate/view for the standalone Blade compiler had to be included as composer dependency as the gettext package doesn't require it on its own/expects a Blade compiler to be available.

illuminate/view v8.83.0 requires PHP 7.3, but this command still supports PHP 7.0, so that doesn't work. Unfortunately all older versions that would be compatible have security advisories.

@strarsis
Copy link
Contributor Author

strarsis commented Feb 21, 2022

@swissspidy: The illuminate/view - PHP 7.0 issue is now solved by using the very well maintained BladeOne standalone Blade compiler/engine release 3.52 which is also supported by PHP 7.0. There are currently no security advisories listed for BladeOne. And it is basically a re-implementation of the Blade engine used by illuminate/view. The changelog entries from that older release 3.5.2 of BladeOne up to the current release doesn't list any security issues either. Hence BladeOne 3.5.2 can be considered a suitable replacement.
The Blade Extractor from the Gettext package was slightly modified and added as a separate class to use BladeOne instead. This is probably better than monkey-patching the class and method invocation of the original implementation that uses illuminate/view.

PHP 7.1.x or 7.2.x minimum requirement for wp i18n would be nice though because then one of the most recent releases of BladeOne can be used (4.4.2 for PHP 7.2.5 or 4.3 for PHP 7.1.3).

The tests pass fine except for the one that doesn't ignore the translation domains, it doesn't include any translations from the blade file then. I still try to figure out what the reason is, probably the PHP function scanner.

@strarsis
Copy link
Contributor Author

strarsis commented Feb 22, 2022

@swissspidy: Alright, the translation domain issue was fixed.
Tests all pass fine.

I would really like to see this merged (and released) so I can easily use it,
most themes I work on are based on Sage (9/10) and they all use Blade Templates.

@swissspidy
Copy link
Member

@schlessera WDYT about the PHP version requirement bump mentioned above (and this enhancement in general)?

@schlessera
Copy link
Member

@swissspidy, @strarsis The current PHP version requirement policy is the following:
"Whenever WordPress Core raises the PHP minimum version requirement past WP-CLI, WP-CLI will stay at the old minimum requirement for at least 1 full year."
(see https://make.wordpress.org/cli/2019/01/15/wp-cli-php-requirements-strategy/)
This means we still need to stay at PHP 5.6+ for now and are blocked to that minimum as long as WP Core itself does not bump it.

@strarsis
Copy link
Contributor Author

strarsis commented Mar 11, 2022

@schlessera: So even PHP 7.1 would be still too high for i18n then.

The good news is that the Blade Engine BladeOne release v3.52 (which doesn't have issues) used in this PR supports PHP 5.6+!

It would be great if this PR can be merged, I need wp i18n for most of the themes I work on.

@swissspidy
Copy link
Member

This command already has a higher PHP version requirement AFAIK

@schlessera
Copy link
Member

Nope, it is still being tested against 5.6
Merge pull request #302 from shendy-a8c_fix_include-subfolder-of-excl… · wp  2022-03-11 at 10 23 22 AM
:

@swissspidy swissspidy merged commit e506287 into wp-cli:main Mar 13, 2022
@strarsis
Copy link
Contributor Author

@swissspidy: Great that this was merged! I can install this WP CLI command from source, but it would be even better if it is published as a new release. Are there plans for a new release of i18n in the near future?

@swissspidy
Copy link
Member

We should be able to tag a new release soon, but usually that's done closer to the release of WP-CLI itself. In the meantime, wp package install [email protected]:wp-cli/i18n-command.git is the way to go.

@strarsis
Copy link
Contributor Author

strarsis commented Mar 21, 2022

@swissspidy: There is an issue with installing WP CLI i18n from the repository (wp package install [email protected]:wp-cli/i18n-command.git): The new wp i18n uses the previous version of IterableCodeExtractor from the wp CLI PHAR,
which prevents .blade.php files from being correctly filtered in/out.

Edit: Available with recent release of wp i18n 2.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants