ENH Add domain support for the main site. #473

GuySartorelli · 2021-12-06T23:23:38Z

Fixes #472

This ensures the AbsoluteLink for pages will always be correct for main site pages (if a domain is set up for the main site) - even when calling it from the context of a subsite. This allows easily linking to main site pages from subsites, as well as any other situation where the absolute URL for the main site needs to be fetched correctly.

GuySartorelli · 2021-12-06T23:24:46Z

Note: In managed models, the title was set explicitly for the subsites tab to avoid the main site domains tab becoming the first tab.

michalkleiner

Thanks @GuySartorelli, the approach looks sensible. I've added a few minor inline comments if you could look at those, thanks!

src/Admin/SubsiteAdmin.php

src/Model/Subsite.php

src/Admin/SubsiteAdmin.php

michalkleiner · 2021-12-06T23:52:10Z

Though I always thought the main site has mechanisms to get its primary domain anyway, with or without using the subsites module. Is the issue rather in the bespoke code/additions or modules where it doesn't correctly pick the subsite e.g. from CLI or similar, and configs like alternate_base_url should be used instead?

GuySartorelli · 2021-12-06T23:59:30Z

Thanks @GuySartorelli, the approach looks sensible. I've added a few minor inline comments if you could look at those, thanks!

Sweet! I'll make those changes shortly.

Though I always thought the main site has mechanisms to get its primary domain anyway, with or without using the subsites module. Is the issue rather in the bespoke code/additions or modules where it doesn't correctly pick the subsite e.g. from CLI or similar, and configs like alternate_base_url should be used instead?

