Remove phpstan ignore for "#^Missing cache backend declaration for performance.#"

16 New errors without the Missing cache backend declaration for performance. rule.
At first (quick) glance they all seem legit.

LanguageNegotiationMethodManager could call setCacheBackend() instead of initialising the properties itself. In fact it looks like the $cacheKeyPrefix is not even used so enabling this has caught some historical cruft.

The migrate plugins look like false positives; it might be worth raising an upstream bug, given that setCacheBackend() is actually called in the parent constructor.

Most of the others are test cases where we don't really care about performance.

The migrate plugins look like false positives; it might be worth raising an upstream bug, given that setCacheBackend() is actually called in the parent constructor.

Most of the others are test cases where we don't really care about performance.

By what I gather of the current implementation, every plugin class is now required to have a setCacheBackend() in it's __construct().

What I gather of the above quote is:

We want setCacheBackend() in every plugin class __construct(), unless:
- it's the base class, where we define the base class as being the class that has the implementation of setCacheBackend().
- it's a test class.
- it has a parent that already has a setCacheBackend() in it's __construct().

Is that about right?

Since this is going to be postponed on a Yet To Be Created upstream issue, I've split off the low hanging fruit LanguageNegotiationMethodManager-bit from #4 📌 Remove phpstan ignore "#^Missing cache backend declaration for performance.#" Needs review to 📌 Remove cruft from LanguageNegotiationMethodManager Fixed .

Also:

Most of the others are test cases where we don't really care about performance.

Performance is indeed a bit of a non-issue in tests, but shouldn't we force it anyway, for Best Practice-reasons?

Your summary in #5 looks correct to me.

In this issue, should we just add all the cases to the baseline, to ensure any future plugin managers are caught, and open upstream issue(s) as appropriate? Then we can handle fixing the baselined issues as we normally do in followups, or when phpstan-drupal updates and breaks the updated deps build.

In this issue, should we just add all the cases to the baseline,

That's indeed probably the best way to ensure any progress, or at least no new regressions creeping in.
Let's wait until the already RTBC-ed 📌 Remove cruft from LanguageNegotiationMethodManager Fixed gets committed to prevent us dancing the Reroll-Rumba.

Opened https://github.com/mglaman/phpstan-drupal/issues/598 upstream to handle #5 📌 Remove phpstan ignore "#^Missing cache backend declaration for performance.#" Needs review .

Leaving this postponed for a little bit, while I try to get a PR up there.

In this issue, should we just add all the cases to the baseline, to ensure any future plugin managers are caught, and open upstream issue(s) as appropriate? Then we can handle fixing the baselined issues as we normally do in followups, or when phpstan-drupal updates and breaks the updated deps build.

@longwave in #7 📌 Remove phpstan ignore "#^Missing cache backend declaration for performance.#" Needs review

Sorry, this got lost somewhere in my torrent of ToDo's.

The upstream PR is proving more difficult than expected, also it's hard testing over there when core has the rule messages ignored.
So let's indeed just add to baseline and take it from there.

MR seems to be doing what @spokje says in #10 but not sure the pro of moving from the .dist to 15 entries in the baseline?

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@smustgrave Moving the entries to the baseline means we can fix them individually as needed, when appropriate to the context, and sort legit failures from any further false positives.

The bot was wrong, this still applies for me. Let's just get this in given it is is only baseline changes.

Committed and pushed 6514a92979 to 11.x and 1b80269cd5 to 10.2.x. Thanks!

Spokje → closed merge request !4478

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