Problem/Motivation

Unfortunately, there are aspects of PHP attributes and the cache system that I don't fully understand to be able to perfectly explain the issue here. However, I noticed during site template installation that Commerce Kickstart began failing at the point of installing default content. The error was fairly self-explanatory:

TypeError: Drupal\file\FileReferenceResolver::__construct(): Argument #2 ($memoryCache) must be of type Drupal\Core\Cache\MemoryCache\MemoryCacheInterface, Drupal\Core\Cache\MemoryBackend given in Drupal\file\FileReferenceResolver->__construct() (line 40 of /var/www/html/web/core/modules/file/src/FileReferenceResolver.php)

I just didn't know why it should suddenly start failing until I asked Claude to help me understand the issue. It pointed me to last year's change record related to the cache.memory cache bin, which demonstrates that autowiring should be paired with the more generic CacheBackendInterface class.

The FileReferenceResolver is actually newer, but the circumstances of its normal use do not fully match the circumstances of its use during site installation. I assume that's why the bug wasn't surfaced sooner. Additionally, full test coverage for the resolver is still pending.

Steps to reproduce

composer create-project centarro/commerce-kickstart-project kickstart
cd kickstart
ddev config
ddev start

And install the full demo store. It will crash out as soon as it gets to default content installation.

Proposed resolution

Update the typehint to use the more generic interface, as in #3571724: ModuleAdminLinksHelper should use CacheBackendInterface instead of MemoryCacheInterface for Drupal 11.3+ compatibility. After making the change locally, I can install just fine.

Remaining tasks

Review the MR.

Release notes snippet

TBD

Issue fork drupal-3608288

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

rszrama created an issue. See original summary.

rszrama changed the visibility of the branch 3608288-update-filereferenceresolverconstruct-to to hidden.

rszrama changed the visibility of the branch 3608288-update-filereferenceresolverconstruct-to to active.

rszrama opened merge request !16210

rszrama’s picture

rszrama commented
Status: Active » Needs review

Ok, prepared the MR. I wasn't sure if I was supposed to target 11.x-dev or 11.4.x-dev. Feel like I got it wrong last time.

In any case, while I used Claude for research, I coded and tested the patch myself based on the change record. Not sure what sort of attribution that's going to require here - open to guidance! Thanks. : )

rszrama’s picture

rszrama commented
Issue summary: View changes

rszrama opened merge request !16216

rszrama’s picture

rszrama commented
Version: 11.x-dev » main

Thanks to Gabor for reminding me to check main first. The issue is still a problem there, so I opened an MR against main and am updating the version number of the issue. Will still need to be merged back to 11.x / 11.4.x as well, though I'm not sure how to represent that in the issue queue. 😅

rszrama’s picture

rszrama commented

Best I can tell current test failures are from main itself, not my commit.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Component: file system » file.module
Status: Needs review » Reviewed & tested by the community

We're aware of edge cases around that, we also had that in other places. installer/update container builder overrides incorrectly change cache services in some cases.

It makes me a bit sad, because in my mind, it makes sense that services that explicitly want memory-only caching should type hint that accordingly, because for these scenarios, it should never receive anything else, that is undesired and could cause bugs/performance regressions. But it doesn't work then we'll have to deal with that for now.

on versions: core committers will handle backports when it applies by themself, and update the issue if it needs a manual rebase, bugfixes are by default backported to all supported releases. always main, worry about anything else later.

rszrama’s picture

rszrama commented

Thanks for the explanation / advice, Sascha!

catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

So the issue is from using MemoryBackend during runtime, despite it's name, it's not a real MemoryCacheInterface but a memory implementation of a persistent backend for testing purposes, that we put into a non-test namespace over a decade ago and never moved out, see #2973286: Deprecate MemoryBackend and move it to the test namespace for the issue to clean that up.

Until that issue is fully done we should probably just widen the type hint though.

Committed/pushed to main, 11.x and 11.4.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !16216

  • catch committed 06b8d836 on 11.4.x 
    fix: #3608288 Update FileReferenceResolver::__construct() to use the...

  • catch committed 6c1b45bf on 11.x 
    fix: #3608288 Update FileReferenceResolver::__construct() to use the...

  • catch committed e2bbdab7 on main 
    fix: #3608288 Update FileReferenceResolver::__construct() to use the...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

@catch: specifically, before we can do that we need to do #3223580: Use MemoryCache (not MemoryBackend) in the installer, which is what this issue is about. And my #3590964: Update/Install-time service overrides for cache should support MemoryCacheInterface which I guess is a duplicate but I started, so we could close yours?