So... in some situations the main site has mechanisms to get its primary domain. And sometimes they work. Sometimes they don't. For the times it doesn't work I have added a PR to allow that to be configured dynamically (see silverstripe/silverstripe-framework#10168) but that's a bit out of scope.

alternate_base_url is fine for most situations, but I'm often in a situation where the client hasn't made a final decision on the production domain until after we've already deployed code to the production environment, so I always end up adding in a field to the SiteConfig to store the main site domain anyway. This PR gives a more consistent place to set that domain and means I don't have to add it myself every time I add subsites to a codebase.

This ensures the AbsoluteLink for pages will always be correct for main site pages (if a domain is set up for the main site) - even when calling it from the context of a subsite. This allows easily linking to main site pages from subsites, as well as any other situation where the absolute URL for the main site needs to be fetched correctly.

GuySartorelli · 2021-12-07T00:02:35Z

@michalkleiner I've made the requested changes.

michalkleiner · 2021-12-07T00:10:33Z

src/Model/Subsite.php

@@ -885,6 +885,16 @@ public function domain()
     */
    public function getPrimarySubsiteDomain()
    {
+        // Main site
+        if (!$this->isInDB()) {


Now seeing the method this is in and the not-in-db check... is there a better/different place this could go to? Alternatively, can we get more explanation why it's here and why we need to check it's not in the db?

An alternative would be if (!$this->ID) {.
This is for situations like SiteTreeSubsites::alternateAbsoluteLink (inline below for convenience) where we call domain (which calls getPrimarySubsiteDomain) on the relationship directly from the has_one relation object.
If the page has no SubiteID (i.e. it belongs to the main site), the relation object will still be an instantiated Subsite object which has no ID and which returns false from isInDB - so we end up getting the main site domain.

I can't think of somewhere else to put the call, since the intention is to distinguish between a main site or subsite Subsite object (where a main site Subsite object is any Subsite object without an ID, effectively) when fetching the domain.

This allows project-specific code or code from other modules to remain the same (if they don't have the "if there is a subsite ID" check in place - in that case there's still only a very minor change required) and now get the added benefit of automagically fetching the correct main site domain as well.

silverstripe-subsites/src/Extensions/SiteTreeSubsites.php

Lines 410 to 417 in 0b7a63e

public function alternateAbsoluteLink($action = null)

{

// Generate the existing absolute URL and replace the domain with the subsite domain.

// This helps deal with Link() returning an absolute URL.

$url = Director::absoluteURL($this->owner->Link($action));

$url = preg_replace('/\/\/[^\/]+\//', '//' . $this->owner->Subsite()->domain() . '/', $url);

return $url;

}

Just realised I didn't explicitly say why we need to check for the main site vs an actual subsite there - the main site domains aren't stored on the Domains relation of any subsite object, so we can't just use $this->Domains() for the main site.

emteknetnz

I'm worried this PR has a high chance of causing regressions when people run composer update

What will the experience be like for all existing sites that do not have SubsiteDomain's defined once this code is released?

Is this 100% backwards compatible with "Incomplete data"? Or is there an expectation that everyone will fill create a new SubsiteDomain record (they won't)

I'd need unit tests testing things like AbsoluteLink() on this PR both with and without a SubsiteDomain record on the main site to feel comfortable here

emteknetnz · 2022-02-01T23:27:11Z

src/Extensions/SiteTreeSubsites.php

-        if ($this->owner->SubsiteID) {
-            $url = preg_replace('/\/\/[^\/]+\//', '//' . $this->owner->Subsite()->domain() . '/', $url);
-        }
+        $url = preg_replace('/\/\/[^\/]+\//', '//' . $this->owner->Subsite()->domain() . '/', $url);


Why are we removing this check? If $this->owner->SubsiteID == 0 then this will cause an exception i.e. (null)->domain()

I'm about 60% sure $this->owner->Subsite() would give a new Subsite object with no ID - but that would need to be checked if someone picks this PR up again. If that's not the case, then another mechanism will need to be added here to get the main site Domain where no subsite is set.

I'm 100% sure.
https://github.com/silverstripe/silverstripe-framework/blob/6d90eec0c883a69a87e0ad31d0e8ba617b809057/src/ORM/DataObject.php#L1890

GuySartorelli · 2022-02-01T23:39:14Z

Honestly it's been far too long since I've touched this PR. It needs work and I don't have the time to do it and we've found ways around this in the project I was working on at the time. Closing, but someone else can pick it up if they need something like this in their projects and haven't found their own workaround.

michalkleiner reviewed Dec 6, 2021

View reviewed changes

src/Admin/SubsiteAdmin.php Show resolved Hide resolved

src/Model/Subsite.php Outdated Show resolved Hide resolved

src/Model/Subsite.php Outdated Show resolved Hide resolved

src/Admin/SubsiteAdmin.php Outdated Show resolved Hide resolved

GuySartorelli added 2 commits December 7, 2021 13:01

ENH: Use sanitiseClassName for consistency.

0b7a63e

GuySartorelli force-pushed the enh/main-site-domains branch from 17505d3 to 0b7a63e Compare December 7, 2021 00:02

michalkleiner reviewed Dec 7, 2021

View reviewed changes

emteknetnz requested changes Feb 1, 2022

View reviewed changes

GuySartorelli closed this Feb 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Add domain support for the main site. #473

ENH Add domain support for the main site. #473

GuySartorelli commented Dec 6, 2021

GuySartorelli commented Dec 6, 2021

michalkleiner left a comment

michalkleiner commented Dec 6, 2021

GuySartorelli commented Dec 6, 2021

GuySartorelli commented Dec 7, 2021

michalkleiner Dec 7, 2021

GuySartorelli Dec 7, 2021

GuySartorelli Dec 7, 2021

emteknetnz left a comment •

edited

emteknetnz Feb 1, 2022

GuySartorelli Feb 1, 2022

NightJar Feb 3, 2022 •

edited

GuySartorelli commented Feb 1, 2022 •

edited

	public function alternateAbsoluteLink($action = null)
	{
	// Generate the existing absolute URL and replace the domain with the subsite domain.
	// This helps deal with Link() returning an absolute URL.
	$url = Director::absoluteURL($this->owner->Link($action));
	$url = preg_replace('/\/\/[^\/]+\//', '//' . $this->owner->Subsite()->domain() . '/', $url);
	return $url;
	}

ENH Add domain support for the main site. #473

ENH Add domain support for the main site. #473

Conversation

GuySartorelli commented Dec 6, 2021

GuySartorelli commented Dec 6, 2021

michalkleiner left a comment

Choose a reason for hiding this comment

michalkleiner commented Dec 6, 2021

GuySartorelli commented Dec 6, 2021

GuySartorelli commented Dec 7, 2021

michalkleiner Dec 7, 2021

Choose a reason for hiding this comment

GuySartorelli Dec 7, 2021

Choose a reason for hiding this comment

GuySartorelli Dec 7, 2021

Choose a reason for hiding this comment

emteknetnz left a comment • edited

Choose a reason for hiding this comment

emteknetnz Feb 1, 2022

Choose a reason for hiding this comment

GuySartorelli Feb 1, 2022

Choose a reason for hiding this comment

NightJar Feb 3, 2022 • edited

Choose a reason for hiding this comment

GuySartorelli commented Feb 1, 2022 • edited

emteknetnz left a comment •

edited

NightJar Feb 3, 2022 •

edited

GuySartorelli commented Feb 1, 2022 •

edited