Problem/Motivation

Here is the error I have on Drupal 10:

Exception: TypeError: Drupal\variationcache\Cache\VariationCacheFactory::__construct(): Argument #1 ($request_stack) must be of type Drupal\Core\Http\RequestStack, Symfony\Component\HttpFoundation\RequestStack given, called in /var/www/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php on line 262

variation_cache_factory service definition:

  variation_cache_factory:
    class: Drupal\variationcache\Cache\VariationCacheFactory
    arguments: ['@request_stack', '@cache_factory', '@cache_contexts_manager']

(the constructor is expecting \Drupal\Core\Http\RequestStack as first argument)

However, request_stack service definition is:

  request_stack:
    class: Symfony\Component\HttpFoundation\RequestStack

I think there was some oversight in #3298801: Drupal 10 compatibility fixes, where this change was introduced. As I understand, we should no longer use a shim class. The tests passed because they don't cover the factory class.

Steps to reproduce

1) Install Drupal 10;
2) Do something that will trigger variation_cache_factory (in my case it's a test dependent on group module);

CommentFileSizeAuthor
#12 3306650-12.patch3.24 KBmatroskeen
#4 vc-3306650-4.patch1.27 KBkristiaanvandeneynde

Comments

Matroskeen created an issue. See original summary.

matroskeen’s picture

matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
commented
Issue summary: View changes
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Argh, I thought I got this.

As I understand, we should no longer use a shim class. The tests passed because they don't cover the factory class.

It's to be used to get from Drupal 9 -> Drupal 10. It's deprecated in Drupal 10.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Active » Needs review
StatusFileSize
new vc-3306650-4.patch1.27 KB

Uh wait, so what's the fix here?

I see the following in core.services.yml (9.5.0):

  request_stack:
    class: Drupal\Core\Http\RequestStack
    tags:
      - { name: persist }

And in Drupal 10.0.x:

  request_stack:
    class: Symfony\Component\HttpFoundation\RequestStack
    tags:
      - { name: persist }

So do I need to release separate versions for D9 and D10, 'cause that would be a huge pain in the ass over something so small :/ Given how the Drupal shim extends the Symfony one, can't we keep typehinting for the Symfony one and be done with it?

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Hm. You shouldn't need separate versions. The test passed before because they are Unit tests, no Kernel tests to verify the services configuration. I guess I provided the wrong fix? But the CR said to use the Drupal class. I didn't realize Drupal 10.0.x already changed the services.yml to switch back.

If we had a kernel test we could verify. Unit tests still passed on D10 with the patch.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

I'm fairly sure the fix here will work because the Drupal shim extends the class we're now typehinting. So both versions should work when typehinting the Symfony class.

I'm not sure what there is to Kernel test in this module, whether services load properly seems like an odd thing to test as it's expected from Symfony/Drupal that any well-defined service loads. This module is entirely an API that builds on top of Drupal's cache bins, the API part is well tested using unit tests.

What we're seeing here is a Drupalism between two major versions. I don't think we should write tests for every change record we implemented going from D9 to D10? That said, we do need to verify whether the patch here works, even though I'm 99% sure it does.

matroskeen’s picture

matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
commented

I did some digging in Symfony-compatibility tasks and here is my understanding:

Drupal 9 depends on Symfony 4, where Symfony\Component\HttpFoundation\RequestStack does not have ::getMainRequest() method.
Drupal 10 depends on Symfony 6, where Symfony\Component\HttpFoundation\RequestStack does not have ::getMasterRequest() method.

That's why there is a shim, which has both and was supposed to make the transition easier. We're supposed to use ::getMainRequest() method now and use this shim from Drupal 9.3 to Drupal 10, and then go back to Symfony class from Drupal 10.

Given that core.services.yml defines the correct class in the service definition, we should be able to use a parent class as mentioned in #6. The only downside is that for Drupal 9 installations it might not understand where this method is coming from. The PHPStorm shows the following inspection for this line:
return (int) $this->requestStack->getMainRequest()->server->get('REQUEST_TIME') + $max_age;
Potentially polymorphic call. The code may be inoperable depending on the actual class instance passed as the argument.

Are we fine with that? If so, we can probably recommend this approach for other modules in one of the change records:

If we go with this approach, we should change typehints to Symfony\Component\HttpFoundation\RequestStack in VariationCache class as well.

There is also an example of how to stay compatible with previous versions: #3258992: Replace getMasterRequest() with getMainRequest() for Drupal 9.3.x.

matroskeen’s picture

matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
commented

Speaking of tests, I think that enabling automated testing for Group module with Drupal 10 stack will reveal this issue.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Re #7 I don't think IDEs complaining is a big deal given how D10 is around the corner and the current code works on D9, but not D10. Also, let's be honest: VC is an API module, who else but the maintainers is going to dig into the code, let alone worry about an IDE hint not being fully accurate when on D9?

matroskeen’s picture

matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
commented

I agree, but what about other modules? Can we recommend this approach to others, so they don't hit the same wall? I'm curious to ask people involved in Symfony-related tasks.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

I think having an IDE throw a hissy fit for a short while when we know it's harmless won't be the biggest concern for many maintainers. Hell, I've seen a lot of contrib (and core!) code that doesn't even typehint or assert properly where the IDE is left in the dark too.

matroskeen’s picture

matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
commented
StatusFileSize
new 3306650-12.patch3.24 KB

Ok, let's do this! I'm uploading a new patch with VariationCache fixes and really looking forward to have this committed and available in a new release :)

Anonymous’s picture

Anonymous (not verified) commented

#12 >>> 3306650-12.patch worked for me.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Title: VariationCacheFactory contructor has incorrect parameter type » VariationCacheFactory constructor has incorrect parameter type
Related issues: +#3278740: D10 compatibility

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Fixed

Fixed, thanks!

Status: Fixed » Closed (fixed)

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