undefined - Contrib.social

🇳🇱Netherlands Spokje

📌 | Drupal core | [PP-1] Bump PHPStan rule level to 2

🇳🇱Netherlands Spokje

a handful of quick fixes in MR!7547 cut it down by about 25%.

Great, but not sure this is part of this issue? Or even what an acceptable amount of added errors to a baseline would be?

Personally, (n=1, yadayada) this seems like follow-up issue material to me and might be derailing the original intent?
But what do I know? _shrug_

📌 | Drupal core | Update to Twig 3.9

🇳🇱Netherlands Spokje

3.9.2 landed and made it all better.

📌 | Drupal core | Update to Twig 3.9

🇳🇱Netherlands Spokje

To be fair, we can still:

- Bump twig/twig to 3.9.1 which fixes Exception: Warning: Undefined variable $blocks.
- Bump to that version in composer.json, since lower version won't pass with the yield changes.

Did that just now.

📌 | Drupal core | Update to Twig 3.9

🇳🇱Netherlands Spokje

longwave → credited Spokje → .

🇳🇱Netherlands Spokje

Ah, thanks

🇳🇱Netherlands Spokje

With 3.9.1 less errors, more Since twig/twig 3.9: Twig node "Foo" is not marked as ready for using "yield" instead of "echo"; please make it ready and then flag it with the #[YieldReady] attribute..

https://github.com/twigphp/Twig/issues/4008 Still messes with (at least) our Twig deprecation testing.

📌 | Drupal core | Downgrade (temporarily) nikic/php-parser to ^4

🇳🇱Netherlands Spokje

📌 | Drupal core | Downgrade (temporarily) nikic/php-parser to ^4

🇳🇱Netherlands Spokje

Not a big fan of lowering versions of dependencies, but seeing that:

- None of our other dependencies need it
- Tests are green
- PHPUnit 10 waits for no man...

I'm going to RTBC

🇳🇱Netherlands Spokje

Looks like we've bumped into https://github.com/twigphp/Twig/issues/4008

🇳🇱Netherlands Spokje

Spokje → created an issue.

📌 | Drupal core | [PP-1] Bump PHPStan rule level to 2

🇳🇱Netherlands Spokje

Added two CRs.

📌 | Drupal core | [PP-1] Bump PHPStan rule level to 2

📌 | Drupal core | Replace calls to ::expectWarning*() in Drupal\Tests\user\Kernel\Views\HandlerFilterRolesTest

🇳🇱Netherlands Spokje

Let's bump it to RTBC and see if the core committers think we still need FM approval and if so, if they can ping them :)

📌 | Drupal core | Replace calls to ::expectWarning*() in Drupal\Tests\user\Kernel\Views\HandlerFilterRolesTest

🇳🇱Netherlands Spokje

Thanks @quietone, good enough for me :)

🇳🇱Netherlands Spokje

- Approach makes sense, seeing there is precedence.
- Code changes make sense (Personally I find the new logged warning a lot more clear than before).
- Tests are green.

The only thing from RTBC-ing this is the CR link, which needs, at least for me, mentioning the new required LoggerInterface $logger, in Roles::__contruct().

📌 | Drupal core | [PP-1] Bump PHPStan rule level to 2

🇳🇱Netherlands Spokje

One argument against staggering would be that raising the PHPStan level would be disruptive to (almost) all open MRs for each bump, so bumping more then one level would make it less disruptive.

That might be the case when the baseline was in the NEON format, and IIRC was one (of many) reasons the Bump-to-L9 issue is stalling.
But as part of that issue, we switched the baseline over to PHP with the reasoning that Git would be much smarter about merging it.

Is bumping a PHPStan level (with each one bringing in loads of changes on the baseline file) still as disruptive to other MRs as we (or at least I) think, or is this a non-issue by now?

If we're in non-issue land on that, I'm all about staggering, if not, we might want to prevent all/a lot of open MRs grinding to a rebase-halt each time we stagger-bump and do it in one go?

📌 | Drupal core | [PP-1] Bump PHPStan rule level to 2

