Problem/Motivation

Active domain returns null in some cases in domain/domain_access/domain_access.module:419 .

Steps to reproduce

Not completely clear.

Proposed resolution

Add hook to retrieve domain from domain Negotiator with parameter TRUE to circumvent.

Remaining tasks

Completely Reproduce.

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

None

Issue fork domain-3488024

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

medusa43 created an issue. See original summary.

medusa43’s picture

medusa43’s picture

redzeuf’s picture

StatusFileSize
new921 bytes

I have exactly the same error message when trying to access a add content page in the admin with a user having specific role (for admin it works well).

Here is a patch. It looks like the is_null logic is_numeric are the opposite that needed.
I also take care of the non object that is mentioned in issue https://www.drupal.org/project/domain/issues/2936697

redzeuf’s picture

StatusFileSize
new844 bytes

oups here is the applicable patch for pervious comment.

andreasderijcke made their first commit to this issue’s fork.

andreasderijcke’s picture

Version: 2.0.0-beta2 » 2.0.x-dev
Status: Active » Needs review

Different patch as MR, because I think the base check can be simplified.

In addition, the is_numeric check should not be necessary according to the return type of getDomainId(), so either the check is obsolete, or the function doc is incomplete.
As I don't have the correct answer, have flagged it as a todo.

dieterholvoet’s picture

Status: Needs review » Reviewed & tested by the community

The MR fixes the issue for me.

danrod’s picture

danrod’s picture

As mentioned in the file domain_access/domain_acccess.module lines 416-421, this needs to be reviewed:

// @todo According to \Drupal\domain\DomainInterface::getDomainId, only
  // integers are returned. Either this check is obsolete, or the function
  // documentation is incomplete.
  if (!is_numeric($active_domain->getDomainId())) {
    return AccessResult::neutral();
  }

As far as I know this method always returns an integer, not NULL or 0, but I think we can merge this MR for now.

danrod’s picture

danrod’s picture

mably’s picture

Not sure to understand what the original problem is exactly?

Could someone mind explaining?

danrod’s picture

Status: Reviewed & tested by the community » Fixed

Merged to the 2.0.x branch, thanks everyone for the input, if anyone is still having any problems with this, I can re-open the issue.

danrod’s picture

mably’s picture

@danrod could you explain what the original problem was exactly?

Not sure to understand why the is_null test was not working.

danrod’s picture

Status: Fixed » Reviewed & tested by the community

@mably some users were reporting that the variable $active_domain being NULL in the file domain_access/domain_access.module line 419:

$id = $active_domain->getDomainId();

Not sure why exactly, and I tried to reproduce it and I couldn't, so a different condition was proposed to check if $active_domain is a valid object (of type DomainInterface)and not empty: https://git.drupalcode.org/project/domain/-/merge_requests/134/diffs

Also, an extra condition was added:

/ @todo According to \Drupal\domain\DomainInterface::getDomainId, only
  // integers are returned. Either this check is obsolete, or the function
  // documentation is incomplete.
  if (!is_numeric($active_domain->getDomainId())) {
    return AccessResult::neutral();
  }

In theory, $active_domain->getDomainId() should always return an integer.

mably’s picture

Don't see how $active_domaincould be NULL on line 419, probably a different codebase...

Anyway, the change seems ok, but it will probably not fix the original problem.

danrod’s picture

Alright, thanks @mably , I'll let you know in advance when I try to merge an issue to the 2.0.x branch, the change seems to have fixed the issue for some users, let's see.

danrod’s picture

Status: Reviewed & tested by the community » Fixed

@mable I'll move it to "Fixed" for now (since it's already committed to the 2.0.x branch), we can re-open it if the issue re-appears again.

Status: Fixed » Closed (fixed)

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