Multiple hooks no longer work with domain_config in some cases

Are you loading any module configuration as part of domain_config? That is, are you trying to enable/disable any modules on a per-domain basis?

We should think about how to write a test for this.

Adding domain_config to the DomainAliasNegotiatorTest does not cause a failure, which I would expect based on this report.

An automated test.

Please answer the questions in #2.

I'm going to need better steps to replicate. This does not affect Domain Alias, which is part of the module package.

If this report affected all modules, the Domain Alias tests would fail if Domain Config were added. The module would also fail to issue redirects in the browser if those are set.

That is also testable.

That means that replicating this issue now requires more information:

* Is the code in question something that I can see/install?
* What modules in the domain suite are you using?
* How is domain_config being handled? Does just turning it on cause the issue, or have you loaded some configurations? If so, how are those being loaded?

Here's my problem:

* I have the same custom code in Domain Alias, and this problem does not occur. (See domain_alias_domain_request_alter()).

* I cannot reproduce the issue that you have reported.

So I need to understand how to repeat this error reliably.

* What other modules do you have installed that might interfere with variable loading? In particular, if you are using Config Translation, that might cause an issue.

* Is this module enabled / working otherwise? (Simple question, I know, but I'm not sure what else to ask you.)

* Are you doing anything custom in settings.php or services.yml?

I can reproduce the problem. In our case we have 2 domains configured, with aliases for the OTAP environments. Only, the aliases weren't working. After disabling the Domain Configuration module the hook gets called again and aliases are working like they should.

This is STILL not a sufficient definition of how to recreate the issue.

Please send me your code.

Thanks!

What I see in my quick test is that if domain_config is enabled, then country_path_domain_request_alter() runs twice. Without domain_coinfig, it runs once.

Testing country_path 8.x-1.0-rc2 with Domain 8.x-1.x-dev and Drupal 8.4.

That means that I still cannot replicate the reported issue.

All I did was put a kint() statement in that function to see that it is being called.

It runs twice because of the code in DomainConfigOverrider::initiateContext(), but solving that has nothing to do with the original issue posted here.

@amarincolas what Drupal and module versions are you running? What behavior, if anything are you seeing in country_path_domain_request_alter()?

What, if any, settings are being overridden by Domain Config?

Are you running multilingual? What language negotiation?

_Maybe_ it's an 8.3.2 issue, however, that version is no longer supported. I will take a look at 8.3.7. But otherwise, I'm running much the same thing right now.

What _symptoms_ are you seeing that indicate the hook is not being fired?

What type of language negotiation are you using?

I have never been able to replicate that report.

Part of the debug callstack showing invocation of the hook.

There is something in your configuration that I do not have or that has not been reported.

What values are being overridden at the Domain Config level? Anything?

All I can do at this point is write a specific test case that shows if this works or not, like I mentioned in comment #6.

I have not been able to replicate this issue under any circumstances, and can't just continue to ask random questions to get the missing information.

The test will do the following:

* Add an implementation of hook_domain_request_alter() to the domain_config_test module.
* Write a specific test that enables domain, domain_config, and domain_config_test.
** Load some config changes.
** Test fire the domain request hook to see if it loads properly,

If anyone can write a test case that demonstrates this bug, please do.

Here's the pull request with automated test runners for Drupal 8.3 and 8.4 across a variety of PHP versions.

https://github.com/agentrickard/domain/pull/365 (Domain cannot be tested on drupal.org because of hostnames.)

And here's the patch that includes a test that shows why i cannot act on this report.

These new tests pass just fine on Drupal 8.3 and 8.4

I'm sorry but I am closing this issue as non-replicable.

I would try filing a report with the module that you are using that implements this hook. The problem is likely coming from elsewhere. Could be a service collision, I just can't tell.

It is defintely working as designed, as the test shows.

Before anyone reopens this issue, you need to provide complete steps to reproduce from a base installation of Drupal. That includes any modules enabled and configurations.

I also encourage people to rewrite the new test in order to prove a failure.

If we cannot reproduce the issue via testing, there is nothing that can be done.

First-off: Thanks for taking the issue seriously as seen by the amount of effort to reproduce it!

I looked into some more and I found the issue.

If a Middleware for the HttpKernel loads config the domain_config module is triggered. The domain_config module fetches the active domain (DomainConfigOverrider::initiateContext -> getActiveDomain). In this phase the modules aren't loaded yet though, causing the domain_request_alter hooks to be missed.

I experienced the issue because I had the module 'shield' enabled. Enabling this module suddenly makes reproducing this issue easy.

1. Add 2 domains with at least 1 alias.
2. Add content that is available on domain 1
3. Keep shield disabled
4. Visit the content on domain 2: you should get a 404/403.
5. Enable shield
6. Visit the content on domain 2: you will see your content (which is incorrect).

I updated the IS to reflect these findings.

I made a test-patch to fix the problem. This checks if all modules are loaded in the initiateContext method to make sure the alter hooks will be called.

The question is though where this should be fixed.

The domain_config module enables this behavior because it requests the domain in a bootstrap phase where loading config is not unlikely. The DomainNegotiator is the one though that uses the hook while the modules aren't loaded yet.

If this check is a solution the following paths are possible as I see it:

1. This patch, check for it in domain_config
2. Check for it in DomainNegotiator::setRequestDomain and return early without setting the domain (is that even possible i.e. will that not break a lot of code?)
3. Check for it in DomainNegotiator::setRequestDomain and if not loaded: load all modules

@michielnugter YOU ARE MY NEW FAVORITE PERSON!

In this case, I think we may have to load modules in the Negotiator. It assumes that bootstrap has completed.

This was a problem in D7 and lower, and we had to introduce our own bootstrap phase to handle it (which is why we loaded and include into settings.php).

The middleware issue is interesting. If it doesn't interfere in rendering, we might simply delay domain_config (and domain negotiation loading) until its ready.

The other option (which we used in D7) is to keep a registry of modules that implement hook_domain_request_alter() and run those directly from our code.

We may need to figure out the difference between Shield's use of http_middleware as a service as opposed to our event_subscriber DomainSubscriber.

I haven't worked with http_middleware, so I'm not sure if that matters.

@michielnugter YOU ARE MY NEW FAVORITE PERSON!

Nice :) Was a lot of fun actually figuring this one out, never dove this deep into the Middleware and bootstrap of Drupal 8.