🇳🇱Netherlands Spokje

Thanks @mstrelan, should have found that myself, but somehow didn't. (Makes mental note to check caffeine-percentage of new bought coffee-brand).

Seeing those checks and also the fact that if we want to do it with the least amount of disruptions to other MRs, it's probably now or when D12 arrives, I'm pulling the trigger and put the ball in core commiters court: RTBC for me.

📌 | Drupal core | [PP-1] Bump PHPStan rule level to 2

🇳🇱Netherlands Spokje

Looks like when we bump to PHPStan L2 we get an initial addition of ~7800 suppressions.
Seeing the amount of active issues to tackle the L1 suppressions, I think it's safe to assume these won't go away quickly.

So the benefit would be preventing new L2 issues creeping in new/refactored code.

It's probably my lack of Google skills, but what are the extra checks in L2 compared to L1, what will be prevented from creeping in by us doing PHPStan L++?

📌 | Drupal core | Bump phpstan/phpstan and mglaman/phpstan-drupal to latest

📌 | Drupal core | Use a tagged service iterator for uninstall validators instead of individual lazy proxies

🇳🇱Netherlands Spokje

Ah crap, of course, forgot that in my "rage" about the baseline formatting.

🇳🇱Netherlands Spokje

📌 Bump phpstan/phpstan and mglaman/phpstan-drupal to latest Needs review probably needs to land to make this MR work.

📌 | Drupal core | Bump phpstan/phpstan and mglaman/phpstan-drupal to latest

🇳🇱Netherlands Spokje

Note: Creating a new PHPStan-baseline exposed some formatting differences between the generated and committed-in-HEAD one.
Let's use this issue to reconcile these.

As a side note: If people are manually editing the baseline, let's make the GitLab job fail on these differences, so we don't need this kind of reconciliation.

📌 | Drupal core | Bump phpstan/phpstan and mglaman/phpstan-drupal to latest

🇳🇱Netherlands Spokje

📌 | Drupal core | Bump phpstan/phpstan and mglaman/phpstan-drupal to latest

🇳🇱Netherlands Spokje

Spokje → created an issue.

📌 | Drupal core | Replace ContainerAwareEventDispatcher with Symfony EventDispatcher

🇳🇱Netherlands Spokje

Spokje → changed the visibility of the branch 10.3.x to active.

📌 | Drupal core | Replace ContainerAwareEventDispatcher with Symfony EventDispatcher

🇳🇱Netherlands Spokje

📌 | Drupal core | Replace ContainerAwareEventDispatcher with Symfony EventDispatcher

🇳🇱Netherlands Spokje

Spokje → changed the visibility of the branch 10.3.x to hidden.

📌 | Drupal core | Replace ContainerAwareEventDispatcher with Symfony EventDispatcher

🇳🇱Netherlands Spokje

Spokje → changed the visibility of the branch 11.x to hidden.

📌 | Drupal core | CacheCollector::normalizeLockName() is pointless

🇳🇱Netherlands Spokje

LGTM

📌 | Drupal core | Use DependencySerializationTrait in ViewsExecutable

🇳🇱Netherlands Spokje

