Fix usage with grouped constant statements (#219)

humbug · Jun 9, 2018 · a503f54 · a503f54
1 parent d56d480
commit a503f54
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 26 deletions.
diff --git a/README.md b/README.md
@@ -51,6 +51,7 @@ potentially very difficult to debug due to dissimilar or unsupported package ver
     - [PSR-0 support](#psr-0-support)
     - [String values](#string-values)
     - [Native functions and constants shadowing](#native-functions-shadowing)
+    - [Grouped constants whitelisting](#grouped-constants-whitelisting)
     - [Composer](#composer)
     - [Composer Plugins](#composer-plugins)
 - [Contributing](#contributing)
@@ -543,6 +544,23 @@ is_array([]);
 The situation is exactly the same for constants.
 
 
+### Grouped constants whitelisting
+
+When a grouped constant declaration like the following is given:
+
+```php
+const X = 'foo', Y = 'bar';
+```
+
+PHP-Scoper will not be able to whitelist either `X` or `Y`. The statement
+above should be replaced by multiple constant statements:
+
+```php
+const X = 'foo';
+const Y = 'bar';
+```
+
+
 ### Composer
 
 PHP-Scoper does not support prefixing the dumped Composer autoloader and autoloading files. This is why you have to

diff --git a/specs/const/const-declaration.php b/specs/const/const-declaration.php
@@ -26,6 +26,7 @@
 <?php
 
 const FOO_CONST = foo();
+const X = 'x', Y = '';
 define('BAR_CONST', foo());
 define('Acme\BAR_CONST', foo());
 define(FOO_CONST, foo());
@@ -37,6 +38,7 @@
 namespace Humbug;
 
 const FOO_CONST = \Humbug\foo();
+const X = 'x', Y = '';
 \define('BAR_CONST', \Humbug\foo());
 \define('Humbug\\Acme\\BAR_CONST', \Humbug\foo());
 \define(\Humbug\FOO_CONST, \Humbug\foo());
@@ -52,6 +54,7 @@
 <?php
 
 const FOO_CONST = foo();
+const X = 'x', Y = '';
 define('BAR_CONST', foo());
 define('Acme\BAR_CONST', foo());
 define(FOO_CONST, foo());
@@ -62,6 +65,7 @@
 
 namespace {
     const FOO_CONST = \foo();
+    const X = 'x', Y = '';
     \define('BAR_CONST', \foo());
     \define('Acme\\BAR_CONST', \foo());
     \define(\FOO_CONST, \foo());
@@ -78,6 +82,7 @@
 <?php
 
 const FOO_CONST = foo();
+const X = 'x', Y = '';
 define('BAR_CONST', foo());
 define('Acme\BAR_CONST', foo());
 define(FOO_CONST, foo());
@@ -89,12 +94,23 @@
 namespace Humbug;
 
 \define('FOO_CONST', \Humbug\foo());
+const X = 'x', Y = '';
 \define('BAR_CONST', \Humbug\foo());
 \define('Acme\\BAR_CONST', \Humbug\foo());
 \define(\FOO_CONST, \Humbug\foo());
 \define(\FOO_CONST, \Humbug\foo());
 \define(\Acme\BAR_CONST, \Humbug\foo());
 
+PHP
+    ],
+
+    'Whitelisted grouped constants declaration in the global namespace' => [
+        'whitelist' => ['X'],
+        'payload' => <<<'PHP'
+<?php
+
+const X = 'x', Y = '';
+----
 PHP
     ],
 
@@ -105,6 +121,7 @@
 namespace Acme;
 
 const FOO_CONST = foo();
+const X = 'x', Y = '';
 define('BAR_CONST', foo());
 define('Acme\BAR_CONST', foo());
 define(FOO_CONST, foo());
@@ -116,6 +133,7 @@
 namespace Humbug\Acme;
 
 const FOO_CONST = foo();
+const X = 'x', Y = '';
 \define('BAR_CONST', foo());
 \define('Humbug\\Acme\\BAR_CONST', foo());
 \define(FOO_CONST, foo());
@@ -133,6 +151,7 @@
 namespace Acme;
 
 const FOO_CONST = foo();
+const X = 'x', Y = '';
 define('BAR_CONST', foo());
 define('Acme\BAR_CONST', foo());
 define(FOO_CONST, foo());
@@ -144,6 +163,7 @@
 namespace Acme;
 
 const FOO_CONST = foo();
+const X = 'x', Y = '';
 \define('BAR_CONST', foo());
 \define('Acme\\BAR_CONST', foo());
 \define(FOO_CONST, foo());
@@ -161,6 +181,7 @@
 namespace Acme;
 
 const FOO_CONST = foo();
+const X = 'x', Y = '';
 define('BAR_CONST', foo());
 define('Acme\BAR_CONST', foo());
 define(FOO_CONST, foo());
@@ -172,6 +193,7 @@
 namespace Humbug\Acme;
 
 const FOO_CONST = foo();
+const X = 'x', Y = '';
 \define('BAR_CONST', foo());
 \define('Acme\\BAR_CONST', foo());
 \define(FOO_CONST, foo());

diff --git a/src/PhpParser/NodeVisitor/ConstStmtReplacer.php b/src/PhpParser/NodeVisitor/ConstStmtReplacer.php
@@ -20,10 +20,11 @@
 use PhpParser\Node\Expr\FuncCall;
 use PhpParser\Node\Name;
 use PhpParser\Node\Name\FullyQualified;
-use PhpParser\Node\Param;
 use PhpParser\Node\Scalar\String_;
 use PhpParser\Node\Stmt\Expression;
 use PhpParser\NodeVisitorAbstract;
+use UnexpectedValueException;
+use function count;
 
 /**
  * Replaces const declaration by define.
@@ -62,31 +63,38 @@ public function enterNode(Node $node): Node
             return $node;
         }
 
-        // TODO: check this: when can Node\Stmt\Const_ be empty or have more than one constant
-        /** @var Node\Const_ $constant */
-        $constant = current($node->consts);
+        foreach ($node->consts as $constant) {
+            /** @var Node\Const_ $constant */
+            $resolvedConstantName = $this->nameResolver->resolveName(
+                new Name(
+                    (string) $constant->name,
+                    $node->getAttributes()
+                )
+            )->getName();
 
-        $resolvedConstantName = $this->nameResolver->resolveName(
-            new Name(
-                (string) $constant->name,
-                $node->getAttributes()  // Take the parent node attribute since no "parent" attribute is recorded in
-                                        // Node\Const_
-                                        // TODO: check with nikic if this is expected
-            )
-        )->getName();
+            if (false === $this->whitelist->isClassWhitelisted((string) $resolvedConstantName)) {
+                continue;
+            }
 
-        if (false === $this->whitelist->isClassWhitelisted((string) $resolvedConstantName)) {
-            return $node;
+            if (count($node->consts) > 1) {
+                throw new UnexpectedValueException(
+                    'Whitelisting a constant declared in a grouped constant statement (e.g. `const FOO = '
+                    .'\'foo\', BAR = \'bar\'; is not supported. Consider breaking it down in multiple constant '
+                    .'declaration statements'
+                );
+            }
+
+            return new Expression(
+                new FuncCall(
+                    new FullyQualified('define'),
+                    [
+                        new String_((string) $resolvedConstantName),
+                        $constant->value,
+                    ]
+                )
+            );
         }
 
-        return new Expression(
-            new FuncCall(
-                new FullyQualified('define'),
-                [
-                    new String_((string) $resolvedConstantName),
-                    $constant->value,
-                ]
-            )
-        );
+        return $node;
     }
 }
diff --git a/tests/Scoper/PhpScoperTest.php b/tests/Scoper/PhpScoperTest.php
@@ -36,6 +36,7 @@
 use Roave\BetterReflection\SourceLocator\Type\StringSourceLocator;
 use Symfony\Component\Finder\Finder;
 use Throwable;
+use UnexpectedValueException;
 use function Humbug\PhpScoper\create_fake_patcher;
 use function Humbug\PhpScoper\create_parser;
 use function implode;
@@ -422,7 +423,7 @@ function (...$args) use (&$i): bool {
     /**
      * @dataProvider provideValidFiles
      */
-    public function test_can_scope_valid_files(string $spec, string $contents, string $prefix, Whitelist $whitelist, string $expected)
+    public function test_can_scope_valid_files(string $spec, string $contents, string $prefix, Whitelist $whitelist, ?string $expected)
     {
         $filePath = 'file.php';
 
@@ -448,7 +449,21 @@ public function test_can_scope_valid_files(string $spec, string $contents, strin
             )
         );
 
-        $actual = $this->scoper->scope($filePath, $contents, $prefix, $patchers, $whitelist);
+        try {
+            $actual = $this->scoper->scope($filePath, $contents, $prefix, $patchers, $whitelist);
+
+            if (null === $expected) {
+                $this->fail('Expected exception to be thrown.');
+            }
+        } catch (UnexpectedValueException $exception) {
+            if (null !== $expected) {
+                throw $exception;
+            }
+
+            $this->assertTrue(true);
+
+            return;
+        }
 
         $formattedWhitelist = 0 === count($whitelist)
             ? '[]'
@@ -559,7 +574,7 @@ private function parseSpecFile(array $meta, $fixtureTitle, $fixtureSet): Generat
                 $fixtureSet['whitelist-global-constants'] ?? $meta['whitelist-global-constants'],
                 ...($fixtureSet['whitelist'] ?? $meta['whitelist'])
             ),
-            $payloadParts[1],   // Expected output
+            '' === $payloadParts[1] ? null : $payloadParts[1],   // Expected output
         ];
     }
 }