$database declaration in settings.php must use expanded format like D7

[izmeez]

After installing the module the error

Domain access failed to load during phase: bootstrap include. Please check your settings.php file and site configuration

appears on every page load.

@rudy880719 and @sudipto68 have identified that the problem is the syntax used for defining the $database. See rudy880719/domain#4 It fails if the sort syntax is used: $database = 'mysql://user:pass@localhost/database_name';

Using the extended syntax like D7 works:

$databases['default']['default'] = array(
  'driver' => 'mysql',
  'database' => 'database_name',
  'username' => 'user',
  'password' => 'pass',
  'host' => 'localhost',
  'port' => '',
  'charset' => 'utf8mb4',
  'collation' => 'utf8mb4_unicode_ci',
);

It is odd that one method of declaring the database works and the other does not.

[yorkshire-pudding]

@izmeez - it might be worth referencing this bug in the quickstart wiki until we can fix it.

[izmeez]

This issue may be related to the last commit in the 7.x-3.x branch, https://github.com/backdrop-contrib/domain/commit/ef94c4abd0373e74f931457dad0324ee89a3e448 before the 1.x-3.x branch begins.

[yorkshire-pudding]

This issue may be related to the last commit in the 7.x-3.x branch, ef94c4a before the 1.x-3.x branch begins.

Interesting in that that commit took out domain_get_primary_table() and all references to it, but the port put that function back somehow but there are no references to it anywhere. However, I have an idea. Assigning to self.

[izmeez]

On the old branch this may have been fixed in this closed issue with this PR.

[yorkshire-pudding]

I did try that (as that is what I remembered as fixing that message) and it got rid of the error, but didn't allow the module to work. Unassigning self for now as I can't figure this out.

[yorkshire-pudding]

This issue is a blocker to the module being released. @rudy880719 you may wish to tackle this if you wish to claim the bounty

[rudy880719]

@yorkshire-pudding @izmeez PR created to solve this issue, I don't know if it's the best solution or if it applies good practices code. https://github.com/backdrop-contrib/domain/pull/35

[izmeez]

@rudy880719 On the PR #35 in the first comment, add Fixes https://github.com/backdrop-contrib/domain/issues/25 it will then show at the top of this issue. Thanks.

[izmeez]

@rudy880719 Sorry I may not have been clear enough. You will have to go to the PR and edit your very first opening comment that has nothing in it and add the line there as the first line. Thanks.

[izmeez]

I have just done some initial testing of the PR (copying the diff into a patch file and applying locally) and it appears to work with both the short $database syntax and the extended D7 syntax. Looking at the code shows the domain settings.inc is doing the conversion of the short syntax to the extended syntax. Is this the best approach, I'm not sure but it seems to work.

I am also seeing an issue that may be totally unrelated that I should put in a new issue. When on a subdomain go to structure > domains > and edit of one of the domains. I am redirected to the primary domain and only when I go back with the browser back button to the subdomain and flush caches I see a message You have been redirected: This page must be accessed from the primary domain. This message seems out of place. It probably should appear on the primary domain when user is redirected.

[izmeez]

The reason I am questioning the approach is because I am not sure how it will be impacted by the changes being contemplated in https://github.com/backdrop/backdrop-issues/issues/2231

[yorkshire-pudding]

@izmeez - yes this will need to take https://github.com/backdrop/backdrop-issues/issues/2231 into account but there would have been no way for @rudy880719 to know that. I tackled updating that PR first before coming here as then could give up to date advice.

@rudy880719 I have tested this PR and it works great. Thanks. It will need adapting to cover the new approach I have been working on in core, which is to allow Backdrop to use a simplified array of the form:

$database = array(
  'database' => 'database_name',
  'username' => 'user',
  'password' => 'pass',
  'host' => 'localhost',
);

See the PR attached to https://github.com/backdrop/backdrop-issues/issues/2231

I think the overall approach here is right. The reason this wasn't needed in D7 is that D7 had no simplified version of database settings; everyone used the long form. I think because of the way domain works to intercept bootstrap, repeating it in this function is necessary and will ensure it works. The new approach can be a drop-in replacement, which I will suggest in the PR code review

[yorkshire-pudding]

@rudy880719 On the PR #35 in the first comment, add Fixes https://github.com/backdrop-contrib/domain/issues/25 it will then show at the top of this issue. Thanks.

@izmeez - the full URL is only needed on backdrop core as the issues and PRs are in different repos. In contrib, you can just use: Fixes #25

@rudy880719 Yes, it should be the first line in the PR description. Adding to a comment won't have the same effect.