"Interestingly" \Drupal\Tests\views\Functional\UserBatchActionTest passes locally and on Gitlab on PHP 8.1 (https://git.drupalcode.org/issue/drupal-3390360/-/jobs/1183273#L65) but fails on PHP 8.3.

Also I would have expected either no or a bunch of failures, not just one test that (on first glance) doesn't seem to do anything to wild.

📌 | Drupal core | Use DependencySerializationTrait in ViewsExecutable

🇳🇱Netherlands Spokje

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

📌 | Drupal core | CacheCollector::normalizeLockName() is pointless

🇳🇱Netherlands Spokje

Explanation makes sense
Code changes make sense
Tests are green
-------------------------+
RTBC

📌 | Drupal core | Remove unused DependencySerializationTrait alias from ContextDefinition

🇳🇱Netherlands Spokje

📌 | Drupal core | Remove unused DependencySerializationTrait alias from ContextDefinition

🇳🇱Netherlands Spokje

Looks like the __sleep alias can be dropped, which would prevent future-us from trying to drop the whole Trait.

📌 | Drupal core | Remove unused DependencySerializationTrait alias from ContextDefinition

🇳🇱Netherlands Spokje

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

📌 | Drupal core | Replace RequestCloseSubscriber with needs_destruction tag on ModuleHandler

📌 | Drupal core | Method getMockForAbstractClass() of class PHPUnit\Framework\TestCase is deprecated in PHPUnit 10 - Step 1 interfaces

🇳🇱Netherlands Spokje

Makes sense to me, and tests are green => RTBC

📌 | Drupal core | Method getMockForAbstractClass() of class PHPUnit\Framework\TestCase is deprecated in PHPUnit 10 - Step 1 interfaces

🇳🇱Netherlands Spokje

Thanks @mondrake, I'll take a 50% success rate any day...

RTBC for me now.

📌 | Drupal core | Method addMethods() of class PHPUnit\Framework\MockObject\MockBuilder is deprecated in PHPUnit 10

🇳🇱Netherlands Spokje

Like the approach, like the MR, but we've might have a few stragglers (at least, if I understand the scope correctly):

- \Drupal\Tests\Component\Plugin\Discovery\StaticDiscoveryDecoratorTest::testGetDefinition: https://git.drupalcode.org/project/drupal/blob/b73c68de45491739c0752d859... and https://git.drupalcode.org/project/drupal/blob/b73c68de45491739c0752d859...
- \Drupal\Tests\Core\Entity\Sql\SqlContentEntityStorageTest::testLoadMultiplePersistentCached: https://git.drupalcode.org/project/drupal/blob/654b1f2074ea90b75a8750f2b...
- \Drupal\Tests\Core\Entity\Sql\SqlContentEntityStorageTest::testLoadMultipleNoPersistentCache: https://git.drupalcode.org/project/drupal/blob/654b1f2074ea90b75a8750f2b...
- \Drupal\Tests\Core\Entity\Sql\SqlContentEntityStorageTest::testLoadMultiplePersistentCacheMiss: https://git.drupalcode.org/project/drupal/blob/654b1f2074ea90b75a8750f2b...
- \Drupal\Tests\Core\Entity\TypedData\EntityAdapterUnitTest::setUp: https://git.drupalcode.org/project/drupal/blob/9add7f71ff26864477f80e349...

🇳🇱Netherlands Spokje

Explanation and code changes make sense. Overall a nice cleanup!

✨ | Drupal core | Bump PHPStan to level 9 and accept a large baseline

📌 | Drupal core | Method getObjectForTrait() of class PHPUnit\Framework\TestCase is deprecated in PHPUnit 10

🇳🇱Netherlands Spokje

Let's clear out everything in the current baseline, then bump it to level 2 or 3

This is the current plan anyway,

To be fair, that plan is not going fast, or to be more precise, seems to have been grinded to a halt.

There's so much BC in play with fixing even the low hanging issues, that even determining what arethe low hanging fruits is difficult and leads to much discussion.

I myself have (almost) given up on working on PHPStan base-level issues, since they really suck the life-force out of my contributing spirit.

Having said that, although I can see some advantages in upping to level 9 and at the very least be warned when new PHPStan issues creep in, the sheer amount that would have to be committed to the baseline because of the BC-promises made, would be nullifying those advantages IMHO.

So I personally don't see advantages upping to any level above the current level 1, until we have either more persons willing to contribute on fixing the current issues with the current PHPStan level and/or have a long hard look at BC policies we currently have and if they're perhaps holding us back to much.

But of course, n=1, personal opinion and all that.

📌 | Drupal core | Method getMockForTrait() of class PHPUnit\Framework\MockObject\MockBuilder is deprecated in PHPUnit 10

🇳🇱Netherlands Spokje

MR actually extends the existing test-coverage and replaces the (as said) only occurrence of getObjectForTrait().