The code in _potx_find_context does not support PHP's [] array syntax.

For example it won't process the following code correctly.

$b = new TranslationWrapper('TranslationWrapper string with context', [], ['context' => 'With context']);
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
5.42 KB

Here's a failing test.

Status: Needs review » Needs work

The last submitted patch, 2: 2598910.2.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
680 bytes

#2 had the wrong patch.

Status: Needs review » Needs work

The last submitted patch, 4: 2598910.4.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.52 KB

Here's an approach that works and does not break anything.

Status: Needs review » Needs work

The last submitted patch, 6: 2598910.6.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.15 KB

alexpott-- failed at patch creation

Status: Needs review » Needs work

The last submitted patch, 8: 2598910.8.patch, failed testing.

alexpott’s picture

This is awesome...

Parse error: syntax error, unexpected '[' in ./sites/default/modules/potx/tests/potx_test_8.module on line 22
Errors parsing ./sites/default/modules/potx/tests/potx_test_8.module].

So we can't test this because Drupal 7 test infra will fail cause lint checking

alexpott’s picture

Status: Needs work » Needs review
FileSize
824 bytes
3.98 KB

Okay linting Drupal 8 code is not a good idea - let's rename it.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Drupal 8 compatibility

Yay, thanks! While the patch also parses array(.... ] and [....), those should hopefully not appear in Drupal code. :) There are other things that break more badly with bad code.

So this is my only concern:

+++ b/potx.inc
@@ -1274,19 +1274,19 @@ function _potx_find_context($tf, $ti, $file, $function_name) {
-          if ($_potx_tokens[$ti] == ')') {
+          if (_potx_is_end_of_array($_potx_tokens[$ti])) {

@@ -1317,6 +1317,39 @@ function _potx_find_context($tf, $ti, $file, $function_name) {
+/**
+ * Determines if at the end of an array.
+ *
+ * @param string $token
+ *   The token being processed.
+ *
+ * @return bool
+ *   TRUE is  at the end of an array, FALSE if not.
+ */
+function _potx_is_end_of_array($token) {
+  return $token == ')' || $token == ']';
+}

This second use of this function is not actually meant to find end of array, it was meant to figure out the end of the arg argument which may include functions invoked, etc. For example:

t('Boo @value', array('@value' => boo_load_multiple([1])[0]->id()) (a contrived example to contain all of long arrays, short arrays and methods :)

In this case the method invocations and arrays both go down to the nesting and we are interested when we come back out at the end of the arguments short or long array part :)

Gábor Hojtsy’s picture

Also causes issues with #2601370: [META] Parsing notices with Drupal 8 RC2, adding as parent.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.31 KB

More test coverage with unique strings for different combinations of tokens and inlining the end of nesting check where it does not necessarily mean end of array. The test includes function calls, nested arrays, etc. to demonstrate that now :)

Status: Needs review » Needs work

The last submitted patch, 15: 2598910-15.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

I did not apply @alexpott's patch proper. Now with the rename of module to module.txt.

Gábor Hojtsy’s picture

FileSize
12.06 KB

Status: Needs review » Needs work

The last submitted patch, 18: 2598910-17.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
12.87 KB
2.92 KB

Test was failing because it asserted a source string we did not have in the test anymore :) Adding one more test for a complicated options array and some docs.

  • Gábor Hojtsy committed f30403e on 7.x-3.x
    Issue #2598910 by alexpott, Gábor Hojtsy: Support new fandangled PHP...
Gábor Hojtsy’s picture

Status: Needs review » Fixed

All right, got this one in. Will see if this resolves the outstanding ones from #2601370: [META] Parsing notices with Drupal 8 RC2 :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.