Undefined index: scope_closer for Javascript do/while loops without braces [#2530854]

When I scan a javascript file with the following code snippet in it:

do i++;
while (something())

I get the following notice:

Notice: Undefined index: scope_closer in coder/coder_sniffer/Drupal/Sniffs/ControlStructures/ControlSignatureSniff.php on line 185

Call Stack:
    0.0002     224056   1. {main}() vendor/squizlabs/php_codesniffer/scripts/phpcs:0
    0.0204    1542856   2. PHP_CodeSniffer_CLI->runphpcs() vendor/squizlabs/php_codesniffer/scripts/phpcs:25
    0.0212    1601320   3. PHP_CodeSniffer_CLI->process() vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php:95
    0.0855    4681872   4. PHP_CodeSniffer->processFiles() vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php:855
    0.0857    4683736   5. PHP_CodeSniffer->processFile() vendor/squizlabs/php_codesniffer/CodeSniffer.php:619
    0.0858    4684328   6. PHP_CodeSniffer->_processFile() vendor/squizlabs/php_codesniffer/CodeSniffer.php:1685
    0.0862    4724936   7. PHP_CodeSniffer_File->start() vendor/squizlabs/php_codesniffer/CodeSniffer.php:1807
    0.0891    5013688   8. Drupal_Sniffs_ControlStructures_ControlSignatureSniff->process() vendor/squizlabs/php_codesniffer/CodeSniffer/File.php:567

This occurs because in Javascript, unlike PHP, curly braces are not required in do-while loops if the loop only contains a single statement. Since PHP CodeSniffer has originally been more PHP-oriented this Javascript specific edge case seems to have never been tested.

Comment	File	Size	Author
#18	interdiff.txt	11.98 KB	pfrenssen
#18	2530854-18.patch	11.62 KB	pfrenssen
#10	interdiff.txt	3.51 KB	pfrenssen
#10	2530854-10.patch	20.54 KB	pfrenssen
#9	interdiff.txt	2.17 KB	pfrenssen
#9	2530854-9.patch	20.17 KB	pfrenssen
#8	2530854-8.patch	18 KB	pfrenssen
#5	interdiff.txt	748 bytes	pfrenssen
#5	2530854-5.patch	5.1 KB	pfrenssen
#2	2530854-1.patch	5.5 KB	pfrenssen
#2	2530854-1-test-only.patch	2 KB	pfrenssen

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

pfrenssen

Sofia

CreditAttribution: pfrenssen for Drupal Association commented 10 July 2015 at 16:49

Issue summary:

View changes

Comment #2

pfrenssen

Sofia

CreditAttribution: pfrenssen for Drupal Association commented 10 July 2015 at 21:28

Status:

Active

» Needs review

File	Size
2530854-1-test-only.patch	2 KB
2530854-1.patch	5.5 KB

This was not so easy. Do-while loops can be implemented without curly braces in javascript, while in PHP the curly braces are required. For example this is valid in javascript:

do $i++;
while (TRUE)

This not only throws this notice in ControlSignatureSniff but it also causes trouble in InlineControlStructureSniff. This is what the fixed code of the above sample looks like with these two sniffs working in tandem:

do {
  $i++; } while (TRUE) {
  }

The best solution I found was to disable the inline sniffs when parsing javascript for a WHILE token that is preceded with a DO token. If there are better ways to solve this I'd be happy to adapt the patch.

Comment #3

pfrenssen

Sofia

CreditAttribution: pfrenssen for Drupal Association commented 10 July 2015 at 22:01

+++ b/coder_sniffer/Drupal/Sniffs/ControlStructures/ControlSignatureSniff.php
@@ -194,7 +208,7 @@ class Drupal_Sniffs_ControlStructures_ControlSignatureSniff implements PHP_CodeS
-        if ($tokens[$stackPtr]['code'] === T_DO) {
+        if ($tokens[$stackPtr]['code'] === T_DO && $closer !== false) {

This hunk is probably not needed any more, it is a leftover from an earlier approach.

Comment #4

pfrenssen

Sofia

CreditAttribution: pfrenssen for European Commission and European Union Institutions, Agencies and Bodies commented 10 July 2015 at 22:02

Comment #5

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 29 July 2015 at 14:45

File	Size
2530854-5.patch	5.1 KB
interdiff.txt	748 bytes

2 files were hidden/shown/deleted

File	Size
2530854-1-test-only.patch	2 KB
2530854-1.patch	5.5 KB

Fixed remark from #3.

Comment #6

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 29 July 2015 at 16:12

Status:

Needs review

» Needs work

This still needs work. If I put the entire statement inline then this will not be handled correctly:

do i++; while (i < 5);

This seems to be due to a bug in PHP CodeSniffer itself.

Comment #7

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 29 July 2015 at 17:24

Created pull request to fix this in PHP Codesniffer: https://github.com/squizlabs/PHP_CodeSniffer/pull/657

Comment #8

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 30 July 2015 at 16:40

Status:

Needs work

» Needs review

File	Size
2530854-8.patch	18 KB

2 files were hidden/shown/deleted

File	Size
2530854-5.patch	5.1 KB
interdiff.txt	748 bytes

Here is a better implementation. This fixes the inline issue, but it is depending on https://github.com/squizlabs/PHP_CodeSniffer/pull/657 so we can better wait until that is accepted upstream so we don't need to use my fork.

No interdiff since there is very little code left over from the previous version of the patch.

Comment #9

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 30 July 2015 at 16:45

File	Size
2530854-9.patch	20.17 KB
interdiff.txt	2.17 KB

1 file was hidden/shown/deleted

File	Size
2530854-8.patch	18 KB

Added test coverage.

Comment #10

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 30 July 2015 at 17:16

File	Size
2530854-10.patch	20.54 KB
interdiff.txt	3.51 KB

2 files were hidden/shown/deleted

File	Size
2530854-9.patch	20.17 KB
interdiff.txt	2.17 KB

Fixed coding standards.

Comment #11

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 31 July 2015 at 11:10

Issue summary:

View changes

Comment #12

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 31 July 2015 at 11:13

Issue summary:

View changes

Comment #13

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 2 August 2015 at 18:31

Title:	Undefined index: scope_closer	» Undefined index: scope_closer for Javascript do/while loops without braces
Status:	Needs review	» Postponed

Patch looks good, let's wait for the upstream change.

Comment #14

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 11 November 2015 at 12:48

Status:

Postponed

» Reviewed & tested by the community

Fix has been accepted upstream: https://github.com/squizlabs/PHP_CodeSniffer/pull/657#issuecomment-15155...

Reopening, setting to RTBC as per #13.

Comment #15

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 11 November 2015 at 12:50

Status:

Reviewed & tested by the community

» Needs work

Woops patch needs an update to the composer.lock file.

Comment #16

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 11 November 2015 at 12:56

I don't think we need to update composer.lock - just copy in the changes for InlineControlStructureSniff from upstream.

Comment #17

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 11 November 2015 at 13:49

Assigned:

Unassigned

» pfrenssen

From taking a quick look I think it isn't needed to backport any changes, InlineControlStructureSniff::process() does a few checks and then defers the execution to the parent::process().

I'll double check to be sure.

Comment #18

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 11 November 2015 at 14:35

Assigned:	pfrenssen	» Unassigned
Status:	Needs work	» Needs review

File	Size
2530854-18.patch	11.62 KB
interdiff.txt	11.98 KB

2 files were hidden/shown/deleted

File	Size
2530854-10.patch	20.54 KB
interdiff.txt	3.51 KB

Rerolled the patch on the latest 8.x-2.x and updated the Composer dependencies. Ran the test suite and everything's green.

This now requires the PHP CodeSniffer to be updated to commit 139fe39bea7 on the master branch. Because of this it needs to be downloaded using git which makes the composer install operation quite a bit slower. Would it be preferable to wait until the next release of PHP CodeSniffer instead?

Comment #19

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 13 November 2015 at 14:43

Status:

Needs review

» Fixed

Committed, thanks!

I think it is ok to point to a PHPCS commit in the dev version of coder. We will switch back to a stable version once that is released and then create a coder release.