🇺🇸United States @tim.plunkett

We can rename the file anyway we want, but it has to be separate because of the conditional loading while we are also supporting PHPStan 1 (as these errors are only reported in PHPStan 2)
Agreed we can add some docs if we pursue 794

Two new MRs (!793 and !794) to finish this issue.

Both of them fix a logic "bug" in SortHelper, there's two redundant checks.
Both of them bump our composer.json to the latest version of phpstan-drupal

793 takes the opinion that PHPStan knows best and that we don't need to enforce the types via assert, and removes the extraneous ones.
This has the side benefit of making the calling code actually ensure it is passing what it thinks it's passing, see the change to DrupalCore.php that actually enforces what's in the configuration

794 takes the opinion that while PHPStan might know best, that doubling up with asserts is okay too.
This codifies the doubled asserts in a new baseline file, that is only because PHPStan 1 (or maybe phpstan-drupal 1) aren't flagging this yet.

I slightly prefer 793 because it has less maintenance burden, but I'd be fine with 794.

Merging for now, the 2.0.4 PHPStan-Drupal release solved some but not all of the problems, and I want to unblock other MRs.

Merged !791 🎉, leaving open

tim.plunkett → created an issue.

This should include test coverage for `container`

Discussed this with @leslieg, @jquijano, @mshaik, and @phenaproxima.

Adam and I suggested that we move AU to require from require-dev, so that it is always in the codebase. Then the message can link to the module install page. But it retains the ability for people to NOT use Package Manager.

tim.plunkett → made their first commit to this issue’s fork.

There's a test for pagination across multiple project browser instances that *should* be able to test this too.

Look for \Drupal\Tests\project_browser\FunctionalJavascript\MultipleInstancesTest::testIndependentPagination()

Thanks for addressing the feedback @kunal.sachdev

Fixed

Not to put too fine a point on it, but let's leave this open until the beta is tagged.

Merged !779 🎉

Saving credit

With the move to Attributes, I think this is safe to close

tim.plunkett → created an issue.

tim.plunkett → created an issue.

Also those ignores were done conditionally: 📌 Switch to conditional usage of PHPStan for upstream incpompatibilies Active

What job is this on? hard to tell from the link above.

I am getting PHPstan passes locally with both PHPStan 1 and 2

Sorry @juandhr, it was easier to just fix directly.

@param array<string|int>|null $ids is what the other MR does, and that's the more PHPStan-like syntax, and also is more descriptive than just mixed[]

Confirmed.

This can be closed.

But a bit of history:
That constant was removed from core in March 2014.
That hook never existed as proposed, it was instead committed as hook_menu_link_defaults() and was then promptly removed in July 2014 in favor of the current *.links.menu.yml approach.

Merged !780 🎉

Saving credit

tim.plunkett → made their first commit to this issue’s fork.

Merged !778 🎉

Saving credit

Merged !762 🎉

Saving credit

Merged !766 🎉

Saving credit

Merged !769 🎉

Saving credit

Merged !774 🎉

Saving credit. Excellent work!

Merged !776 🎉

Saving credit

#15: https://git.drupalcode.org/project/drupal_cms/-/blob/1.x/project_templat...

/**
 * Implements hook_form_alter() for install_configure_form.
 */
function drupal_cms_installer_form_install_configure_form_alter(array &$form): void {
  // We always install Automatic Updates, so we don't need to expose the update
  // notification settings.
  $form['update_notifications']['#access'] = FALSE;
}

Merged !770 🎉

Merged !764 🎉

Saving credit

Merged !765 🎉

Saving credit

Saving credit

This is so clean, love to see it.

Merged !773 🎉

tim.plunkett → made their first commit to this issue’s fork.

PHPStan is never wrong™!
Love the refactor since yesterday, as well as the docs on the render element. Great work!

Merged !768 🎉

Saving credit

Merged !767 🎉

Saving credit

Merged !733 🎉

Saving credit

Merged !751 🎉

This looks great. The changes to main.js are so clean!

I don't feel that strongly, will let Chris decide.

Re-reviewed, re-tested, all looks good. Awesome work @utkarsh_33!

tim.plunkett → made their first commit to this issue’s fork.

Merged !754

Saving credit, and rebased

tim.plunkett → made their first commit to this issue’s fork.

#4 makes me think we could add tests for this, right? Some extra fixtures for testRecipesAreDiscovered

I think Chris should review this one as he was able to reproduce the flickering.
But this looks good and completely explains why there was weirdness.

I left the self-review comments from @phenaproxima in-place for Chris, but all my concerns are addressed.

Ugh, we can't fully do this until we drop support for 11.1.x, which is a long ways off (or never?)
That sucks.

Merged !747

Saving credit

tim.plunkett → created an issue.

Reverted my commit, will address in follow-up. This is good to go

tim.plunkett → made their first commit to this issue’s fork.

My main point was going to be about a bit of minor code duplication between the RandomData plugin and the Mock plugin, which led to me asking why we still have both, which is answered by 📌 Remove the RandomData source Active . So this is good for now, and we'll clean up that duplication there!

Reviewed this, and even went down a rabbit hole of the safety of hash_file(), but we're fine because of the file_exists() check (and because the only other error condition is if the file being hashed is over 2GB, and if your composer.lock is over 2GB, you are beyond help)

If this is still desired for PB, it needs to be restarted for the 2.x branch

As discussed, #107 is half pre-existing, half-out-of-scope.
#109 is in response to that, but unchanged here.

I updated the CR per discussion with @lauriii.

I rebased the branch after fully re-reviewing it again.

Thanks @kunal.sachdev!

tim.plunkett → made their first commit to this issue’s fork.

Awesome!

Reviewed!

Awesome, thanks @fjgarlin for debugging!

lol

Split off my work to 📌 Switch to conditional usage of PHPStan for upstream incpompatibilies Active and restored the branch

tim.plunkett → created an issue.

tim.plunkett → made their first commit to this issue’s fork.

Rebuilt this based on how XB is handling it, with @todos pointing to 📌 Only run linting jobs if the files changed make sense for the job Active

tim.plunkett → changed the visibility of the branch 2.0.x to hidden.

#21 all three of those methods are being removed from DrupalDotOrgJsonApi and as of this MR now only exist on DrupalDotOrgSourceBase.

Great! This is a nice modernization.

tim.plunkett → changed the visibility of the branch 3322601-pp-1-add-instructions to active.

tim.plunkett → changed the visibility of the branch 3322601-pp-1-add-instructions to hidden.

Merged !726 🎉

tim.plunkett → made their first commit to this issue’s fork.

All feedback addressed.

Even if the other issue about stripping out inheritdoc landed, I think this issue still has merit. There is value in the knowing the intention of an overridden method, and this isn't a Drupal-ism or from Symfony or Laravel, it's a language-level "feature".

This is a great clean-up. I'd like to see a test for #17, I'm actually surprised PHPStan didn't catch that. But that can happen in a follow-up.

To be clear, this is an MR I made of the branch @joachim of the patch @claudiu.cristea made in 2018. There's definitely a lot of cruft in it.

@nicxvan feel free to push to the MR fork!

Thanks @nod_! Rebased