POC for the new php_blacklist task

[llaville]

A draft version to solve situation presented in #53 with the PHP_Parser rather than with php tokens.

[llaville]

Exemple with YAML config

parameters:
  bin_dir: "./vendor/bin"
  git_dir: "/var/grumphp/examples"
  tasks:
      php_blacklist:
        keywords:
          - "::var_export("
          - "var_dump("
          - "->print_r("
          - "error_log("

Searching for var_dump, error_log functions, static method call named var_export, concrete method call named print_r

[veewee]

Thanks for the PR. I will take a deeper look at it as soon as possible!
In the meantime: is it possible to add specs / unit tests (for the parser stuff)?
There is also a problem when using the lower dependencies. Check Travis for more information.
Maybe you could also update the documentation? This way we know how to use the task.

[llaville]

I know the problem with Travis job and I've check in. Issue come from PHP_CodeSniffer (version used 2.3.0).
See details on issue squizlabs/PHP_CodeSniffer#883 I've opened few minutes ago

Ok for other task todo (unit tests, ...) but I can do it only this week end now

[veewee]

Hi @llaville,

It looks pretty nice already, but I think there are some things we can improve
I took the time to browse through the code and made a list of small / larger remarks.
Here is a list per file:

GrumPHP\Task\ParseErrorsCollection

• Maybe it's best to introduce a ParseError object. Somewhat similar to the LintError` object.This makes it easier to transfer the error data in a non formatted way. In a later fase of the project we are planning to introduce formatter classes, which will make it easy to display the messages the way you want!

GrumPHP\Task\Php\Blacklist

• The triggered_by logic could be moved to the abstract task. It will always be the same for every parser.
• The triggered_by option should be empty in the abstract task but specified for this blacklist task.
• Not sure if we need to put the PhpBlacklist in a separate namespace. Most tasks are for PHP currently.
• You got a unused use statement.

GrumPHP\Task\AbstractParserTask

• I would just transfer the SplFileObject to the parser. No need to build a path there. It is best to keep objects for as long as possible.

GrumPHP\Parser\ParserInterface

• Make sure to add the ParseErrorCollection to the use statements and use the short name in the docblocks. This gives a better idea of the class/interface dependencies.

GrumPHP\Parser\Php\NodeVisitor

• I was thinking in splitting this visitor up into 3 different visitors. Not sure this is possible since I did not use the PhpParser package yet.
• It would be very cool if the visitors were registered as a service and there was an option 'visitors' added to the task. This way, a user could easily create their own visitors for the task or just use 1 or 2 of the provided methods. This one is probably not that easy, but would be very cool :)
• Information about how the keywords are parsed is a must! Otherwise people won't know how to use these.

GrumPHP\Parser\Php\PhpParser

• In the PhpParser library I saw they were using a factory for creating the parser. In this factory it was possible to say if it's PHP 5 or PHP 7 or a combination. This seems like a new option for the parser task to me.

TODO: Testing + documentation.

You really did a great job on this PR already, really appreciate it! Thanks!

[llaville]

Hi @veewee,

I really appreciate your deeply code review :)

I'll try to fix all points, but it should be propably for monday or tuesday now !

Thanks.

[veewee]

No problem! Take your time :)

[llaville]

I've already fixed some point, but I'm still working on other improvements. Especially the node visitors option registered as services.

[llaville]

@veewee About your comment

t would be very cool if the visitors were registered as a service and there was an option 'visitors' added to the task. This way, a user could easily create their own visitors for the task or just use 1 or 2 of the provided methods. This one is probably not that easy, but would be very cool :)

I've just implemented a solution, but I'm not sure it is that you've in mind

Current config will be:

      php_blacklist:
        keywords:
          - "::var_export("
          - "var_dump("
          - "->print_r("
          - "error_log("
          - "BAR"
        visitors:
          - 'GrumPHP\Parser\Php\MyNodeVisitor' 

If option visitors is not specified, the default GrumPHP\Parser\Php\NodeVisitor' (that checks for methods, functions) is added

I've create a new node visitor (e.g) to check class constant

[veewee]

Well I was thinking about splitting the visitors up into separate visitors:

• FunctionCallVisitor
• StaticCallVisitor
• MethodCallVisitor
• ConstantVisitor

Those visitors can be added as services in the container. Something like:

services:
    grumphp.parser.php.visitor.function_call:
        class: GrumPHP\Parser\Php\Visitor\FunctionCallVisitor
        tags: {name: php_parser.visitor}
                public: false

parameters:
    php_blacklist:
        keywords:
          - "::var_export("
          - "var_dump("
          - "->print_r("
          - "error_log("
          - "BAR"
        visitors:
          - '@grumphp.parser.php.visitor.function_call' 

This would require an additional collection class which contains a full list of all visitors on which it is possible to filter based on the configured visitors.
By default all your visitors can be enabled, but this will make it possible to easily create your own specific visitor for validating whatever you want.

[llaville]

ok thanks for feedback. I'll review my code copy !

[llaville]

I must admit, I've some difficulties to retrieve list of services corresponding to node visitors.

I've checked on ContainerFactory and two compilers examples, but no way to add a new one (an idea ?).

So how to get list of node visitors services ? Help would be greatly appreciated. Thanks in advance.

[llaville]

Probably via a new extension that is the only way we can configure it ?

[veewee]

Hello @llaville,

We don't need an extension for this.
Take a look at: http://symfony.com/doc/current/components/dependency_injection/tags.html
You can register your own compiler pass in the ContainerFactory that adds the visitors to a VisitorCollection in the Parser object.
Next you can filter by the configured filters.