In this case, I think we may have to load modules in the Negotiator. It assumes that bootstrap has completed.

It would definitely fix the issue here. I do question if doing a part of the bootstrap is the best solution. It somehow feels incorrect.

I think it would be ideal to delay the domain detection untill the preHandle() phase of DrupalKernel has been executed. As today was my first dive into the DrupalKernel I'm not completely sure on how to achieve it. I do think it's possible in a better way than it was in Drupal 7.

Small update: the problem with the hooks is a little bigger, not only the domain_request_alter hook wont work correctly but all hooks encounter problem. I had a different issue with tokens not working in URL alias which was caused by the tokens hook not beeing properly initialized in the getImplementations().

Leaving Priority on Normal for now as you do need an additional contrib to cause it, feel free to change it though.

We may need to figure out the difference between Shield's use of http_middleware as a service as opposed to our event_subscriber DomainSubscriber.

I hope to be able to make some time tomorrow to see if I can make something of this.

Honestly, I don't think Domain Config is the problem.

The problem is in Shield's invocation of the Config Factory prior to the entire bootstrap being finished. Config overrides are a valid process that Shield (or core) needs to account for. If the http_middleware service can break hooks, that's a problem.

I wonder if you can duplicate the issue with Config Translation? If not, that means we likely have to rewrite everything about how Domain Config works -- and update all the documentation about configuration overrides.

That said, here's a patch that fixes the direct issue with Domain. Forcing the module reload() here might solve the other problems.

Again, testing happens on Travis, so you can follow the test results here:

https://github.com/agentrickard/domain/pull/366

If the http_middleware service can break hooks, that's a problem.

Its super tricky. Middlewares are designed to be as early as possible and doing as little things as possible.
For example the page_cache middleware is executed before \Drupal\Core\DrupalKernel::preHandle which does a good amount of the bootstrapping.

Sadly the shield module has to return even before the page_cache middleware, which means at the point of the shield middleware, module hooks aren't loaded yet.

