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 phpstan/phpstan related dependencies to ^2.0" #805

Merged
merged 22 commits into from
Nov 14, 2024
Merged

Conversation

Boegie
Copy link
Contributor

@Boegie Boegie commented Nov 11, 2024

Naive first stab

@mglaman
Copy link
Owner

mglaman commented Nov 11, 2024

I'll probably want to open a 2.x branch as well. Thanks for trying at this.

@Boegie
Copy link
Contributor Author

Boegie commented Nov 11, 2024

No worries, I'll chip away at this for an hour 2 from now.

@Boegie
Copy link
Contributor Author

Boegie commented Nov 12, 2024

Ok, so this was more of a grind than actually Big Brain stuff.

I would recommend:

  • Going over the identifiers that were (forcefully) added. There are a lot of them and they might not be consistent.
  • The changes in .github/workflows/php.yml can be reverted when drupal core includes phpstan/phpstan:^2.0
  • Replacing all @phpstan-ignore next-line nuke-em-all lines with the more targeted @phpstan-ignore [identifier] in a separate MR. I found some "hidden sins" whilst creating this MR.

Also:

A new branch might be a nice clean cut-off point to drop D9 support?

Copy link
Owner

@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.

oh, did rule identifiers become required, essentially fixing #803 😱 I wish I could have landed that sooner to reduce this effort. I appreciate it. I literally fell asleep at my laptop 2 times while working on that.

Comment on lines -81 to 84
->identifier('plugin.manager.alterInfoMissing')
->tip('For example, to invoke hook_mymodule_data_alter() call alterInfo with "mymodule_data".')
->line($node->getStartLine())
->identifier('pluginManagerInspection.alterInfoMissing')
->build();
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't review the commit log, but I'm guessing something about the format of the error identifier made PHPStan mad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, this is a rare occasion where there was actually an identifier and I completely overlooked it.

Not too sure what to do now...

Do we want double-full-stop identifiers?

If we do I'll have to change the other identifiers in this class to the same prefix.

Help! :)

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine taking your changes, I was just curious

src/Rules/Drupal/GlobalDrupalDependencyInjectionRule.php Outdated Show resolved Hide resolved
@mglaman
Copy link
Owner

mglaman commented Nov 12, 2024

A new branch might be a nice clean cut-off point to drop D9 support?

We can say 2.x drops Drupal 9 support, as a follow up to his PR.

@Boegie
Copy link
Contributor Author

Boegie commented Nov 12, 2024

I appreciate it. I literally fell asleep at my laptop 2 times while working on that.

I couldn't handle in batches longer than one hour, extremely grindy stuff

@mglaman
Copy link
Owner

mglaman commented Nov 14, 2024

Thanks for this!!

@mglaman mglaman merged commit ce1e7a1 into mglaman:main Nov 14, 2024
17 of 18 checks passed
This was referenced Nov 14, 2024
@Boegie Boegie deleted the 2.0 branch November 15, 2024 06:40
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