Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hi,
Domain_Config is affected by this core issue:
https://www.drupal.org/project/drupal/issues/2802403,
in DomainRouteProvider class we override getRouteCollectionForRequest, and in core it has a bug that make that in multilanguage sites it gives aleatory 404 in some routes. We must add language to the cache ID.
I send a patch.
Best
Comment | File | Size | Author |
---|---|---|---|
#13 | 2940296-route.patch | 1.63 KB | agentrickard |
#2 | routing-cache-language-2940296-1.patch | 1.25 KB | david.gil |
Comments
Comment #2
david.gil CreditAttribution: david.gil commentedHere is the patch
Comment #3
BerdirNote that this will then require the core version that the core patch will be committed against, not sure if it will be backported to 8.4, possibly. Without a supported core version, this will be a fatal error as $this->languageManager will not exist.
Comment #4
david.gil CreditAttribution: david.gil commentedSure Berdir,
for this patch to work, you must first add the patch in the core issue related to this.
Best
Comment #5
BerdirYes, I knew you were aware of this. This was an information for people who want to try the patch and the maintainer that he needs to consider that when committing this/releasing a new version (e.g. by addingadding an explicit version dependency on whatever core version that will contain the fix in core)
Comment #6
agentrickardSo for the moment this only works on 8.6.x when the core patch is also installed?
Comment #7
david.gil CreditAttribution: david.gil commentedHi Ken,
it only works only if you first patch core with this patch:
https://www.drupal.org/project/drupal/issues/2802403#comment-12452920
or you modify my patch to obtain the current language using directly the LanguageManager service and not DI in the constructor.
Best.
Comment #8
agentrickardYes, but what version(s) of core does that patch apply cleanly to?
Comment #9
BerdirI'd expect the patch applies to 8.4+, but the question is against which versions it will be committed in the end. It is a critical bugfix, which means it is still viable for 8.4 but since it's a constructor change, might not get in, but i really hope it will get into 8.5.
Comment #10
agentrickardTesting notes:
* The original cache behavior is covered in DomainConfigHomepageTest.
* That needs to be extended to cover languages as well, see RouteCachingLanguageTest in the core patch.
The larger question here is if we have to move or adjust our routing behavior. At the moment, only Domain Config cares about the route cache and that was because the homepage was getting cached incorrectly.
We may need to assume that any code that relies on Domain may run into this issue, which would mean moving that service change into the main module.
See for instance, how DomainRedirectResponse is handled.
Perhaps the DomainRouteProvider needs to be in the main module and can be swapped in when other modules request?
That aside, its implementation needs to check for the presence of the LanaguageManager service and load it if needed.
Comment #11
david.gil CreditAttribution: david.gil commentedHi Ken,
core patch is now in 8.5.
Comment #12
agentrickardI just looked at this code, and there is an easier solution for 8.5 and 8.6, but it breaks 8.4, since https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Routing%2... was just added.
Comment #13
agentrickardAnd a patch that should work on 8.4, 8.5. and 8.6.
Testing at https://github.com/agentrickard/domain/pull/411
Comment #14
BerdirI don't think there's anything wrong with requiring 8.5 in a newer version of domain, you'd just add drupal:system (>= 8.5) in info.yml, and then users see that they should not update that module before the update to 8.5. Otherwise you have to keep your implementation in sync with whatever core is doing there, will just overriding the cache id method is less likely to get out of sync.
Comment #15
agentrickardI have committed this (with a shim for 8.4 support), but need to fix https://www.drupal.org/project/domain/issues/2976127 before pushing to Drupal.org.
Comment #16
agentrickardAnd this fix -- https://www.drupal.org/project/domain/issues/2976127 -- would drop support for 8.4, so that shim can go.
Comment #17
agentrickardThis has been committed.