Skip to content

Extract strings contained within eval() - #222

Merged
schlessera merged 7 commits into
wp-cli:masterfrom
dd32:extractJSEval
Mar 5, 2021
Merged

Extract strings contained within eval()#222
schlessera merged 7 commits into
wp-cli:masterfrom
dd32:extractJSEval

Conversation

@dd32

@dd32 dd32 commented Jul 24, 2020

Copy link
Copy Markdown
Contributor

Some build systems can result in the built files gettext functions being inside eval() calls.

One such example of this, is a webpack'd built file in this plugin: https://plugins.trac.wordpress.org/browser/mathml-block/trunk/dist/mathml-block.js?rev=2276811#L245

Currently, the relevant output is this:

#: src/mathml-block.js:35
msgid "MathML formula:"
msgstr ""

#: src/mathml-inline.js:10
msgid "MathML"
msgstr ""

which doesn't work for WordPress Translation files, as those src files are never enqueued, only the built files.
After the change, here's the output:

#: dist/mathml-block.js:245
#: src/mathml-block.js:35
msgid "MathML formula:"
msgstr ""

#: dist/mathml-block.js:257
#: src/mathml-inline.js:10
msgid "MathML"
msgstr ""

@dd32
dd32 requested a review from a team as a code owner July 24, 2020 05:11
@dd32 dd32 added bug command:i18n-make-pot Related to 'i18n make-pot' command labels Jul 24, 2020
@dd32 dd32 mentioned this pull request Jul 24, 2020
Merged
@dd32

dd32 commented Jul 25, 2020

Copy link
Copy Markdown
Contributor Author

The unit tests keep timing out on trunk only - I'm not sure why as I've not been able to run then locally.

@swissspidy

swissspidy commented Jul 30, 2020

Copy link
Copy Markdown
Member

Interesting edge case & nice solution for the line number!

Would you be able to add some Behat tests for this? Could probably just copy some lines from the MathML plugin and then verify that strings are extracted as expected with the correct line number.

@dd32

dd32 commented Jul 31, 2020

Copy link
Copy Markdown
Contributor Author

I'll get some tests added @swissspidy - Hopefully I can figure out how to run the tests locally

@dd32

dd32 commented Jul 31, 2020

Copy link
Copy Markdown
Contributor Author

Unfortunately I wasn't able to get tests running locally, and there are no existing tests for the JS set of extraction methods, so I can't really fly into it blind either.

@swissspidy

swissspidy commented Jul 31, 2020

Copy link
Copy Markdown
Member

There are no unit tests for it, but Behat tests, starting here:

i18n-command/features/makepot.feature

Lines 1559 to 1580 in b02ecdc

Scenario: Extracts functions from JavaScript file
Given an empty foo-plugin directory
And a foo-plugin/foo-plugin.php file:
"""
<?php
/**
* Plugin Name: Foo Plugin
*/
"""
And a foo-plugin/foo-plugin.js file:
"""
// Included to test if peast correctly parses regexes containing a quote.
// See: https://github.com/wp-cli/i18n-command/issues/98
n = n.replace(/"/g, '&quot;');
n = n.replace(/"|'/g, '&quot;');
__( '__', 'foo-plugin' );
_x( '_x', '_x_context', 'foo-plugin' );
_n( '_n_single', '_n_plural', number, 'foo-plugin' );
_nx( '_nx_single', '_nx_plural', number, '_nx_context', 'foo-plugin' );
__( 'wrong-domain', 'wrong-domain' );

If it's not working, feel free to add me to your fork so I could amend, otherwise I can open a new PR with your commit in it and the tests.

@swissspidy

swissspidy commented Sep 19, 2020

Copy link
Copy Markdown
Member

@schlessera Any idea about the test timeouts? I can't re-start Travis in case it's just a flake.

@adamsilverstein adamsilverstein mentioned this pull request Oct 19, 2020
Closed
@schlessera schlessera added this to the 2.2.7 milestone Mar 5, 2021
@schlessera
schlessera merged commit 9cdde49 into wp-cli:master Mar 5, 2021
schlessera added a commit that referenced this pull request Jan 6, 2022
@schlessera
Merge pull request #222 from dd32/extractJSEval
bfbfaf0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swissspidy swissspidy swissspidy approved these changes

Assignees

No one assigned

Labels

bug command:i18n-make-pot Related to 'i18n make-pot' command

Projects

None yet

Milestone

2.2.7

Development

Successfully merging this pull request may close these issues.

3 participants

@dd32 @swissspidy @schlessera