Account created on 13 January 2016, over 8 years ago
#

Merge Requests

More

Recent comments

πŸ‡³πŸ‡±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_

πŸ‡³πŸ‡±Netherlands Spokje

3.9.2 landed and made it all better.

πŸ‡³πŸ‡±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.

πŸ‡³πŸ‡±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.

πŸ‡³πŸ‡±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

Spokje β†’ created an issue.

πŸ‡³πŸ‡±Netherlands Spokje

Added two CRs.

πŸ‡³πŸ‡±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 :)

πŸ‡³πŸ‡±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().

πŸ‡³πŸ‡±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?

πŸ‡³πŸ‡±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.

πŸ‡³πŸ‡±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++?

πŸ‡³πŸ‡±Netherlands Spokje

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

πŸ‡³πŸ‡±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.

πŸ‡³πŸ‡±Netherlands Spokje

Spokje β†’ changed the visibility of the branch 10.3.x to active.

πŸ‡³πŸ‡±Netherlands Spokje

Spokje β†’ changed the visibility of the branch 10.3.x to hidden.

πŸ‡³πŸ‡±Netherlands Spokje

Spokje β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡³πŸ‡±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.

πŸ‡³πŸ‡±Netherlands Spokje

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

πŸ‡³πŸ‡±Netherlands Spokje

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

πŸ‡³πŸ‡±Netherlands Spokje

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

πŸ‡³πŸ‡±Netherlands Spokje

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

πŸ‡³πŸ‡±Netherlands Spokje

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

πŸ‡³πŸ‡±Netherlands Spokje

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

RTBC for me now.

πŸ‡³πŸ‡±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!

πŸ‡³πŸ‡±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.

πŸ‡³πŸ‡±Netherlands Spokje

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

πŸ‡³πŸ‡±Netherlands Spokje

This MR replaces getMockForTrait() with adding a test class which uses the Trait.
AFAICT this is indeed the recommended way to do this.

After applying the MR there are no references left regarding getMockForTrait(), tests are green => RTBC.

πŸ‡³πŸ‡±Netherlands Spokje

I'll start doing lousy work then

Ah, doing lousy work, the foundation of my whole career... ;)

πŸ‡³πŸ‡±Netherlands Spokje

Oh no, I very much like this approach.

My comment was just meant to try to amuse myself whilst c/p-ing my previous RTBC messages.

The problem here is my lousy jokes and your great work, which makes me not able to fault any of it :)

πŸ‡³πŸ‡±Netherlands Spokje

Same ol' same ol':
Replacement makes sense, couldn't find any other occurrences than the ones in the MR, Green tests => RTBC.

πŸ‡³πŸ‡±Netherlands Spokje

Said it before and will say it again:

Replacement makes sense, couldn't find any other occurrences than the ones in the MR, Green tests => RTBC.

πŸ‡³πŸ‡±Netherlands Spokje

Ah, so something like hook_help_topics_info_alter is actually used by the help module?

πŸ‡³πŸ‡±Netherlands Spokje

After applying the MR I still see a few textual references to the help_topics module and at least two test modules with the name help_topics in them.

I guess we want to remove all of those as well?

πŸ‡³πŸ‡±Netherlands Spokje

Spokje β†’ changed the visibility of the branch 3394694-sf7-no-containeraware-less-rebases to hidden.

πŸ‡³πŸ‡±Netherlands Spokje

PHP 8.2 is the minimum for SF 7, so not really a hard blocker.

