Arbitrary null value from DomainNegotiator::getActiveDomain()

Trying a retest as the fails did not seem wholly related to the issue.

Also noting that I had issues with this in an access check hook, but calling `->getActiveId()` seemed to work.

This seems to cause a segfault on CR when I apply the patch.

I've spent a while looking into the segfault.

What I believe happens is that as part of resolving the domain a dummy domain can be created.

protected function setRequestDomain() ...
$domain = $this->domainStorage()->create($values);

Unfortunately in the process of creating a domain at least domain_alias_domain_load and Drupal\domain_language\LanguageDefault::get() are called, both of these call getActiveDomain() and we end up in a loop

It looks like these both assume that getActiveDomain can return null if not resolved and will handle it. This means that changing the behaviour is more of an API change than anticipated.

I'm not quite sure of the best approach, from here on in. It feels like there is some inconsistency around this. I note that there is some code to handle recursive call errors in LanguageDefault::get() so I am wondering if the dummy domain creation is causing more issues than we would expect. It doesn't seem obvious that resolving a domain would cause the creation of an entity. NB, I messed with this and it seems to trigger the recursive loop even if you load an entity before the resolver is complete.

One approach could be to have a method to explicitly check if the domain is negotiated rather than inferring this from the behaviour of getActiveDomain I've tested this locally and it seems to work, but it is an API change to what has been done previously. As one of the fixes which is needed is in domain_language I don't think we can do this.

This leaves the option of a new method (or attribute to getActiveDomain) which will explicitly load the domain if it is not loaded. Is getOrLoadActiveDomain() acceptable?

This seems to work well now. I'm not sure about the remaining test fail but the code looks OK and It seems to have fix some long-standing issues I have seen with the domain resolver.

I still need to see a reliable test for this case. It's likely happening at some very early stage of the bootstrap process.

What we need to know is what other modules or conditions trigger this error.

Re: #11

Ran into this bug with the pate module enabled, and occurs when $node->access('create', $account); is called as part of the access checks (and before Domain's event subscriber is called and Drupal's had a chance to cache the node create access permission check).

domain_access_node_create_access() doesn't have the same NULL check behaviour as in domain_access_node_access().

See 68ecdbf4eebc8127476bd5154082e919bf0c7029 commit.

Thanks!

Re-rolled patch as the previous one could not be applied, and can not do an intertdiff.
The patch should focus on this issue and not fix other stuff. Will make it harder to review and get it applied.
Still needs tests.

Tests which seem unrelated may be failing because the patch changes the method signature of negotiateActiveDomain(), which should return the value of the current domain.

   /**
    * Constructs a DomainNegotiator object.
    *
@@ -131,16 +138,16 @@ class DomainNegotiator implements DomainNegotiatorInterface {
    * Determine the active domain.
    */
   protected function negotiateActiveDomain() {
+    $this->negotiated = TRUE;
     $httpHost = $this->negotiateActiveHostname();
     $this->setRequestDomain($httpHost);
-    return $this->domain;
   }

This return is a convenience for the caller.

Please review.

I actually had to do the same in domain_access_node_create_access after (some) editors started complaining.

Could we have this rebased on the 2.0.x-dev branch please?

I rebased against 2.0.x and I fixed another case I encountered when the domain could not be negotiated.

Thanks a lot! Looks like the MR needs to be rebase and some CI warnings and errors fixed.

Still thinking that we are fixing the symptoms here and not the root causes of the problem...

Thinking out loud:

We are probably trying to implement a fix to avoid looping/reentrancy.

But doing so, once a piece of code has started negotiating the active domain, until negotiation is finished, all subsequent calls creating loops, will receive a NULL active domain. Looks like there is no way to avoid that, so all domain related modules should handle that properly.

Looks like we should probably replace negotiated by negotiating to fix that reentrancy/looop problem.

And add another boolean to simply indicate that no domain is available at all and that we should stop trying to negotiate one. Don't see the case when a domain would suddenly become available later on in the request (except when we are creating domains, as is done in some tests).

To be continued...

Pushed a few fixes to MR 146. Could anyone review it, please? Thanks.

MR !146 seems to be the same as MR !147 now, which means that there aren't any changes since you last asked me to test.

Let's wait a few days before merging.