Feature/php parser task #138

llaville · 2016-04-05T18:38:28Z

Q	A
Branch	master for features and deprecations
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	not yet
Documented?	no
Fixed tickets	comma-separated list of tickets fixed by the PR, if any

…ified for this blacklist task

…se the short name in the docblocks

… php_parser task/branch

veewee · 2016-04-06T04:58:15Z

doc/tasks/php_parser.md

+            visitors_options:
+              function_call_visitor:
+                blacklist: ["var_dump", "error_log"]
+                whitelist: ["count", "get_class"]


What exactly is the difference between blacklist and whitelist. Aren't whitelist items allowed by default?
In the code I see that whitelist returns a notice. But the notice will still make the task fail?
I am not sure what this should do :)

whitelist was a try to distinct two groups but, you're right the task fail anyway, so it's not a nice try !

veewee · 2016-04-06T04:58:56Z

doc/tasks/php_parser.md

+            ignore_patterns: []
+```
+
+**visitors_options**


Maybe you could describe what options you can configure per visitor?

Each class visitor as it's own configuration:

When class named Namespace\To\ClassFooVisitor the option entry should underscore named as base class name. In this example it will be class_foo_visitor

veewee · 2016-04-06T05:34:24Z

HI @llaville,

The PR is starting to look pretty good. I made some remarks about the code.
We will continuing discussing the feature in this PR, so I will close the other one.

I think there is something struturally wrong with how the visitors are added, initialized and configured.
I was thinking about using Symfony service configurators as described here:
http://symfony.com/doc/current/components/dependency_injection/configurators.html

Maybe this could be the solution to the problems we are facing?
It will also make it possible to configure the visitors as described in the previous threat. I think the current implemented visitor_options will make the configuration confusing.

One other thing I don't like about current visitors implementation is the constructor and init() method. It looks like both the runtime configuration, the file and the errorlist should be added through the constructor. So maybe the visitors aren't really services, but factory services that can create a vistor during runtime?

Can you give me your feedback on these remarks so that we can get the architecture of the parser right?
Thanks!

llaville · 2016-04-06T05:58:19Z

Hi @veewee

Thanks again for all your remarks and code review. I appreciate a lot !

I'll reply to you ASAP.

llaville · 2016-04-08T18:09:16Z

Hello @veewee
I need your help about phpspec usage.
I've wrote some spec files but I can't solve an issue. Perharps you could help me. Here are the ouput I got on my windows platform with PHP 5.6.19

GrumPHP\Collection\FilesCollection
 101  - it should filter by date
      warning: strtotime(): It is not safe to rely on the system's timezone settings. You are *required* to use the
      date.timezone setting or the date_default_timezone_set() function. In case you used any of those methods and you are
      still getting this warning, you most likely misspelled the timezone identifier. We selected the timezone 'UTC' for now,
      but please set date.timezone to select your timezone. in
      C:\home\github\grumphp\spec\GrumPHP\Collection\FilesCollectionSpec.php line 107

GrumPHP\Task\PhpParser
  23  - it should have a name
      exception <label>[exc:GrumPHP\Exception\RuntimeException("The php_parser can't run on your system. Please install
all dependencies.")]</label> has been thrown.

GrumPHP\Task\PhpParser
  28  - it should have configurable options
      exception <label>[exc:GrumPHP\Exception\RuntimeException("The php_parser can't run on your system. Please install
all dependencies.")]</label> has been thrown.

GrumPHP\Task\PhpParser
  38  - it should run in git pre commit context
      exception <label>[exc:GrumPHP\Exception\RuntimeException("The php_parser can't run on your system. Please install
all dependencies.")]</label> has been thrown.

GrumPHP\Task\PhpParser
  43  - it should run in run context
      exception <label>[exc:GrumPHP\Exception\RuntimeException("The php_parser can't run on your system. Please install
all dependencies.")]</label> has been thrown.

GrumPHP\Task\PhpParser
  20  - it should handle ignore patterns
      exception <label>[exc:GrumPHP\Exception\RuntimeException("The php_parser can't run on your system. Please install
all dependencies.")]</label> has been thrown.

                                      98%                                        397
61 specs
397 examples (391 passed, 6 broken)
13046ms

I've tried to set timezone to UTC, but I got same result (with or without)

llaville · 2016-04-08T19:07:50Z

@veewee Forget my previous help request about spec files. I've solved the first broken spec about timezone settings.

I got another PHP in my path that was used but timezone was not defined in UTC on php.ini !

veewee · 2016-04-09T10:29:47Z

Dus you also solve the php_parser can't run on your system error? You probably have to add the parser to the dev dependences in composer.

llaville · 2016-04-11T08:05:12Z

I'll push the code updated this evening, when I'll be back from Office

veewee · 2016-08-28T13:17:59Z

@llaville Are you planning to finish this PR any time soon? Thanks!

veewee · 2016-10-26T17:05:12Z

@llaville
For your info: I've started a little branch myself:
veewee@e5de05c

Maybe the task has to be configured through services in a first version. I'll try to work on it again when I find some spare time again :)
Feel free to add your remarks.

veewee · 2016-10-27T14:46:30Z

We'll continue integrating this task in #218.
Thanks for your work, I managed to reuse quite a lot of it.

llaville added 27 commits February 4, 2016 22:01

POC for the new php_blacklist task

288c557

fixed PSR2 CS

bb950f5

fixed PSR-2 CS

8560efd

Merge branch 'master' into feature/php_blacklist_task

821f304

remove unused use statement

77e39a8

The triggered_by option should be empty in the abstract task but spec…

7b03ac8

…ified for this blacklist task

Make sure to add the ParseErrorCollection to the use statements and u…

09574c1

…se the short name in the docblocks

easy way to filter based on extensions (see PR 115 by @veewee)

edb335b

keywords must be common to all php parser

539e5d2

ability to provide full real path on each item of filesCollection

b00fa10

php_blacklist task used now the SplFileObject instead of filename

830aa78

common parser task used now the SplFileObject instead of filename

fd2389f

RunContext may use now the PathHelper to get more info on each file

24a1685

introduce a ParseError object

993272a

split nodes into separate features

8c44df5

remove hard coded node visitors classes

dc28f7a

improve error location on php lint while building AST

3c16c84

use now visitors as services registered (see VisitorCompilerPass)

7e6936f

introduce documentation for new task php_blacklist

f10c2ef

to fix Travis and AppVeyor jobs failures. See also GH-118

716c2ab

fixe PSR-2 CS

3028685

fix compatibility with existing unit tests

735e507

Merge branch 'master' into feature/php_blacklist_task

8bdd6a6

rename task from php_blacklist to php_parser

5c9425b

remove unused Use statement

8e97fbc

[REFACTOR] update code from old php_blacklist task/branch to new code…

1d44554

… php_parser task/branch

merge master branch

e12f1b8

llaville mentioned this pull request Apr 5, 2016

POC for the new php_blacklist task #108

Closed

[FIX] PSR-2 cs

34d3ce1

veewee reviewed Apr 6, 2016
View reviewed changes

llaville added 4 commits April 7, 2016 07:58

[FIX] path is no more available in any context

b7b5043

[REFACTOR] visitors and visitors_options are php parser specific

2ea6ae9

[FIX] summary message

174a6ab

[FIX] run method always return a TaskResultInterface

882d34e

llaville added 2 commits April 9, 2016 16:45

init spec files for php_parser task

499d395

temporary deactivated php parser install checker

7685c45

veewee mentioned this pull request Aug 10, 2016

Bug in Blacklist task #171

Closed

veewee mentioned this pull request Oct 27, 2016

Php_parser task #218

Merged

18 tasks

veewee closed this Oct 27, 2016

llaville deleted the feature/php_parser_task branch May 3, 2017 04:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/php parser task #138

Feature/php parser task #138

llaville commented Apr 5, 2016

veewee Apr 6, 2016

llaville Apr 6, 2016

veewee Apr 6, 2016

llaville Apr 6, 2016

veewee commented Apr 6, 2016

llaville commented Apr 6, 2016

llaville commented Apr 8, 2016

llaville commented Apr 8, 2016

veewee commented Apr 9, 2016

llaville commented Apr 11, 2016

veewee commented Aug 28, 2016

veewee commented Oct 26, 2016

veewee commented Oct 27, 2016

Feature/php parser task #138

Feature/php parser task #138

Conversation

llaville commented Apr 5, 2016

veewee Apr 6, 2016

Choose a reason for hiding this comment

llaville Apr 6, 2016

Choose a reason for hiding this comment

veewee Apr 6, 2016

Choose a reason for hiding this comment

llaville Apr 6, 2016

Choose a reason for hiding this comment

veewee commented Apr 6, 2016

llaville commented Apr 6, 2016

llaville commented Apr 8, 2016

llaville commented Apr 8, 2016

veewee commented Apr 9, 2016

llaville commented Apr 11, 2016

veewee commented Aug 28, 2016

veewee commented Oct 26, 2016

veewee commented Oct 27, 2016