Closed (fixed)
Project:
Drupal core
Version:
9.4.x-dev
Component:
base system
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
18 Jan 2022 at 11:06 UTC
Updated:
31 Oct 2022 at 18:34 UTC
Jump to comment: Most recent
Show commands
changes
Comments
Comment #3
mondrakeComment #4
mglamanThis looks good to me! I opened an issue for phpstan-drupal to fix the return type for config entities, which is why it had to be set in this MR. Not sure if we want to add a `@todo` next to that statement that it should be able to be removed once phpstan-drupal has a release with the fix.
Comment #5
neclimdul+1 to the extend array syntax used here but this seems to be the first use of it in core. Putting this on people's radar for future adoption. #3259019: Errors when using newish array syntax for docs
Comment #6
mglamanComment #7
daffie commentedPostponing this issue on phpstan-drupal fixing: https://github.com/mglaman/phpstan-drupal/issues/308
Comment #8
mondrakeI think we can proceed now; after #3260928: Move more PHPStan global ignore patterns to baseline this can also be partial if some things can’t be solved yet.
Comment #9
mondrakeAh no it's useless to do this without #3259355: Always do a full phpstan analysis on DrupalCI.
Comment #10
mondrakeActually no need to postpone if we change the baseline directly.
Comment #11
mondrakererolled
Comment #12
daffie commentedAll the changes look good to me.
Comment #13
mondrakererolled
Comment #14
mondrakerebased
Comment #16
longwaveMerged in HEAD, but I think a few of the cases need revisiting, many of the tests are hardcoded so there is no point asserting something is missing when we explicitly set that already a few lines above.
Comment #17
andregp commentedI'll see what I can do to address the above points from @longwave.
Comment #18
andregp commentedI hope this helps.
Comment #19
longwaveThank you @andregp!
The NewDefaultThemeBlocksTest change can be reverted as well, as it is fixed in newer versions of mglaman/phpstan-drupal.
Comment #20
andregp commented@longwave, done. :)
Comment #21
longwaveThanks again - just spotted another three cases I think we can fix instead of ignoring.
Comment #22
andregp commented@longwave, sorry for the late response.
I removed the
phpstan-ignorelines and cast $random to string as suggested.Comment #23
mondrakeiirc, $GLOBALS does not really play nice with PHPStan, so I went for ignoring the lines altogether
Comment #24
mondrakeComment #25
longwaveFor some reason PHPStan treats numeric-string and string differently here, but forcing the random key to be a full string works for me locally.
Comment #26
daffie commentedAll code changes look good to me.
Back to RTBC.
Comment #27
mondrakeNeeds rebasing
Comment #28
longwaveMerged 10.0.x, back to RTBC.
Comment #29
mondrakeComment #30
longwaveUnrelated fail in HEAD, now fixed.
Comment #31
mondrakeRebased, back to RTBC
Comment #32
mondrakerebased
Comment #33
mondrakerebased
Comment #34
mondrakerebased
Comment #35
mondrakererolled
Comment #36
mondrakerebased
Comment #37
mondrakerebased
Comment #38
alexpottDid a review in gitlab. I think the MR needs some changes before we can merge.
Comment #39
mondrakeRebased and addressed the feedback on the MR
Comment #40
daffie commentedAll the code changes look good to me.
The file core/phpstan-baseline.neon has the exceptions removed.
For me it is RTBC.
Comment #41
mondrakeSorry, I realized there were more unsets to remove in
ActiveLinkResponseFilterTest, needs to RTBCed again.Comment #42
daffie commentedYour last change does not remove any exceptions from core/phpstan-baseline.neon.
Comment #43
mondrakeWas addressing this comment from @neclimdul
In other words, this is to keep consistency across the test code, not to remove baseline entries.
Comment #44
daffie commentedThe testbot is not happy with the change to core/tests/Drupal/Tests/Core/EventSubscriber/ActiveLinkResponseFilterTest.php
Comment #45
mondrakeShould be fixed now.
Comment #46
daffie commentedAll the code changes look good to me.
Back to RTBC.
Comment #47
alexpottCommitted 74b0acb and pushed to 10.1.x. Thanks!
Committed ab04257 and pushed to 10.0.x. Thanks!
Committed and pushed 0bab40ee30 to 9.5.x and 59f14c20d0 to 9.4.x. Thanks!
Backported to 9.4.x and 9.5.x without the PHPStan changes to keep the codebase aligned. The only non test change is a harmless one to core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
Comment #53
mondrakeFiled #3315808: Fix TermStorage serialization for follow-up.