So basically you can add a visitor service and tag is as a php.parser.visitor or something like that. The CompilerPass will load all the tagged visitors and append them to the php parser. When running the parser, the registered visitors are filtered by the configured parser options.

I am leaving on vacation today, but will be back in one week if you need additional help. Maybe someone else can help out in the meantime. (/cc @aderuwe? )

[llaville]

Ok finally it was as I've expected first. BTW, and in parallel I've worked on a copy that used visitors as class name strings.
I'll push the code right away, but I'll continue to fix it following your feedback (about compiler pass).

[llaville]

With such config

      php_blacklist:
        keywords:
          - "::var_export("
          - "var_dump("
          - "->print_r("
          - "->run("
          - "error_log("
          - "BAR"
        visitors:
          - 'GrumPHP\Parser\Php\Visitor\FunctionCallVisitor'
          - 'GrumPHP\Parser\Php\Visitor\ConcreteMethodCallVisitor'
          - 'GrumPHP\Parser\Php\Visitor\StaticMethodCallVisitor'

Visitors are registered as string class names (not yet services: WIP), I've results expected.
For example:

You have blacklisted keywords in your commit:
[WARNING] C:\var\grumphp\examples\task\Phpparser.php: Found "var_dump" function call on line 61
[WARNING] C:\var\grumphp\examples\task\sample2.php: Found "var_export" static method call on line 21
[ERROR] C:\var\grumphp\examples\task\syntax_error.php: Syntax error, unexpected T_PUBLIC, expecting ',' or ';' on line 10

Last line is produced by PHP_Parser that act as a php -l lint command.

[llaville]

Just added visitors as services implementation. See commit 7e6936f
Config is now

      php_blacklist:
        keywords:
          - "::var_export("
          - "var_dump("
          - "->print_r("
          - "->run("
          - "error_log("
          - "BAR"
        visitors:
          - '@grumphp.parser.php.visitor.function_call'
          - '@grumphp.parser.php.visitor.concrete_method_call'
          - '@grumphp.parser.php.visitor.static_method_call'

[veewee]

Thanks for the work! I will try to take the time to review your code at the end of the week.

[llaville]

Ok waiting for your feedback that have a great importance for me !
I've really appreciated your deeply code review in past

[veewee]

Hi @llaville,

Sorry it took me so long, but I didn't find the time this weekend.
As promised I took a look at the code and got som remarks:

• I am starting to think that blacklist isn't the best name for the task. Since it is possible to validate whatever you want with the visitors. Maybe we could just name it php_parser with a configurable option blacklist? this way it is possible to add other configurable checks in the future, but keep the parser as is. What do you think about this?

• I really don't want the PathsHelper in any Context object. Other commands don't need another path then the one that is available. Is this really needed? Maybe we could move some of the helper logic to a separate Filesystem Util class and inject this Filesystem in the parser itself? Also it shouldn't be the contexts responsibility to make files relative.

• In the visitors you are using if's in if's. Maybe it's better to inverse the if statement and return early. You can find some more information on the subject at: www.slideshare.net/rdohms/bettercode-phpbenelux212alternate (Don't mind the title ;-) )

• The visitors could be renamed to for example ConcreteMethodBlacklistVisitor. This way you can add multiple concrete method visitors with other functionality. I was also thinking into make the configuration a bit different to skip the init() step. Maybe something like this:

  php_parser:
    visitors:
        {visitor: '@grumphp.parser.php.visitor.blacklist_function_call', options: {blacklist: ["var_dump", "error_log"]}}
        {visitor: '@grumphp.parser.php.visitor.blacklist_concrete_method_call', options: {blacklist: ["print_r", "run"]}}
        {visitor: '@grumphp.parser.php.visitor.blacklist_static_method_call', options: {blacklist: ["var_export"]}}
  

What do you think about this way of configuring? Do you think it's possible? This will enable people to create their own kick-ass visitors. Looks pretty cool to me.

• Currently the ParserInterface has visitors, but this is propably only used with the PHP parser? So it doesn't make much since to me. Also the AbstractParser should not contain visitors IMO.
• The compiler pass might want to add the node visitors directly to the php parser instead of the blacklist task? Next the configuration will tell which one is enabled and which configuration it should use as described in the YAML above.
• Currently also tests are missing.

[veewee]

Hi @llaville,

It's been a while since your last worked on this PR. We are assembling features for our next 0.9.0 relase and I was wondering if you needed some additional help with this PR?

[llaville]

@veewee Sorry I've been busy at Office, and have not enough free time to work on this subject.
But I'll have a look this week-end to make it advance. I've to learn phpspec to build unit tests too !
A little challenge, but nothing I can't do

[llaville]

@veewee I've just pushed new PR #138 related to code changes

• renamed task to php_parser rather than php_blacklist
• spec tests are only available on my local host (not yet fully operational)
• visitors configuration, as you suggested is impossible for me. I propose a new way

parameters:
    tasks:
        php_parser:
            visitors_options:
              function_call_visitor:
                blacklist: ["var_dump", "error_log"]
                whitelist: ["count", "get_class"]
              concrete_method_call_visitor:
                blacklist: ["print_r", "run"]
              static_method_call_visitor:
                blacklist: ["var_export"]
            visitors:
              - {visitor: '@grumphp.parser.php.visitor.function_call'}
              - {visitor: '@grumphp.parser.php.visitor.concrete_method_call'}
              - {visitor: '@grumphp.parser.php.visitor.static_method_call'}
            triggered_by: [php]
            ignore_patterns: []

[veewee]

This PR is being continued in #138.