False negatives

// Concatenating some symbols is fine.
$x = '(' . t('Test') . ')';
$x = '[' . t('Test') . ']';
$x = '- ' . t('Test') . ' -';
$x = '<' . t('Test') . '>';

The above code should not produce warnings.

False positive

// Concatenating markup with text in it is not fine.
$x = '<p>' . t('Test') . '</p><p>More text.</p>';

This code should produce a warning.

CommentFileSizeAuthor
#7 improve-2716963-1.patch4.37 KBborisson_
#7 interdiff.txt2.99 KBborisson_
#5 improve-2716963-5.patch3.66 KBborisson_
#5 interdiff.txt2.5 KBborisson_
#2 2716963.2.patch3.62 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2716963.2.patch3.62 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented

We have a similar warning in Search API's codebase on using $this->t('General') . ' » ', I think « and » should be included as allowed special characters as well.

Otherwise, this patch looks great.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work 
+++ b/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTSniff.php
@@ -128,5 +128,29 @@ class Drupal_Sniffs_Semantics_FunctionTSniff extends Drupal_Sniffs_Semantics_Fun
+        $string = eval('return ' . $string . ';');

wait what, eval()??? Does that mean I can write a PHP file that can do arbitrary code execution as soon as it is scanned with PHPCS?

It should be safe to run PHPCS on any third party code base, so I think this is a no-go. Can we do this some other way?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.5 KB
improve-2716963-5.patch3.66 KB

Tests pass, so this approach should work as well.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work 
  1. +++ b/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTSniff.php
@@ -87,7 +87,7 @@ class Drupal_Sniffs_Semantics_FunctionTSniff extends Drupal_Sniffs_Semantics_Fun
+                && !$this->checkConcatString($tokens[$stringAfter]['content'])

    PHPCS coding standards: do not use "!", "use === false" instead. 

  2. +++ b/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTSniff.php
@@ -128,5 +128,29 @@ class Drupal_Sniffs_Semantics_FunctionTSniff extends Drupal_Sniffs_Semantics_Fun
+     * @param $string

    type is missing (string) 

  3. +++ b/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTSniff.php
@@ -128,5 +128,29 @@ class Drupal_Sniffs_Semantics_FunctionTSniff extends Drupal_Sniffs_Semantics_Fun
+     *   The string to check.

    Comment should be "The string that is concatenated to a t() call." 

  4. +++ b/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTSniff.php
@@ -128,5 +128,29 @@ class Drupal_Sniffs_Semantics_FunctionTSniff extends Drupal_Sniffs_Semantics_Fun
+        // Interpret the encapsulated string.
+        $string = str_replace("'", "", $string);

    Comment is wrong now. 

  5. +++ b/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTSniff.php
@@ -128,5 +128,29 @@ class Drupal_Sniffs_Semantics_FunctionTSniff extends Drupal_Sniffs_Semantics_Fun
+            return TRUE;

    TRUE should be lower case in PHPCS coding standards, make sure to run phpcbf --standard=PHPCS on the sniff file.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.99 KB
improve-2716963-1.patch4.37 KB

Fixed all remarks made in #6.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I'm on the go atm. There was a reason I did the eval(). It means that whatever the string was we would treat it correctly - things like \n. There actually was no danger whatsoever unless php' tokeniser is broken. as the only thing being passed in is something between single or double quotes. I'll update the tests to show what I mean.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

While it seems like you are right I still have a very bad feeling about using eval() - there might be creative ways to trick the PHP tokenizer or whatever that we don't know yet. Let's not risk that.

It seems you only want to remove the quotes, so just trim() them off? I think we don't care if some escaped quotes remain in the string.

  • klausi committed c0a868a on 8.x-2.x authored by alexpott 
    Issue #2716963 by borisson_, alexpott, klausi: Allow symbols to be...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Title: Improve Drupal.Semantics.FunctionT.ConcatString by reducing false positives and negatvies » Allow symbols to be concatenated to t() calls
Status: Needs review » Fixed

Also added the regression to good.php and committed it, thanks!

Feel free to follow up with further improvements.

Status: Fixed » Closed (fixed)

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