Create namespace for whitelisted classes #138

moorscode · 2018-01-05T13:37:56Z

Technical choices:

This is the simplest solution to the problem we could think of.

Global namespace

~~Decided to not put classes in a global namespace that are not whitelisted in a file, which contains a whitelisted class.~~

~~We have tried this, but it resulted in having to put every class in a namespace { } wrap, which fails a lot of tests.~~

As PSR-0 declares:
This means each class is in a file by itself, and is in a namespace of at least one level: a top-level vendor name.

Updated the functionality to make sure any code that is not wrapped in a namespace will receive the global namespace. This prevents invalid PHP from being generated.

Paired on it together with @atimmer

Fixes #135

moorscode · 2018-01-05T14:20:54Z

While testing this code in a project, we ran across the problem that defines are placed outside the namespace.

This is causing fatal errors.

Placing the define inside the namespace resolves the error.

moorscode · 2018-01-05T14:24:24Z

Having to resolve this, I think we need to wrap -anything- in a namespace group as this is the method of prefixing used in this tool.

This would result in:

// Global namespace define
namespace {
    define('SOME_DEFINE', 'value');
}

// Prefixed class
namespace Humbug {
    class b {}
}

// Global namespace class
namespace {
    class c {}
}

theofidry

Big 👍

I added comments; a few are regarding some missing tests and others are mostly nitpicks.

Huge thanks for this one 😄

theofidry · 2018-01-06T09:27:16Z

specs/namespace/creation-for-whitelist.php

+    ],
+
+    [
+        'spec' => <<<'SPEC'


good use of the format:

[ 'spec' => '', 'payload' => '', ]

but I think it's a case simple enough where we can use the simplified format:

'spec content' => 'payload content'

As you can find here

theofidry · 2018-01-06T09:30:02Z

specs/namespace/creation-for-whitelist.php

+    ],
+
+    [
+        'spec' => <<<'SPEC'


I would add two more cases:

One with multiple different namespaces like you can find here (it doesn't need to be the same it's just for the idea)

One with two classes belonging to the global namespace: one which is a whitelisted one, so AppKernel which as a result will be prefixed, and another which is not like Closure which should not be prefixed

theofidry · 2018-01-06T09:31:45Z

src/NodeVisitor/NamespaceStmtCreator.php

+use PhpParser\NodeVisitorAbstract;
+
+/**
+ * Creates a namespace for whitelisted classes.


[...] belonging to the global namespace. It is an important part as the whitelisting feature is different depending of if the class belongs or not to the global namespace cf. global-namespace-whitelisting and whitelist

theofidry · 2018-01-06T09:39:41Z

src/NodeVisitor/NamespaceStmtCreator.php

+final class NamespaceStmtCreator extends NodeVisitorAbstract
+{
+    private $prefix;
+


let's remove the blank lines between those properties

Added those because the code style checker was marking them as errors. Will remove the lines.

Ah right. I need to figure out which parameter in the CS fixer it is to disable that, it really annoys me

theofidry · 2018-01-06T09:40:39Z

src/NodeVisitor/NamespaceStmtCreator.php

+     * @param string                  $prefix
+     * @param NamespaceStmtCollection $namespaceStatements
+     * @param callable                $globalWhitelister
+     */


the doc here is not useful as completely redundant with the signature, I know I may not have been consistent everywhere but I would remove it still :)

theofidry · 2018-01-06T09:41:28Z

src/NodeVisitor/NamespaceStmtCreator.php

