Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update rector config to resolve bootstrap issues with recent rector releases #219

Merged
merged 11 commits into from
Mar 23, 2023

Conversation

goba
Copy link
Contributor

@goba goba commented Mar 13, 2023

Description

See https://www.drupal.org/project/rector/issues/3346620, I reproduced the same problem locally and adding this conflict to my global composer file helped. Matt Glaman suggested to add it to drupal-rector which makes a ton of sense.

To Test

Install drupal-rector as usual. It will produce the error from https://www.drupal.org/project/rector/issues/3346620 instead of doing anything useful. Apply this PR and it works (for me at least).

Drupal.org issue

https://www.drupal.org/project/rector/issues/3346620

@tobiasbaehr
Copy link
Contributor

Lets try the comment of rectorphp/rector#7795 (comment)

@tobiasbaehr
Copy link
Contributor

I updated rector to the latest version and the changes from the comment works.

@goba
Copy link
Contributor Author

goba commented Mar 15, 2023

Did those updates in this PR, let's see @tobiasbaehr

@goba
Copy link
Contributor Author

goba commented Mar 15, 2023

Looking at the fails, it seems like the first one is phpunit 9.5 requires sebastian/diff ^4.0.3, while rector/rector-src dev-main requires sebastian/diff ^5.0, so they cannot be installed at once. So appears like we need phpunit 10 compatibility first.

@goba goba changed the title Temporary rector release conflict to resolve bootstrap issue Update rector config to resolve bootstrap issues with recent rector releases Mar 15, 2023
@goba
Copy link
Contributor Author

goba commented Mar 15, 2023

This one now passes! "drupal_rector_sandbox / Rector Sandbox | PHP 7.4 (pull_request)"

Next one is "phpunit / PHPUnit | PHP 8.1 (pull_request)" which fails due to lack of static data provider methods. PHPUnit 9.5 deprecated those and PHPUnit 10 wants to look for the static methods but they are not there. So many tests fails due to lack of test methods then.

@goba
Copy link
Contributor Author

goba commented Mar 16, 2023

The failing functional test is producing a bunch of these types of issues:

Could not process
"/home/runner/drupal/web/modules/custom/rector_examples/tests/src/Funct
ional/AssertFieldCheckedTest.php" file, due to:
"System error: "Method assertFieldChecked() was not found in reflection
of class
Drupal\Tests\rector_examples\Functional\AssertFieldCheckedTest."

@goba
Copy link
Contributor Author

goba commented Mar 16, 2023

So now I have 3 of hte 6 checks green. However I think there is a block now with Drupal 10.1 not even allowing PHPUnit 10, so not sure how we could test drupal-rector on PHPUnit 10 with Drupal 8.9 and Drupal 9 :/ The error found above are I think due to the PHPUnit upgrader thingy not being able to deal with PHPUnit 10, so compatible base classes are not present.

So I am not sure we can update to PHPUnit 10 quite yet? https://www.drupal.org/project/drupal/issues/3217904 is the core issue to update core to support it and its not planned to be backported to Drupal 9.

Should we go on and add the conflict instead? We would need to go all the way back to https://packagist.org/packages/rector/rector-src#0.15.11 to have PHPUnit 9.5 support still on rector. Sounds like we may be stuck there to be Drupal 8/9 compatible?

@goba
Copy link
Contributor Author

goba commented Mar 16, 2023

Per @mglaman, this is not an incompatibility. Drupal doesn’t dump Drupal\Tests\ into Composer. So it needs the "phpunit-bootstrap nonsense". I don't really know how to proceed on that though.

@agentrickard
Copy link
Contributor

The functional test runner is still using this matrix -- should we update our versions a bit?

        strategy:
            matrix:
                include:
                    - php-version: "7.4"
                      drupal: "^8.9"
                      fixture: "d8"
                    - php-version: "7.4"
                      drupal: "^9.2"
                      fixture: "d9"
                    - php-version: "8.0"
                      drupal: "^9.2"
                      fixture: "d9"

Copy link
Collaborator

@mglaman mglaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #220 for fixes. Required keeping the bootstrapFiles but adjusting for Rector's changes when including those bootstrap files. Along with a change in exit code reporting for dry runs.

Comment on lines -30 to 31
$rectorConfig->bootstrapFiles([
$rectorConfig->autoloadPaths([
__DIR__ . '/../drupal-phpunit-bootstrap-file.php'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not passing in paths here; we need a bootstrap file that executes PHP code. The autoload paths scan directories instead. Was there a reason?

@agentrickard agentrickard merged commit 9fdfbba into palantirnet:main Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants