Rector 0.10 #134

mglaman · 2021-03-24T20:19:23Z

Fixes #133
Fixes #135

Remaining items

Find out why parsing src/Plugin/WebformElement/WebformLikert crashed PHPStan's doc parser (This should be a follow up, it's not a Rector or this package bug)
Do not assume PHPUnit 7 in trait compatibility

Required follow ups

These are issues that need a fast follows once we merge this PR, otherwise the PR will just grow and grow and grow.

#138 Constants defined in .module files are not discovered
Drupal 9 does not have PhpUnitCompatibility test traits (see Rector 0.10 #134 (comment))
Messaging that users should upgrade to 8.9 if on 8.7, 8.8, etc (see Rector 0.10 #134 (comment))

How to test

Add fork to your Drupal 8 site (assuming Composer build) composer.json

cd /path/to/drupal/recommended-project
composer config repositories.1 vcs https://github.com/bluehorndigital/drupal-rector.git

Require it

composer require palantirnet/drupal-rector:dev-rector-0.10

Configure it

cp vendor/palantirnet/drupal-rector/rector.php .

Run it!

Test it on your own code

php vendor/bin/rector process web/modules/custom

OR Grab the examples

cp -r vendor/palantirnet/drupal-rector/rector_examples web/modules/
php vendor/bin/rector process web/modules/rector_examples --dry-run

Verify it!

Notes

Takes #122 commits and updates to 0.10

composer: update rector to 0.8
switch configs from YML to PHP
drop duplicated info in editorconfig
Add phpstan and rector dev trunk
Add back EntityDeleteMultipleRector
Update rule definitions for Rector 0.10

# Conflicts: # .editorconfig # composer.json # config/drupal-8/drupal-8.0-deprecations.yml

Also adds PHPStan workflow to catch unused calls.

mglaman · 2021-03-29T15:34:52Z

Found out why this is not working: we weren't analyzing any PHP files, setting the extensions is a full parameter overwrite

Now we have a new failure that needs to be debugged BUT is consistent

PHP Fatal error:  Trait 'Drupal\Tests\SessionTestTrait' not found in /home/runner/drupal/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php on line 32
Fatal error: Trait 'Drupal\Tests\SessionTestTrait' not found in /home/runner/drupal/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php on line 32
Error: Process completed with exit code 255.

I opened an issue for Rector that .php files should always be analyzed rectorphp/rector#6008

damontgomery

Thanks again, @mglaman.

Testing steps I used:

Use the Drupal Rector Sandbox (which uses DDEV)
Add your branch within local /drupal-rector
Update the composer dependencies, composer update palantirnet/drupal-rector -W
Copy rector.php into the project root
Run Drupal Rector, vendor/bin/rector process web/modules/custom/rector_examples --dry-run

(HAS SINCE BEEN RESOLVED) Got the error:

PHP Fatal error:  Declaration of RectorPrefix20210420\Symfony\Component\DependencyInjection\ServiceLocator::has(string $id) must be compatible with RectorPrefix20210420\Psr\Container\ContainerInterface::has($id) in /Users/dan/Sites/drupal-rector-sandbox/vendor/rector/rector-prefixed/vendor/symfony/dependency-injection/ServiceLocator.php on line 25
Fatal error: Declaration of RectorPrefix20210420\Symfony\Component\DependencyInjection\ServiceLocator::has(string $id) must be compatible with RectorPrefix20210420\Psr\Container\ContainerInterface::has($id) in /Users/dan/Sites/drupal-rector-sandbox/vendor/rector/rector-prefixed/vendor/symfony/dependency-injection/ServiceLocator.php on line 25

Going to keep looking over the code for readability, etc.

damontgomery

WIP review. Have to jump on a call.

Overall this makes a lot of sense to me.

I left a few comments about developer experience.

I think the automated tests, Github Actions / Behat need to be reviewed. The Behat tests can likely be completely removed since they were replaced and the Github Actions jobs need to be consistent and we may be able to remove some of them if they are no longer needed.

I may be able to take a closer look at this later, but I wanted to post in-progress comments in case it's helpful.

Thanks again.

.github/workflows/local_package_functional_tests.yml

config/drupal-8/drupal-8-all-deprecations.php

damontgomery · 2021-04-21T19:48:07Z

config/drupal-phpunit-bootstrap-file.php

+/**
+ * @file
+ *
+ * This fixes Drupal testing namespace autoloading and PHPUnit compatibility.
+ */


++ It's been a while and I'm not too familiar with all the specifics if we need updates down the road.

Can you comment on the places that were modified?

damontgomery · 2021-04-21T19:52:49Z

rector.php

+    $parameters->set(Option::IMPORT_SHORT_CLASSES, false);
+    $parameters->set(Option::IMPORT_DOC_BLOCKS, false);
+
+    $parameters->set('drupal_rector_notices_as_comments', true);


Developer experience: Can we restore / replace comments?

People are expected to make small changes to this file and the comments may be helpful for common use cases.

@damontgomery actually this keeps comments. Commits like this fixed commenting, it was one of the first regressions found: 02c49a5

@mglaman, sorry, this was a bad place to add my question / suggestion. :(.

I meant the comments like # This will add comments to call out edge cases in replacements. in https://github.com/palantirnet/drupal-rector/blob/master/rector.yml.

damontgomery · 2021-04-21T21:07:35Z

Thanks for testing @shaal.

I'm ok with pinning the release if you want for now. We've had to do that before.

Downgraded with "rector/rector-prefixed": "0.10.6", and composer update palantirnet/drupal-rector -W.

That works well!

Can we update the README with directions on running PhpUnit locally now that that's a requirement for maintainers?

This process from the Github Actions job seems to work, but I get errors.

composer install
composer create-project drupal/recommended-project:^8.9 tests/fixtures/drupal
vendor/bin/phpunit

0.10.9 version error:

1) DrupalRector\Tests\DrupalIntegrationTest::testIntegration with data set #0 ('/Users/dan/Sites/drupal-recto...at.php', array('DrupalRector\Rector\Deprecati...Rector'))
OutOfBoundsException: Package "rector/rector" is not installed

0.10.6 error:

1) DrupalRector\Tests\DrupalIntegrationTest::testIntegration with data set #0 ('/Users/dan/Sites/drupal-recto...at.php', array('DrupalRector\Rector\Deprecati...Rector'))
Error: Class 'Rector\ChangesReporting\ValueObjectFactory\ErrorFactory' not found

I've tried this within a Drupal Rector Sandbox install and with the repo by itself from my host machine.

: vendor/bin/phpunit
PHPUnit 9.5.4 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.3.18
Configuration: /Users/dan/Sites/drupal-rector-test/phpunit.xml

Any thoughts on what I should try instead? Seen any of these errors?

They seem weird since composer show rector/rector shows the package installed for the first and I can't find any mention of ErrorFactory in the code for the second.

Try to be clear about copy & paste steps. We could share jobs, but these run faster. Removed Behat mentions since we aren't using it. Updated the local composer.json repository config to add names and split it up to be easier to read.

These are included directly in Github Actions.

mglaman · 2021-04-28T19:24:27Z

@damontgomery Here's my go and providing feedback

++ It's been a while and I'm not too familiar with all the specifics if we need updates down the road.

Previously, with older versions of PHPStan, the original "point at this folder and autoload" worked OK. That broke. I copied over methods from phpstan-drupal. Once this merges, I'm actually making a new package called drupal-static-autoloader which drupal-rector, phpstan-drupal, and even the Psalm plugin can use.

drupal-phpunit-bootstrap-file.php is used to register test namespaces and work around bug sin traits. It's a big topic to explain, it's covered in previous streams. But this blog helps explain:

I'm ok with pinning the release if you want for now. We've had to do that before.

I'd like to keep it open, but at a floating patch release. They fixed the regression fairly fast.

damontgomery · 2021-05-03T19:40:30Z

Thanks for the comments @mglaman . I haven't dug into the details of the PhpUnit configuration, etc, but I think if it becomes an issues we can reference your comment in this PR.

damontgomery · 2021-05-03T20:59:24Z

@mglaman,

I added some commits to https://github.com/palantirnet/drupal-rector/tree/rector-0.10. I have tried adding your fork as a remote a few times and it doesn't let me push. 🤷.

2 remaining issues for me.

I can't get PHPUnit to run. With the fixture set up, I get this error. It is from the PHPUnit in the fixtures vendor folder... which seems weird. Not sure what to do there.

PHP Fatal error:  Declaration of PHPUnit\Framework\Constraint\IsEqual::evaluate($other, $description = '', $returnResult = false) must be compatible with PHPUnit\Framework\Constraint\Constraint::evaluate($other, string $description = '', bool $returnResult = false): ?bool in /Users/dan/Sites/drupal-rector-test/tests/fixtures/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/IsEqual.php on line 150

Fatal error: Declaration of PHPUnit\Framework\Constraint\IsEqual::evaluate($other, $description = '', $returnResult = false) must be compatible with PHPUnit\Framework\Constraint\Constraint::evaluate($other, string $description = '', bool $returnResult = false): ?bool in /Users/dan/Sites/drupal-rector-test/tests/fixtures/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/IsEqual.php on line 150

Conceptually, I'm not sure what to do about the functional tests. These, in my opinion, are one-of-if-not-the-most valuable tests. They help us make sure the changes we expect area actually what happens. The other tests are nice, but reports that Rector tells us a rule has run are not as convincing to me as the actual changed files.

The codebase needs a copy of Drupal to run. PhpUnit has added that as a fixture. I'm fine using that copy to run functional tests, but I can't get them to work. We were previously running the Behat tests from within our Drupal Rector Sandbox... which now will have two copies of Drupal in it. :(. In addition to having Drupal / Rector available to use, the sandbox also makes it easy to develop with Rector using XDebug and I don't want to lose that feature.

vendor/bin/rector process rector_examples/entity_load.php --dry-run --config=tests/config/rector-phpunit.php

Produces this error:

PHP Fatal error:  Uncaught Error: Undefined class constant 'T_ENUM' in /Users/dan/Sites/drupal-rector-test/vendor/nikic/php-parser/lib/PhpParser/Lexer.php:517
Stack trace:
#0 /Users/dan/Sites/drupal-rector-test/vendor/nikic/php-parser/lib/PhpParser/Lexer.php(40): PhpParser\Lexer->createTokenMap()
#1 phar:///Users/dan/Sites/drupal-rector-test/vendor/phpstan/phpstan/phpstan.phar/src/Parser/LexerFactory.php(20): PhpParser\Lexer->__construct(Array)
#2 /private/var/folders/pn/0q1hkstd1wj9jnldkkf_y5t00000gn/T/cache/nette.configurator/Container_b1d570eb8b.php(4329): PHPStan\Parser\LexerFactory->create()
#3 phar:///Users/dan/Sites/drupal-rector-test/vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php(157): Container_b1d570eb8b->createServiceCurrentPhpVersionLexer()
#4 phar:///Users/dan/Sites/drupal-rector-test/vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php(90): _HumbugBoxe5640220fe34\Nette\DI\Container->createService('currentPhpVersi...')
#5 /private/var/folders/pn/0q1hkstd1wj9jnldkkf_y5t in /Users/dan/Sites/drupal-rector-test/vendor/nikic/php-parser/lib/PhpParser/Lexer.php on line 517
Fatal error: Uncaught Error: Undefined class constant 'T_ENUM' in /Users/dan/Sites/drupal-rector-test/vendor/nikic/php-parser/lib/PhpParser/Lexer.php:517
Stack trace:
#0 /Users/dan/Sites/drupal-rector-test/vendor/nikic/php-parser/lib/PhpParser/Lexer.php(40): PhpParser\Lexer->createTokenMap()
#1 phar:///Users/dan/Sites/drupal-rector-test/vendor/phpstan/phpstan/phpstan.phar/src/Parser/LexerFactory.php(20): PhpParser\Lexer->__construct(Array)
#2 /private/var/folders/pn/0q1hkstd1wj9jnldkkf_y5t00000gn/T/cache/nette.configurator/Container_b1d570eb8b.php(4329): PHPStan\Parser\LexerFactory->create()
#3 phar:///Users/dan/Sites/drupal-rector-test/vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php(157): Container_b1d570eb8b->createServiceCurrentPhpVersionLexer()
#4 phar:///Users/dan/Sites/drupal-rector-test/vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php(90): _HumbugBoxe5640220fe34\Nette\DI\Container->createService('currentPhpVersi...')
#5 /private/var/folders/pn/0q1hkstd1wj9jnldkkf_y5t in /Users/dan/Sites/drupal-rector-test/vendor/nikic/php-parser/lib/PhpParser/Lexer.php on line 517

I think we are close and I'm not sure the best path forward. Maybe a call / discussion.

I understand the feeling of urgency that this is a big change and blocks future work.

However, I really don't want to lose features and I don't want to institute a brittle system that requires a lot of knowledge of other systems to maintain. This is currently in a state where I personally cannot use the tool as well without the things I mentioned. As we delve deeper into the PhpUnit space, it seems like we have made a lot of hacks to get PhpUnit tests to work with Rector and this seems like potentially a brittle system to me. I worry that we moved away from a fairly simple system with the sandbox / Behat functional tests towards one that only you can currently debug and work on. I feel like its just a matter of time that future versions of Rector or changes to PhpUnit or Drupal core will break this setup and we'll need to reconcile all those pieces to fix it.

mglaman · 2021-05-03T21:13:10Z

I've got a handful of errands to take care of and I'll read this tomorrow. All of the work was necessary to get it running. PHPUnit? Maybe not. But it made my velocity possible. If this PR is unsatisfactory or a different approach is desired (stay on Rector 0.7) I'm fine closing the PR and moving on

TomasVotruba · 2021-05-03T21:23:12Z

PHP Fatal error: Uncaught Error: Undefined class constant 'T_ENUM'

It's probably bug in php-parser. We have the same in Rector now. Locking to php-parser 4.10.4 should help

TomasVotruba · 2021-05-03T21:25:48Z

I can't get PHPUnit to run. With the fixture set up,

I would try to bump Rector to rector/rector-prefixed:^0.10.17. Prefixed version helps with some dependency conflicts, as Rector code dependencies is isolated.

damontgomery · 2021-05-03T21:33:37Z

Thanks, @TomasVotruba.

This has a dev dependency on rector/rector I think because it uses dependencies in there like Symfony\Component\Console\Tester\CommandTester.

If I remove rector/rector, I can run rector with the PhpUnit fixture with this command:

vendor/bin/rector process rector_examples/entity_load.php --dry-run --config=tests/config/rector-phpunit.php, but I cannot run the phpunit tests with vendor/bin/phpunit. :(

1) DrupalRector\Tests\DrupalIntegrationTest::testIntegration with data set #1 ('/Users/dan/Sites/drupal-recto...nt.php', array('DrupalRector\Rector\Deprecati...Rector'))
Error: Class 'Symfony\Component\Console\Tester\CommandTester' not found

/Users/dan/Sites/drupal-rector-test/vendor/phpunit/phpunit/src/Framework/TestResult.php:722
/Users/dan/Sites/drupal-rector-test/vendor/phpunit/phpunit/src/Framework/TestSuite.php:677
/Users/dan/Sites/drupal-rector-test/vendor/phpunit/phpunit/src/Framework/TestSuite.php:677
/Users/dan/Sites/drupal-rector-test/vendor/phpunit/phpunit/src/Framework/TestSuite.php:677
/Users/dan/Sites/drupal-rector-test/vendor/phpunit/phpunit/src/Framework/TestSuite.php:677
/Users/dan/Sites/drupal-rector-test/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:667
/Users/dan/Sites/drupal-rector-test/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/Users/dan/Sites/drupal-rector-test/vendor/phpunit/phpunit/src/TextUI/Command.php:96

TomasVotruba · 2021-05-03T22:06:13Z

Symfony\Component\Console\Tester\CommandTester

That's your local dependency, not related to Rector. I would just add the package to have it:

composer require symfony/console

damontgomery · 2021-05-03T23:49:30Z

Thanks for the feedback @TomasVotruba & @mglaman.

Apologies again if I've caused any stress the past few days / weeks.

I've opened #144 which includes all the work I could verify. As commented there, I think PhpUnit support, PhpStan, and the other developer tools Matt has added are a good direction, particularly based on what Matt has said about them being helpful.

I can't verify them right now and I may not have a lot of time to debug my issues with the tools until next week. These could all be issues with my local setup and additional testing from the community is greatly appreciated.

Thanks again.

mglaman · 2021-05-04T03:11:26Z

I'm just closing this.

PHPUnit was added to make testing possible, because Behat just ran Bash commands and I couldn't run it locally.

PHPStan was added to catch some bugs that cropped up with the upgrade.

Rector prefixed is a dependency, but rector/rector is dev so we can use test classes.

I'm fine with the minimal. But I've spent too much time fighting the upgrade that I'll be stepping away

goba · 2021-05-06T08:21:33Z

@KarinG and @joeparsons I believe you tested this PR at DrupalCon, can you help test #144 as well, that is a simplified version of this so should be similar :) Thanks a lot!

TomasVotruba and others added 14 commits September 12, 2020 22:32

composer: update rector to 0.8

17b36a4

switch configs from YML to PHP

9732bf1

drop duplicated info in editorconfig

e1e09b0

Add phpstan and rector dev trunk

b6ad9c8

Merge branch 'rector-08' into rector-0.10

4964e07

# Conflicts: # .editorconfig # composer.json # config/drupal-8/drupal-8.0-deprecations.yml

Add back EntityDeleteMultipleRector

c043b05

Update rule definitions for Rector 0.10

e02f8cc

Fix remaining usages of Option::EXCLUDE_PATHS

541584c

Also adds PHPStan workflow to catch unused calls.

PHPStan workflow to catch unused calls.

f29c567

Add PHPUnit and initial test

669038d

Update local packages flow

7c81ed4

Handle ~/drupal in specific workflow

ece4915

Normalize GitHub flows

d763416

Add PHPUnit flow, move deprecation-index tests to PHPUnit

f72dfaa

mglaman force-pushed the rector-0.10 branch 4 times, most recently from 9cd61c9 to b6298aa Compare March 29, 2021 03:00

Move away from Behat and to specific commands

dcb66ff

mglaman force-pushed the rector-0.10 branch from b6298aa to dcb66ff Compare March 29, 2021 03:04