Git blacklist feature should not be limited to PHP files

[kgust]

If I'm reading the code correctly, you can only use Git Blacklist with PHP files. It would be very useful to allow this to work with other files types as well.

E.g. Javascript files with console.log(…) or debugger; statements.

https://github.com/phpro/grumphp/blob/5b6154df71be6bb3922b50600d54ae8d671375e4/src/GrumPHP/Task/Git/Blacklist.php#L54

[veewee]

That would indeed be a great enhancement. I feel that we need to rethink the blacklist task.

Another idea was to improve it as described in this feature: https://github.com/phpro/grumphp/issues/53

Maybe it's a good idea to define blacklisted words per extension or set of extensions. This way you are able to specify Javascript and PHP specific blacklisted words.

[llaville]

:+1: for this enhancement, but rather than used the token_get_all solution like in #53, consider to implement the famous PHP_Parser of Nikita. See project home page at https://github.com/nikic/PHP-Parser. I could be able to propose a such implement if you want !

[kgust]

You guys are talking about vastly different features.

1. Blacklist is currently, as I understand it, a simple string identification feature. I still believe there is a place for this. I was arguing that you need to open this up to other file types (especially Javascript).
2. What is being argued in the opposite direction, ala #53, is an introspection tool that can parse PHP and not allow certain constructs, syntax, etc. For this the PHP_Parser or something similar could be helpful.

Why not both? A PHP parser isn't going to be very helpful in detecting a javascript debugger; statement.

[llaville]

You right, original GitBlackList task is only a string key ident feature (only grep). PHP_Parser is able to scan only PHP scripts syntax, not Javascripts !

We could have to solve this issue a fix to allow BlackList task to allow other files than just .php And a new task that will support PHP_Parser to detect syntax like ... here are some ideas

        git_blacklist:
            keywords:
                - "var_export("
                - "->var_dump("
                - "::print_r("

Searching for php function var_export, no static method ->var_dump(, static method ::print_r( AST produces by PHP_Parser is able to do it easily !

[veewee]

+1 For separating the functionality.

The blacklist extensions could be made configurable like in the grunt task:

# grumphp.yml
parameters:
    tasks:
        git_blacklist:
            triggered_by: [php, js, ....]

This way the task has sensible defaults but could easily be customized.

@llaville Defenitly +1 for the PHP_Parser blacklist idea. Can you work something out in a separate issue? Thanks!

[llaville]

@veewee Yes, it's possible, especially because I need something at my office. So I'll begin it right soon and propose it in next days now

An idea about the new task name ?

[llaville]

Just finished to code my first version of new task, I've called phpparser following syntax I've previously declared in https://github.com/phpro/grumphp/issues/101#issuecomment-179433497

Go to sleep now, but I should be able to propose a PR tomorrow !

[veewee]

Great, I am Looking forward to see your code! The name is OK for me. I was thinking about something like invalid_php_tokens, but that one is just too long :) Can we continue discussing this feature in #53 to make sure that @kgust's question is also handled in a correct way?

[llaville]

ok will start comment in #53

[kgust]

Having had a chance to think about this more, I'd give a :+1: to @nilportugues comment on #53. At least in regard to intelligently parsing CSS and Javascript.

Simple regex blacklists have their uses, but as #53 points out, they are imprecise.