There is an issue to potentially solve this: #1905334: Only load all modules when a hook gets invoked ... this would load the modules right when they are needed. Maybe earlier, maybe later.
I'm curious whether we could actually achieve the same thing already by calling $this->loadAll(); inside \Drupal\Core\Extension\ModuleHandler::buildImplementationInfo

Does someone mind trying this out for this particular problem?

@michielnugter
You are my favourite person too!

Wow, high praise again, it's not that special ;)

I tried doing that in my current project and this caused another interesting error:

Fatal error: Call to undefined function module_load_include() in [...]/modules/contrib/token/token.module on line 217

Obviously not a core issue but adding this there will still break a lot of contribs.

Looking at the preHandle it does loadLegacyIncludes() first before loading the modules. This would fix the issue but then we would be doing more and kind-of reimplementing preHandle(). This seems wrong here.

I took a quick look at the patch in the mentioned issue but that's quite a bit larger than I can quickly take in. I hope to be able to continue next week.

I took a deeper dive and came to the following conclusions.

The mentioned issue is a little different in scope as it only talks about lazy loading. It does have some overlap but the main question there is whether we can get away with loading on-demand.

The issue here is that if you invoke a hook it is possible that the cache for the hooks will be built incorrectly.

There actually is some safeguard in core for this ModuleHandler::buildHookInfo():

// Make sure that the modules are loaded before checking.
$this->reload();

But, in this case the getHookInfo() returns early because the HookInfo because that part is cached, thus missing the safeguard.

ModuleHandler::buildImplementationInfo continues to check if the functions the requested hook exist for the moduleList and will fail because the modules aren't loaded yet.

To me it seems like a core bug, you can request/execute a certain hook and get an incomplete result. Adding the isLoaded() and loadAll() in ModuleHandler::buildImplementationInfo would solve that.

Any thoughts on my conclusion?

I do think it's a core bug. Have you tried the PR code from #33? https://github.com/agentrickard/domain/pull/366

I commented on your pull request in github, the patch works. I don't have much experience with github yet and couldn't figure out in the time I have right now (work..) to make a patch/PR on your branch. Hope my comment is enough for now.

That was great. I was out last week and forgot that #32 here is the patch from that PR, so testing the patch is sufficient.

I do want to create a test for this, which mimics what Shield is doing.

Here's a patch version where I try to replicate what Shield module is doing. However, I can't make the test pass, and don't know why.

Personally, I think we should move this issue (or create a new one) for the Shield module. Its use of Middleware is the cause of the problem and doesn't even have any test cases.

I dug into the test and found the following.

I don't think it's going to work this way as the KernelTest did not do any request, logically. Because of this the added Middleware is never invoked. To make it work I think it might need to be a functional test. The Drupal\Tests\system\Functional\Form\ResponseTest is a test for a Middleware which serves as a good example I think.

Still not sure on where the patch+test needs to go though. Shield has no coverage at all and in this case triggers the error. I can imagine though that other contribs do the same. I'm also not sure on if fetching config is wrong in the Middleware. To me the choice is 'fix'/work-a-round in domain to harden against this implementation or fix fetching the hooks in core.

Either way this test will be a good starting point for both. I'll try to work on a failing and passing test somewhere today/tomorrow.

I posted a working test on github for the issue which correctly fails if I remove the patch.

I attached my diff from origin/alter_hook on this issue. I also uploaded it zipped on the github issue. Let me know what you think.

EDIT: I've been thinking on the best way forward on this issue. I think a workable approach would be to harden Domain against early hook invocation, create an issue for Drupal core on the hooks-thing and add a @todo in Domain referencing the issue with the note to rework once the Drupal core issue is in. Would this be workable?

EDIT 2: Also I couldn't resist changing the other assertions. You should only do assertTrue when asserting a boolean. Otherwise use the correct assertion methods.

OK, with @michielnugter's help, we've put in a fix for the initial problem.

I think there are other issues that Shield module / core may need to deal with, but those can be put into separate issues targeted to the appropriate place.

Please don't re-open this issue, let's open new ones if a simialr problem comes up.

I found a very similar issue to this, and am opening 3025541 to demonstrate this can still happen.