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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

david.gil created an issue. See original summary.

david.gil’s picture

Status: Active » Needs review
FileSize
1.25 KB

Here is the patch

Berdir’s picture

Note 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.

david.gil’s picture

Sure Berdir,

for this patch to work, you must first add the patch in the core issue related to this.

Best

Berdir’s picture

Yes, 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)

agentrickard’s picture

So for the moment this only works on 8.6.x when the core patch is also installed?

david.gil’s picture

Hi 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.

agentrickard’s picture

Yes, but what version(s) of core does that patch apply cleanly to?

Berdir’s picture

I'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.

agentrickard’s picture

Testing 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.

david.gil’s picture

Status: Needs review » Needs work

Hi Ken,

core patch is now in 8.5.

agentrickard’s picture

I 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.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

And a patch that should work on 8.4, 8.5. and 8.6.

Testing at https://github.com/agentrickard/domain/pull/411

Berdir’s picture

I 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.

agentrickard’s picture

I 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.

agentrickard’s picture

And this fix -- https://www.drupal.org/project/domain/issues/2976127 -- would drop support for 8.4, so that shim can go.

agentrickard’s picture

Status: Needs review » Fixed

This has been committed.

  • agentrickard committed 50b5c52 on 8.x-1.x authored by david.gil
    Issue #2940296 by david.gil, Berdir: Combination of language negotiation...
  • agentrickard authored e106212 on 8.x-1.x
    Merge pull request #411 from agentrickard/route
    
    Issue #2940296 by david...

Status: Fixed » Closed (fixed)

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