Problem/Motivation

If you activate the Domain path module on a site with some existing content, accessing this content will produce a 404 error as the fallback to the the core path alias doesn't work as expected when a domain path alias does not exist yet.

Steps to reproduce

  1. Get a fresh drupal site and or use drush si standard
  2. Enable the following module: pathauto
  3. Create a pathauto pattern for the article content type on the site
  4. Create 2 example articles (at this point you can access them both)
  5. Enable the following modules: domain, domain_path
  6. Create a domain record
  7. The 2 example nodes are now inaccessible and produce a 404 error

Note: If the cache is cleared then the 404 error goes away BUT if a new content item is the created the error returns for the 2 original nodes without a domain configured, any nodes created after domain_access is enabled will not have this error

Proposed resolution

Fix the the getDomainPathByAlias method of the DomainPathAliasManager class to properly handle fallback to core AliasManager service.

Module versions

drupal core: 10.3.x
domain: 3.x
domain_path: 2alpha3

Issue fork domain_path-3566322

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:

About issue forks

Comments

lincoln-batsirayi created an issue. See original summary.

mably’s picture

mably commented

Have you rebuilt the node permissions after enabling domain_access module?

lincoln-batsirayi’s picture

lincoln-batsirayi commented

Yes i rebuilt the node permissions but the error is still there

lincoln-batsirayi’s picture

lincoln-batsirayi commented

Additional note, the only way to get rid of this error completely is to edit one of those node, assign a domain AND set a domain specific url alias but obviously this might not be feasible on a site with 1000s of nodes. There's also an additional error here of needing to set a domain specific url alias otherwise the page errors out but I’ll raise this separately

mably’s picture

mably commented

And it works fine if you do not enable the domain_path module?

lincoln-batsirayi’s picture

lincoln-batsirayi commented

Yes correct, if domain_path is never enabled or disabled after the fact then it works fine as expected

mably’s picture

mably commented

Can you verify that the problem is still present when using MR 61 here:

https://www.drupal.org/project/domain_path/issues/3564844

Without redirect and decoupled_router I cannot reproduce the problem.

mably’s picture

mably commented

As far as I understand the domain_access_install() hook code, it looks like existing nodes are not automatically made accessible on domains:

/**
 * Implements hook_install().
 *
 * Installs the default domain field on nodes. We don't do this via schema.yml
 * files because we have an unknown number of node types.
 */
function domain_access_install() {
  if (\Drupal::isConfigSyncing()) {
    // Configuration is assumed to already be checked by the config importer
    // validation events.
    return;
  }
  $list = [];

  // Assign domain access to bundles.
  $list['user'] = 'user';
  $node_types = \Drupal::entityTypeManager()->getStorage('node_type')->loadMultiple();
  foreach ($node_types as $type => $info) {
    $list[$type] = 'node';
  }
  // Install our fields.
  foreach ($list as $bundle => $entity_type) {
    domain_access_confirm_fields($entity_type, $bundle);
  }
  // Install our actions.
  $domains = \Drupal::entityTypeManager()->getStorage('domain')->loadMultiple();
  foreach ($domains as $domain) {
    domain_access_domain_insert($domain);
  }

  // Use a higher weight than content_translation (10).
  // @see https://www.drupal.org/project/domain/issues/3367186
  module_set_weight('domain_access', 20);
}

I do not see, for now, how the problem can relate to the domain_path module.

The domain_path module doesn't modify node access rights AFAIK.

EDIT: When a user rebuilds the permissions, if no domain access is defined for entities, it adds the domain the user rebuilding permissions is currently connected to by default.

lincoln-batsirayi’s picture

lincoln-batsirayi commented