+    {
+        return ($node instanceof Class_)
+            ? $this->addNamespaceStmt($node)
+            : $node;


return ($node instanceof Class_) ? $this->addNamespaceStmt($node) : $node ;

theofidry · 2018-01-06T09:41:58Z

src/NodeVisitor/NamespaceStmtCreator.php

+    }
+
+    /**
+     * @param Class_ $node


same here, useless phpdoc

theofidry · 2018-01-06T09:46:01Z

specs/namespace/creation-for-whitelist.php

@@ -0,0 +1,85 @@
+<?php


There is also a few other cases that you can find in specs/class that needs to be accounted for. For example it should also apply to:

abstract classes

interfaces

~~traits~~: they cannot be whitelisted; may require a test that it doesn't work there still

~~anonymous~~: they cannot be whitelisted; may require a test that it doesn't work there still

moorscode · 2018-01-06T11:06:41Z

Additional functionality:

I've used the beforeTraverse to determine if this "scope/file" has any whitelisted elements. If so, anything not white-listed needs to have a namespace.

If no namespace exists, the global namespace will be wrapped around it.

This resolves the problems described before.

moorscode · 2018-01-06T11:44:18Z

(Sorry I'm not so good at squashing commits and pushing after everything is done ;))

theofidry · 2018-01-06T11:45:49Z

(Sorry I'm not so good at squashing commits and pushing after everything is done ;))

Don't worry about it I'll do a "Squash & Merge", only the CI may resent you a bit if you push them one by one but I think Travis & Scrutinizer got better at it

theofidry · 2018-01-06T11:50:43Z

src/NodeVisitor/NamespaceStmtCreator.php

+    }
+
+    /**
+     * @param Node[] $nodes List of nodes to find whitelisted nodes from.


I think the signature is descriptive enough, no need for the List [...] comment. I would keep the phpdoc as it does add the type array, but I tend to avoid comments when the code is self-explanatory as the problem with comments/docs is that they easily get out of date with the code they are documenting. So keeping it to minimal is better IMO

Do you prefer to have an empty docblock or just none at all?

In the projects we do at Yoast, we have very strict documentation rules.
We don't find ourselves having a lot of outdated documentation entries in general.

The fact that we are in a WordPress environment, where backwards compatibility is a major thing might have something to do with it, but I don't think that is the only factor in it.

Writing the documentation mostly helps me re-evaluate what I wrote functionally and if the naming of the variables and logic are actually the best solutions.

My two cents on the subject.

I'm happy to oblige to the standards and approach you set for your project.

I think @param Node[] $nodes is enough here

If it's too "redundant" with the code, I really prefer to avoid the comments. For public interfaces, non trivial things or for example the doc to explain the purpose of the class I tend to be in favour comments :)

theofidry · 2018-01-06T11:50:52Z

src/NodeVisitor/NamespaceStmtCreator.php

+    }
+
+    /**
+     * @return bool


no need for the phpdoc here

theofidry · 2018-01-06T11:51:44Z

src/NodeVisitor/NamespaceStmtCreator.php

+            return null !== $node->name && ($this->globalWhitelister)($node->name);
+        });
+
+        return !empty($nodes);


as the returned type is an array I think [] !== $nodes or with a count is more expressive here

theofidry · 2018-01-06T11:52:21Z

src/NodeVisitor/NamespaceStmtCreator.php

+    public function beforeTraverse(array $nodes)
+    {
+        $classes = array_filter($nodes, [$this, 'isWhitelistableNode']);
+        if (empty($classes)) {


as the returned type is an array I think [] !== $nodes or with a count is more expressive here

moorscode · 2018-01-06T12:04:53Z

While writing specs, I came across another problem when a global namespace is already defined around a class.
It does not seem to be in the scope of this feature, so I decided not to add it in this PR:

'Traits in different namespace.' => <<<'PHP'
<?php

namespace Foo {
    trait SomeTrait{}
}

namespace {
    class AppKernel{}
}

----
<?php

namespace Humbug\Foo{
    trait SomeTrait
    {
    }
}
namespace Humbug {
    class AppKernel
    {
    }
}

PHP

As the namespace prefixer should pick this up and add the namespace for the whitelisted class in the global namespace.

theofidry · 2018-01-06T12:09:17Z

You mean right now we would have:

<?php

namespace Foo {
    trait SomeTrait{}
}

namespace {
    class AppKernel{}
}

----
<?php

namespace Humbug\Foo{
    trait SomeTrait
    {
    }
}
namespace Humbug {
    class AppKernel
    {
    }
}

instead of having canonical namespaces, i.e. only one Humbug namespace declaration? If that's the case IMO there is nothing to do here it works fine https://3v4l.org/DCjXW

moorscode · 2018-01-06T12:20:27Z

Hope this output helps to describe it a bit better.
The term whitelist gets a bit confusing as it whitelists from receiving the prefix..

I would expect the AppKernel (whitelisted node name) to be prefixed.
But as it already has a namespace, which is global, it is ignored in the Namespace Statement Prefixer: https://github.com/humbug/php-scoper/blob/master/src/NodeVisitor/NamespaceStmtPrefixer.php#L79

================================================================================
INPUT
================================================================================
<?php

namespace Foo {
    trait SomeTrait{}
}

namespace {
    class AppKernel{}
}

namespace {
    class Bla{}
}


================================================================================
EXPECTED
================================================================================
<?php

namespace Humbug\Foo {
    trait SomeTrait
    {
    }
}
namespace Humbug {
    class AppKernel
    {
    }
}
namespace {
    class Bla
    {
    }
}


================================================================================
ACTUAL
================================================================================
<?php

namespace Humbug\Foo {
    trait SomeTrait
    {
    }
}
namespace {
    class AppKernel
    {
    }
}
namespace {
    class Bla
    {
    }
}

theofidry · 2018-01-06T12:26:07Z

We're clear on the expected input/expected output.

I wonder if the work you're doing shouldn't be in the NamespaceStmtPrefixer as otherwise, this one is missing the necessary context to know if the namespace should be prefixed or not

moorscode · 2018-01-06T13:53:52Z

We're clear on the expected input/expected output; But I wonder if the work you're doing shouldn't be in the NamespaceStmtPrefixer as otherwise, this one is missing the necessary context to know if the namespace should be prefixed or not

That's a good point, I initially started a new visitor because prefixing and creating seemed like two concepts.
As the global namespace doesn't have to be defined but is always present, prefixing it and making it explicit fits in the prefix concept when looking at it that way.

moorscode · 2018-01-06T14:41:04Z

Updated the code and added the desired StyleCI disables to remove the checks on property separation and const separation.

moorscode · 2018-01-06T14:43:26Z

Also put the StyleCI changes in a separate PR: #139

theofidry

Awesome! Could you rebase it (for the CS failure)?

theofidry · 2018-01-07T22:26:40Z

src/NodeVisitor/NamespaceStmtPrefixer.php


    /**
     * @param string                  $prefix
     * @param NamespaceStmtCollection $namespaceStatements
+     * @param callable                $globalWhitelister


let's just remove the phpdoc here

theofidry · 2018-01-07T22:28:13Z

specs/namespace/creation-for-whitelist.php

+    'Defines should be wrapped in namespace alongside whitelisted class.' => <<<'PHP'
+<?php
+
+define( "MY_DEFINE", "value" );


small extra space here :)

Make sure all code is wrapped in global namespace if a whitelisted class is in the same file.

As the global namespace is implicitly always present, we are making it explicit when needed by adding the statement to the stack. Added extra test for complex situation with non-whitespaced class in combination with whitespaced class.

Simplify by combining two if-statements

theofidry · 2018-01-08T09:17:10Z

Many thanks @moorscode, that was a critical bug :)

theofidry requested changes Jan 6, 2018

View reviewed changes

theofidry reviewed Jan 6, 2018

View reviewed changes

theofidry requested changes Jan 7, 2018

View reviewed changes

moorscode added 14 commits January 8, 2018 09:09

Create namespace for whitelisted classes

246054a

Tab -> spaces

528472f

Apply PSR-2

fc0e756

PSR-2

b0a919d

Fix code style problems

2f79890

Fix styling problems

05fb35d

Namespace anything around a whitelisted class

620c4ed

Make sure all code is wrapped in global namespace if a whitelisted class is in the same file.

Refactor and split up code

fc75eb5

Apply CR

f1da747

Remove unintended spec

00a19ce

Apply CS

0695fcb

Added more interface specs

9240fa8

Follow scrutinizer suggestion

2cfedb0

Node has parent always means don't wrap.

84c34ab

moorscode added 10 commits January 8, 2018 09:09

Added tests for traits and anonymous classes

f4f77e1

Added test for abstract class

4fc6221

Add final class spec

bafa89f

Remove unused argument

fae405a

Check for empty array instead of general empty

1eb03b2

Remove self-explanatory documentation

2615e96

Add simple description of parameter

6c26bb7

Move logic into Namespace Statement Prefixer

2fd6d5d

As the global namespace is implicitly always present, we are making it explicit when needed by adding the statement to the stack. Added extra test for complex situation with non-whitespaced class in combination with whitespaced class.

Fix code style

afd7ecc

Reduce complexity

e4e0849

Simplify by combining two if-statements

moorscode force-pushed the 135-create-namespace-for-whitelisted-class branch from 381286a to e4e0849 Compare January 8, 2018 08:16

Apply CR

43815c1

theofidry merged commit 674e24d into humbug:master Jan 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create namespace for whitelisted classes #138

Create namespace for whitelisted classes #138

moorscode commented Jan 5, 2018 •

edited

Loading

moorscode commented Jan 5, 2018

moorscode commented Jan 5, 2018 •

edited

Loading

theofidry left a comment

theofidry Jan 6, 2018

theofidry Jan 6, 2018

theofidry Jan 6, 2018

theofidry Jan 6, 2018

moorscode Jan 6, 2018

theofidry Jan 6, 2018 •

edited

Loading

theofidry Jan 6, 2018

theofidry Jan 6, 2018

theofidry Jan 6, 2018

theofidry Jan 6, 2018

moorscode commented Jan 6, 2018

moorscode commented Jan 6, 2018

theofidry commented Jan 6, 2018

theofidry Jan 6, 2018

moorscode Jan 6, 2018

moorscode Jan 6, 2018

theofidry Jan 6, 2018

theofidry Jan 6, 2018

theofidry Jan 6, 2018

theofidry Jan 6, 2018

moorscode commented Jan 6, 2018 •

edited

Loading

theofidry commented Jan 6, 2018

moorscode commented Jan 6, 2018

theofidry commented Jan 6, 2018 •

edited

Loading

moorscode commented Jan 6, 2018

moorscode commented Jan 6, 2018

moorscode commented Jan 6, 2018 •

edited

Loading

theofidry left a comment

theofidry Jan 7, 2018

theofidry Jan 7, 2018

theofidry commented Jan 8, 2018

Create namespace for whitelisted classes #138

Create namespace for whitelisted classes #138

Conversation

moorscode commented Jan 5, 2018 • edited Loading

Global namespace

moorscode commented Jan 5, 2018

moorscode commented Jan 5, 2018 • edited Loading

theofidry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorscode commented Jan 6, 2018

moorscode commented Jan 6, 2018

theofidry commented Jan 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorscode commented Jan 6, 2018 • edited Loading

theofidry commented Jan 6, 2018

moorscode commented Jan 6, 2018

theofidry commented Jan 6, 2018 • edited Loading

moorscode commented Jan 6, 2018

moorscode commented Jan 6, 2018

moorscode commented Jan 6, 2018 • edited Loading

theofidry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Jan 8, 2018

moorscode commented Jan 5, 2018 •

edited

Loading

moorscode commented Jan 5, 2018 •

edited

Loading

theofidry Jan 6, 2018 •

edited

Loading

moorscode commented Jan 6, 2018 •

edited

Loading

theofidry commented Jan 6, 2018 •

edited

Loading

moorscode commented Jan 6, 2018 •

edited

Loading