openeuropa / drupal-site-template

OpenEuropa template for Drupal sites.
European Union Public License 1.2
12 stars 10 forks source link

OPENEUROPA-9 #1

Closed pfrenssen closed 6 years ago

ademarco commented 6 years ago

@pfrenssen don't we need to explicitly require nikic/php-parser as a dev dependency? We do need that library to perform the following grumphp.yml.dist task:

  extra_tasks:
    phpparser:
      ignore_patterns: %tasks.phpcs.ignore_patterns%
      visitors:
        declare_strict_types: ~
      triggered_by: %tasks.phpcs.triggered_by%

And, at the moment, it we get it there since it's required by a Drush dependency, see below:

$ composer depends nikic/php-parser
psy/psysh  v0.9.7  requires  nikic/php-parser (~1.3|~2.0|~3.0|~4.0)  

$ composer depends psy/psysh  
drupal/console  1.8.0  requires  psy/psysh (0.6.* || ~0.8)  
drush/drush     9.3.0  requires  psy/psysh (~0.6)           

While we should explicitly require all our dependencies since we cannot be sure that our dependencies will not stop depending on nikic/php-parser one day.

Also, if we set major version to ~3 we don't need to add the patch.

pfrenssen commented 6 years ago

The patch has been removed again, put this back please. Also we are not limited to PHP Parser 3.x, we support ~2.0|~3.0|~4.0. It's beneficial for our users if we support as broad of a range as possible, they might have reasons to require and older or newer version.

See my earlier comment for my reasoning to keep the patch:

It's better to have the patch I think. Having a patch clearly communicates to the end user "We are affected by this particular bug in this package, here is the link to the problem".

While having this version declared here communicates "We are depending on PHP Parser version 3 for development purposes". There is no way for an end user to know that we depend on this particular version of PHP Parser to work around a bug in GrumPHP.

There is another advantage to having the patch: once a new version of GrumPHP is released that contains the patch we will be alerted immediately since the patch will fail to apply. That will be the right moment to roll a new release for the code review tool and removing the patch here.