Problem/Motivation

When a site has multiple domains with aliases for different environments, changing the sort order of the domains on the /admin/config/domain page while accessing the site via one of the aliases causes the hostname on any affected domains to be overwritten with the alias on save.

Proposed resolution

The domain_alias module contains a hook_ENTITY_TYPE_load function domain_alias_domain_load. This intentionally changes the hostname, path and URL when loading a domain. As this hook is fired on loading each domain entity, when the weights are changed on the form, the modified hostname is saved overwriting the desired hostname.

As a workaround, we could add a check to see if we are processing this form and then skip processing the rest of the hook if we are.

Remaining tasks

This may not be the best way of doing this and there may be other circumstances where this behaviour is not desirable so this need review

CommentFileSizeAuthor
#5 domain_alias_hostname_overwitten-2925629-4.patch586 bytesalanhdev
#2 domain_alias_hostname_overwitten-2925629-2.patch691 bytesalanhdev

Comments

Alan-H created an issue. See original summary.

alanhdev’s picture

alanhdev commented
StatusFileSize
new domain_alias_hostname_overwitten-2925629-2.patch691 bytes

Patch file for the workaround suggested is attached.

kiwimind’s picture

kiwimind commented
Status: Active » Needs review

Updating status to run tests (assuming there are some!).

opdavies’s picture

opdavies
He/him
Primary language English
Location Wales
commented 
+++ b/domain/domain_alias/domain_alias.module
@@ -83,6 +83,12 @@ function domain_alias_domain_load($entities) {
+  $current_path = \Drupal::service('path.current')->getPath();
+  if ($current_path == '/admin/config/domain') {

Because this is in a .module file, you can use Drupal:: rather than \Drupal:: as there's no namespace to consider.

Also, I'd personally inline that in the condition rather than setting it to a variable.

alanhdev’s picture

alanhdev commented
StatusFileSize
new domain_alias_hostname_overwitten-2925629-4.patch586 bytes

Following a chat with @kiwimind and @opdavies, I've reworked the patch to match the route 'domain.admin' rather than using the path.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Needs work

Nice catch!

We cannot run tests on the d.o. infrastructure. They need to run in Travis (because of the need for multiple subdomains.)

For tests, we need to file a Pull Request against https://github.com/agentrickard/domain

This is a nasty little bug, and does need a test of its own.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Queued for testing here -- https://github.com/agentrickard/domain/pull/384

zerolab’s picture

zerolab commented
Issue tags: -domain alias, -domain access
zerolab’s picture

zerolab commented

The patch LGTM.
My nitpick is with Drupal:: vs \Drupal::. Core uses the namespaced version (see node.module for example)

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

I found an easier fix while solving #2925798: Saving domain on edit page may result in undesired domain sort order change.

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

Needs review. patch is in the other issue.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Needs review

Tests added here -- https://github.com/agentrickard/domain/pull/385 -- they do fail without the save change to submitForm().

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Needs work

I suspect this will also affect the AJAX admin callbacks (enable / disable / make default) on the overview page. Those need to be tested as well.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Fixed

Fixed, with tests for both weight and hostname behaviors.

Status: Fixed » Closed (fixed)

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