Fix PHPStan L1 errors "Variable $foo might not be defined." caused by non-exhaustive switch statements

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

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.

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

.

Adding a parent meta.

sorry to push this back, just a couple of points

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

thanks, two nits but do not stop RTBC

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

Needs a rebase on 11.x

Rebased

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.

Thanks @longwave, resolved all threads.

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

Please review.

Points from threads appear to have been addressed.

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

Back to NR for those.

Resolved last open thread and rebased.

All threads appear to be resolved.

Thanks @Spokje!

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.

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...

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...

For #27 if I'm reading that correctly.

@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.

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?

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

Seems no open points now.

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.

Spokje → closed merge request !3823

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