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

Upgrade to Rector 0.8 #122

Closed
wants to merge 3 commits into from
Closed

Upgrade to Rector 0.8 #122

wants to merge 3 commits into from

Conversation

TomasVotruba
Copy link
Contributor

Hi folks,

in recent 2 months there've been many changes in Rector, like moving from YAML to PHP.

I was thinking it might you save you some troubles, if I'd handle the upgrade myself, as I made those BC breaks you'd hate to fix 😄

So here it is

Copy link
Contributor

@damontgomery damontgomery left a comment

Choose a reason for hiding this comment

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

Thanks so much, Tomas!

We were looking into these updates a bit ago and this is going to help us a lot in upgrading to the new system.

I left some comments on some things that we'll want to add in before merging.

I think with the number of changes, it's probably easier for me to make a fork of your branch and work on those and then merge it in. Does that sounds like a good approach to you?

We may get a chance to work on this later in the week.

Thanks again.

composer.json Show resolved Hide resolved
@@ -10,7 +10,9 @@
"ast"
],
"require": {
"rector/rector-prefixed": "0.7.29"
"php": "^7.2",
"jawira/case-converter": "^1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

This was removed since we weren't using it any more. We'll have to remove it here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, go for it.

I think I messed rebase, that's why it's here

@@ -1,5 +0,0 @@
# Contains automatic fixes for some deprecations introduced in Drupal 8.4.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this one have a .php version?

Copy link
Contributor Author

@TomasVotruba TomasVotruba Sep 14, 2020

Choose a reason for hiding this comment

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

I might have missed that, during rebase or my for to your master.

Could you add it?

use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

return static function (ContainerConfigurator $containerConfigurator): void {
$containerConfigurator->import(__DIR__ . '/vendor/palantirnet/drupal-rector/config/drupal-8/drupal-8-all-deprecations.yml');
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be updated from .yml to .php?

I think we'll want to also add commented out lines for the individual files in case people want to pick and choose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if it's a Rector config.

$parameters = $containerConfigurator->parameters();

$parameters->set(Option::AUTOLOAD_PATHS, ['docroot/core', 'docroot/core/modules', 'docroot/modules', 'docroot/profiles']);
$parameters->set(Option::EXCLUDE_PATHS, ['*/upgrade_status/tests/modules/*']);
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to add another commented out line here for people to exclude the test folders.

use Rector\Core\Configuration\Option;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

return static function (ContainerConfigurator $containerConfigurator): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall, we'll want to add back the comments to help people customize this file.

@TomasVotruba
Copy link
Contributor Author

I think with the number of changes, it's probably easier for me to make a fork of your branch and work on those and then merge it in. Does that sounds like a good approach to you?

Looking at the comments, I agree with you. Go for it!

I'll save my energy for Rector BC breaks ;)

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Sep 14, 2020

@damontgomery Btw, this might be interesting to you to prevent similar "upgrade" PRs in the future: rectorphp/rector#4222

@damontgomery
Copy link
Contributor

Sorry for the late reply. Other things came up and I wanted to apologize and provide an update. Not sure when we / I will be able to have a look at this.

It's at the top of our list of TODOs.

@mglaman mglaman mentioned this pull request Mar 24, 2021
2 tasks
@TomasVotruba
Copy link
Contributor Author

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.

2 participants