Problem/Motivation

Amongst the current suppressions found in the PHPStan level 1 baseline is: Variable $foo might not be defined..

This issue exists to fix all of those caused by non-exhaustive switch statements. See https://github.com/phpstan/phpstan/issues/7521#issuecomment-1164581820

Steps to reproduce

- Run PHPStan on level 1 and see the above issue amongst all others.

Proposed resolution

- Solve all of the reported issues for the above mentioned.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3353918

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Spokje created an issue. See original summary.

Spokje opened merge request !3823

spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review
spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Don't mind marking but is the plan to tackle the others slowly? Seems to be 126 other instances in the baseline file.

spokje’s picture

spokje
Primary language Dutch
commented

Don't mind marking but is the plan to tackle the others slowly? Seems to be 126 other instances in the baseline file.

That's at least my plan. Can't see a patch/MR with 126+ changes which have mostly unrelated causes being reviewable.
So IMHO it's better to "nibble" at that massive number by grouping some by cause.

Granted: There's no way to ensure we get them all per issue, since it's a manual process and some can be missed.
But they will stand out when the number drops to something reasonable, and we can them identify and fix them easily.

spokje’s picture

spokje
Primary language Dutch
commented

Rebased after weirdly TestBot didn't kick this back down to NW after failing to merge since 21 Apr 2023 at 13:56 CEST.

.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Parent issue: » #3358600: [meta] Fix PHPStan L1 errors "Variable $foo might not be defined."

Adding a parent meta, to cope with #5 and #6.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work

sorry to push this back, just a couple of points

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @mondrake, fixed one thread, left a comment in the other.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

thanks, two nits but do not stop RTBC

spokje’s picture

spokje
Primary language Dutch
commented

Thanks @mondrake agiain, think/hope i picked yer nits.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Reviewed & tested by the community » Needs work

Needs a rebase on 11.x

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Reviewed & tested by the community

Rebased

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Overall I like this change as it removes a lot of verbosity, but in some cases perhaps we have gone a bit too far in making the code very dense and hard to understand.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @longwave, resolved all threads.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

One more simplification, and I think a bug has been introduced.

rpayanm made their first commit to this issue’s fork.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Please review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Points from threads appear to have been addressed.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Reviewed & tested by the community » Needs review

Added some remarks (just to show I'm a grumpy old man ;)

Back to NR for those.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Resolved last open thread and rebased.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

All threads appear to be resolved.

Thanks @Spokje!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

match() throws UnhandledMatchError if no cases are matched, and I was wondering if this could somehow be a behaviour change in the asset groupers. I don't think there is anything stopping someone declaring an unhandled type - the default is file, and external is supposedly the only other value, but at the moment it looks like this will cause either a warning or perhaps just a misgrouping, and this behaviour will change now?

I also am a bit confused about this in AssetResolver::getJsAssets(), I couldn't see if or how type setting is somehow handled elsewhere:

      $settings_as_inline_javascript = [
        'type' => 'setting',
        'group' => JS_SETTING,
        'weight' => 0,
        'data' => $settings,
      ];

I don't have any such concerns about the other issues, it seems OK for those to trigger errors if they try to match unexpected values.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

JsCollectionRenderer throws a LogicException if it's not a file, external or setting type, but I still don't see how settings don't end up in the grouper. It feels like these should be refactored to be value objects instead of arrays with a type key...

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

JsCollectionRenderer throws a LogicException if it's not a file, external or setting type, but I still don't see how settings don't end up in the grouper. It feels like these assets should be refactored to be value objects instead of arrays with a type key...

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

For #27 if I'm reading that correctly.

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: +Needs followup

@longwave:

It took me a while to wrap my head around your comments, but the whole behaviour in the asset groupers is weird/bad/unwanted/name-your-posion.

If type is not file nor external, it looks like, besides throwing a notice (Undefined variable $group_keys) when the asset is the first in the assets-array, the asset at least gets grouped with the asset before it.

So at the very least, we need a followup to sort that out.

I can't imagine there being much non-file/external assets around in the wild, but the change in the current MR would indeed break those.

Let's sort both CssCollectionGrouper and JsCollectionGrouper in the follow-up issue.

spokje’s picture

spokje
Primary language Dutch
commented

We _could_ add a default to the current switch statements in CssCollectionGrouper and JsCollectionGrouper where we throw a deprecation if the type is neither file nor external to warn people early.

But that could also be better to do in the follow-up?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Rebased, let's revive this and try to get these cases fixed, the asset grouping ones can be fixed elsewhere.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Seems no open points now.

  • alexpott committed b2c9207d on 10.3.x 
    Issue #3353918 by Spokje, longwave, rpayanm, mondrake: Fix PHPStan L1...

  • alexpott committed 519dc8de on 11.x 
    Issue #3353918 by Spokje, longwave, rpayanm, mondrake: Fix PHPStan L1...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Discussed with @longwave, we agreed that UnhandledMatchError is better than the current behaviour of warning a variable is not set and then breaking in odd ways.

Merged back to 10.3.x - nice work everyone.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Spokje closed merge request !3823

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.