So I’ve done a lot of testing with MR 61 applied and this bug still occurs under certain situations, if i follow the steps I’ve allowed on this issue then the bug doesn’t occur but if i change the order of things slightly and also remove the redirect and decoupled_router modules then the bug still happens, to reproduce it as I’ve found you'd need to do the following

  1. Get a fresh drupal site and or use drush si standard
  2. Enable the following modules: pathauto, admin_toolbar, admin_toolbar_tools (note there's no domain or redirect modules, this simulates a more real world use case of adding the domain modules to an existing site)
  3. Create a pathauto pattern for the content types on the site
  4. Create 2 example nodes (at this point you can access them both)
  5. Enable the following domain modules: domain, domain_path, domain_path_pathauto, domain_source, domain_access
  6. Rebuild content permissions
  7. Create 2 domain records
  8. (nodes are still accessible at this point)
  9. Create 1 or more example nodes (which are accessible)
  10. Now try and visit the 2 original initial nodes which are now inaccessible and produce a 404 error

The only way to fix the error is edit those first 2 nodes and assign a domain to them. I’ve ran through these steps multiple times and been able to reproduce the error each time

mably’s picture

mably commented

You mean that creating a third node make the 2 original ones unavailable? That's pretty strange.

All the path aliases are generated automatically by pathauto?

All your procedure is done on the default domain?

Point 6 should be done after point 7 to work properly (a domain record is needed).
Can you retry switching these two steps?

Code excerpt from the domain_access_node_access_records() hook of the domain_access.module file:

    // If there are no domains set, use the current one.
    $domains = DomainAccessManager::getAccessValues($translation);
    if ($domains === [] && $active_domain instanceof DomainInterface) {
      $domains[$active_domain->id()] = $active_domain->getDomainId();
    }

We can see that an active domain is required to create a node access record.

lincoln-batsirayi’s picture

lincoln-batsirayi commented

Yes that's correct and yes it's on the default domain, weirdly enough after they become unavailable from a node being created, clearing the cache brings them back UNTIL a new node is created and then they go away again... The only way for them to persistently stay that I’ve seen is to edit them and set up a domain on them

mably’s picture

mably commented

Can you try rebuilding the permissions after creating the domain records?

lincoln-batsirayi’s picture

lincoln-batsirayi commented

Unfortunately rebuilding the permissions after creating the records and again after creating new nodes doesn’t solve the error, the original 2 nodes are still inaccessible

mably’s picture

mably commented

Are all the node domain aliases present on the aliases page?

/admin/config/search/path

mably’s picture

mably commented

Could you try to reproduce it on the MR 61 Tugboat?

lincoln-batsirayi’s picture

lincoln-batsirayi commented

So on that path page node 1 and 2 both have aliases there, but obviously no domains attached to them (the page is accessible using the direct node path like /node/1 just not through the aliases)

I’ll take a look on the MR 61 tugboat and let you know

mably’s picture

mably commented

Ah, you mean the original aliases not associated to any domain are not picked up when accessing the nodes once domain_path is activated?

Not sure we are handling that case.

lincoln-batsirayi’s picture

lincoln-batsirayi commented

Yes that's correct, i guess if that case isn’t being handled I don’t know that a 404 is the right way of dealing with it, I’d at least maybe expect a redirect to the regular node path like /node/{id} at the very minimum don't you think?

mably opened merge request !65

mably’s picture

mably commented

@lincoln-batsirayi could you give a try to this issue's MR 65 please?

It should fix your "original alias not taken into account" problem. 🤞

mably’s picture

mably commented
Title: Page not found errors on pages without a configured domain » Broken fallback to core aliases in DomainPathAliasManager causes 404 errors
mably’s picture

mably commented
Issue summary: View changes
mably’s picture

mably commented
Issue summary: View changes
mably’s picture

mably commented
Issue summary: View changes
mably’s picture

mably commented
Version: 2.0.0-alpha3 » 2.0.0-alpha4
Status: Active » Needs review
mably’s picture

mably commented
Issue summary: View changes
mably’s picture

mably commented

Alias fallback behavior is now covered by a dedicated functional test.

lincoln-batsirayi’s picture

lincoln-batsirayi commented

@mably so your new MR didn’t worked when tested with both MR 61 and MR 65 applied, i followed the steps from comment #9 after step 5 (enabling the domain modules) the 2 nodes now get the 404 error, rebuilding the permissions doesn’t fix the issue. Only clearing the cache or adding a domain to those original nodes fixes the issue.

But when i tested MR 65 on it's own and following those steps the issue no longer occurs, the nodes are still accessible which is great! I even also tested it with the redirect and decoupled_router modules enabled and it also worked in the scenario. So I'm happy to change this reviewed and tested but we should be cautious of this bug coming back once MR61 is merged into the dev branch, what do you think?

mably’s picture

mably commented

MR 61 is currently incompatible with MR 65 so the behavior you are observing is perfectly normal.

I'll merge this issue's MR and rebase MR 61 right after.

lincoln-batsirayi’s picture

lincoln-batsirayi commented
Status: Needs review » Reviewed & tested by the community

Sounds good, this is now officially reviewed then.

  • mably committed 24b5d284 on 2.x 
    fix: #3566322 Broken fallback to core aliases in DomainPathAliasManager...
mably’s picture

mably commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

mably’s picture

mably commented

MR 61 has been rebased. Could you give it a try @lincoln-batsirayi?

lincoln-batsirayi’s picture

lincoln-batsirayi commented

I’ve tested the rebased MR61 and that's also worked, so in terms of this specific bug MR 61 is fine.

Status: Fixed » Closed (fixed)

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