However the release of SF 7.0.6 is, if we want to prevent using workarounds (as said by @longwave in #52 πŸ“Œ Symfony 7 compatibility Active )

πŸ‡³πŸ‡±Netherlands Spokje

Replacement makes sense, couldn't find any other occurrences than the ones in the MR, Green tests => RTBC

πŸ‡³πŸ‡±Netherlands Spokje

Replacement makes sense, couldn't find any other occurrences than the ones in the MR, Green tests => RTBC

πŸ‡³πŸ‡±Netherlands Spokje

We might want to be compatible with sebastian/diff v5 but we can't for now:

Not saying we shouldn't, but why all the effort when none of our dependencies apparently need v5?

πŸ‡³πŸ‡±Netherlands Spokje

We might want to be compatible with sebastian/diff v5 but we can't for now:

Not saying we shouldn't, but why all the effort when none of our dependencies apparently need v5?

πŸ‡³πŸ‡±Netherlands Spokje

Thanks @longwave, adjusted test.

πŸ‡³πŸ‡±Netherlands Spokje

Five references left for ContainerAwareInterface after this MR.

They're all needed to keep LoggerChannelFactory working for now.

πŸ‡³πŸ‡±Netherlands Spokje

So we know there's pain when changing fixtures, so we combining the changes so there's less pain.

Now what if a core module doesn't have the remove issue committed on time for 10.3.0?

We then have to go through the pain again when changing the fixture painfully made in 10.3.0 in 10.4.0?
Could we move the removal to 12.0.0, so there's less pain?

(Not sure about if there's already specific rules/documentation on this, just curious and pretty much done with the pain after the gazillion changes we did on fixtures during D9 => D10.)

πŸ‡³πŸ‡±Netherlands Spokje

Confirming the updated-deps job now runs correctly in the scheduled daily runs, see https://git.drupalcode.org/project/drupal/-/pipelines/121984 as a result for the 11.x one.

πŸ‡³πŸ‡±Netherlands Spokje

Reasoning makes sense, so do the code changes.

πŸ‡³πŸ‡±Netherlands Spokje

Looks like the original deprecation of the bool $catchThrowable (https://github.com/symfony/symfony/pull/45997/files#diff-8a470a718608567...) went away in https://github.com/symfony/symfony/commit/8604d3ae048b5383f878db93a834c0....
and it became bool $handleAllThrowables = false https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/HttpKe....

So I think we can close this one as outdated, we still want to opt-in to that behaviour as we currently are, so there's nothing to remove AFAICT.

πŸ‡³πŸ‡±Netherlands Spokje

Spokje β†’ changed the visibility of the branch 3394694-sf7 to hidden.

πŸ‡³πŸ‡±Netherlands Spokje

Dropped some lines as a response, let's see what @mondrake thinks about this.

πŸ‡³πŸ‡±Netherlands Spokje

Rebased to prove that review-bot was wrong, which it was.
Also RTBCed.

πŸ‡³πŸ‡±Netherlands Spokje

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

πŸ‡³πŸ‡±Netherlands Spokje

I don't see the relation with ??=.

There is indeed no relation, HEAD was broken for a while πŸ› ManageGitIgnoreTest failing in HEAD RTBC .
Is fixed now, a rebase should make the error go away.

πŸ‡³πŸ‡±Netherlands Spokje

Since we've only just started removing deprecated code from 11.x, I've been assuming that new deprecations for 11.x removal will get caught by 'remove deprecated code in x' module issues or a final sweep before beta.

Fine by me, although I think this is optimistic and at some point (maybe not in 10.3.x, but still, at some point) we will discover a big BC-issue at the last minute when doing such a final scan.

Then again, there's always a fine balance between too many and too few rules and extra work for deprecations, so let's run with this :)

πŸ‡³πŸ‡±Netherlands Spokje

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

πŸ‡³πŸ‡±Netherlands Spokje

HEAD unbroken, rebased, all is well...

πŸ‡³πŸ‡±Netherlands Spokje

Failure due to (already spotted) broken HEAD: πŸ› ManageGitIgnoreTest failing in HEAD RTBC

πŸ‡³πŸ‡±Netherlands Spokje

Code changes make sense.

πŸ‡³πŸ‡±Netherlands Spokje

Am a bit confused, but shouldn't the 11.x commit remove the core/modules/editor/js/editor.admin.js JS APIs?
Or is this happening in a follow-up?
Or am I just not reading this correctly?

πŸ‡³πŸ‡±Netherlands Spokje

Down to 7 PHPStan errors

πŸ‡³πŸ‡±Netherlands Spokje

No worries, as long as the end result is less errors :)

πŸ‡³πŸ‡±Netherlands Spokje

Rebased: Found 23 errors

πŸ‡³πŸ‡±Netherlands Spokje

Thanks @andypost!

Made some minimal changes to the CR, most notably the link to the recommendations.

6. Do a thorough search of core for any remaining references to the extension. If references are found, outside of the extension, then creates issues to remove the references.

Nice job on finding the two stragglers, the above from the IS would suggest making separate issue(s) for them.
Looking at how minimal the changes are, I think it's not worth the admin-overhead, so I'm marking this RTBC.

πŸ‡³πŸ‡±Netherlands Spokje

There's a (IMHO) valid question from @andypost in the MR and it needs a reroll/rebase after the PHPStan-PHP-Baseline landed, but otherwise this looks fine to me.

πŸ‡³πŸ‡±Netherlands Spokje

Thanks @andypost for the removal of just the ExpectDeprecationTrait.

πŸ‡³πŸ‡±Netherlands Spokje

Looks good, but left comment in the MR.

πŸ‡³πŸ‡±Netherlands Spokje

Rebased and bumped symfony/* to 7.0.5.
After the rebase:
58 errors

πŸ‡³πŸ‡±Netherlands Spokje

The only remaining references to modernizr in the MR is are core/misc/touchevents-test.js, and that leads to its appearance in core/misc/cspell/dictionary.txt.
According to #3239980-41: Deprecate Modernizr β†’ and #3239980-42: Deprecate Modernizr β†’ is fine.

Production build https://api.contrib.social 0.62.1