With the release of mglaman/phpstan-drupal 1.1.37 (https://github.com/mglaman/phpstan-drupal/releases/tag/1.1.37) the false positives on "Missing cache backend declaration for performance." should be gone. (https://github.com/mglaman/phpstan-drupal/pull/592)
Let's bump mglaman/phpstan-drupal to 1.1.37, remove the ignore from core/phpstan.neon.dist and see where we stand.
If the false-positives are indeed gone, we can fix all valid remaining fails on this one in this issue.
- Bump mglaman/phpstan-drupal to 1.1.37
- Remove "Missing cache backend declaration for performance." from core/phpstan.neon.dist
- Evaluate/fix any remaining failures from that (now un-ignored) rule.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 3377000-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Show commandsStart 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:
changes, plain diff MR !4478
Comments
Comment #3
spokje16 New errors without the
Missing cache backend declaration for performance.rule.At first (quick) glance they all seem legit.
Comment #4
longwaveLanguageNegotiationMethodManager could call
setCacheBackend()instead of initialising the properties itself. In fact it looks like the$cacheKeyPrefixis 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.
Comment #5
spokjeBy 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?
That would leave only
\Drupal\Core\Menu\MenuLinkManageras a valid flagged failure.Comment #6
spokjeSince 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 to #3377318: Remove cruft from LanguageNegotiationMethodManager.Also:
Performance is indeed a bit of a non-issue in tests, but shouldn't we force it anyway, for Best Practice-reasons?
Comment #7
longwaveYour 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.
Comment #8
spokjeThat'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 #3377318: Remove cruft from LanguageNegotiationMethodManager gets committed to prevent us dancing the Reroll-Rumba.
Comment #9
spokjeOpened https://github.com/mglaman/phpstan-drupal/issues/598 upstream to handle #5.
Leaving this postponed for a little bit, while I try to get a PR up there.
Comment #10
spokje@longwave in #7
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.
Comment #11
smustgrave commentedMR 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?
Comment #12
needs-review-queue-bot commentedThe 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.
Comment #13
xjm@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.
Comment #14
xjmComment #15
longwaveThe 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!