Discussion improve site entity and site node relation

[mhsdesign]

Having the concept of a site entity (doctrine) and a site node (cr) is not ideal:

• as there are two different stores (doctrine and cr) there is no way to enforce the relation and one can easily end up in states where there is a site but no content or the other way around.
• resolving site node to site or site to site node is complicated and requires deeper understanding of the coupling.
  • for example getting a site node by current request is needlessly hard as it requires to interpret the site detection result (which only informs about the site entity) correctly to fetch the site node

We discussed already several ways of improving their relationship:

• use a different technique - the node id - to link both

  neos-development-collection/Neos.Neos/Classes/Domain/Service/SiteService.php

  Line 169 in a9d381e

  // @todo use node aggregate identifier instead of node name

  but this does not really improve the situation by much
• use more distinct naming and dont call them both "site". For example the site in fusion is a site node, while in php we often mean by $site the site entity (which is mostly unused in fusion) -> the same discussion came up with the introduction of !!! FEATURE: Add Neos.Neos:Site (singular) NodeType for site nodes #4531 where we discussed to name it Neos.Neos:Site or Neos.Neos:HomePage. We decided for Site and declared the site entity rather legacy and some time to be replaced.
• completely fusion both the site and site node together: This is only possible with the introduction of the dedicated site nodetype !!! FEATURE: Add Neos.Neos:Site (singular) NodeType for site nodes #4531. The idea would use aggregate scoped properties for all the fields a site entity had and completely remove the site entity. In places where we need a site but have no specific information about the dimension we would pass the site node aggregate. The experiment can be found here: Experiment: Replace SiteEntity with SiteNodeAggregate #4780
• a simple alternative would be to use a immutable site read model which is backed by the configuration from Neos.Neos.sites. See Discussion improve site entity and site node relation #4470 (comment)

[mhsdesign]

As discussed the real goal would be to remove our neos site entity at one point and save the domains in the Neos.Neos:Site node. This would be great and would save us from much trouble. But this is now a little to late to start for Neos 9, maybe Neos 10?

I updated the pr description and we will evaluate how much effort this may take and if its realistic to fix with 9.0

[mhsdesign]

Early thoughts about implementation:

the scoped properties (maybe via #4779) could be used for the domains and stuff:

'Neos.Neos:Site':
  # ..
  properties:
    domains:
      scope: nodeAggregate
      type: array<Neos\\Neos\\Domain\\Model\\Domain>
    primaryDomain:
      scope: nodeAggregate
      type: Neos\\Neos\\Domain\\Model\\Domain
    siteResourcesPackageKey:
      scope: nodeAggregate
      type: string
    assetCollection:
      scope: nodeAggregate
      type: Neos\\Media\\Domain\\Model\\AssetCollection

The site config (which might be reworked via #4582) would still depend on the node name (which must be unique by convention across multiple crs)

[kitsunet]

How do you find the node with the right domain though if you don't know where to look during routing?

[bwaidelich]

While discussing this topic in Slack we realized that the Node can't be the central authority for sites because there is no central Content Repository.

Instead I would suggest the following:

• Make the SiteRepository an interface without db specific types
  • With that, inline Site::getConfiguration()
• Provide a Settings.yaml- implementation
• Potentially provide a DB-implementation as replacement
• Introduce new Site model, remove all setters
  • To consider: How to change the site state programatically? – maybe a separate, more specific Repository interface with mutation methods?

[mhsdesign]

immutable site read model

As discussed with @kitsunet the site state (offline / online) might be dropped as this is not an important feature.
The assetCollection might be more of a thing to worry but i think we came to the conclusion that one is allowed to configure it via settings and that the concept is not super duper perfect anyway and not fully working?

I played a bit around and the php code of the repository could look like this:
(i wasnt totally sure about the nullable or non nullable return types but that might work ^^ - only findSiteBySiteNode is truly non nullable because with a node at hand one truly expects a site!)

interface SiteRepository
{
    /**
     * Find the site that was specified in the configuration `Neos.Neos.defaultSiteNodeName`
     *
     * If the configuration is not set it must fall back to the first available site or return null
     *
     * @throws \Neos\Neos\Domain\Exception if the configuration is invalid and the default site is not found
     */
    public function findDefault(): ?Site;

    public function findByDomain(Domain $domain): ?Site;

    public function findByHost(UriInterface $uriWithHost): ?Site;

    public function findAll(): iterable;

    public function findOneByNodeName(SiteNodeName $siteNodeName): ?Site;

    /**
     * Finds a given site by site node.
     * 
     * @throws \Neos\Neos\Domain\Exception in case the passed $siteNode is not a real site node or no site matches this site node
     */
    public function findSiteBySiteNode(Node $siteNode): Site;
}

The Site model will be stripped down and become and immutable vo. As the site will be build from the configuration it will already hold all information of SiteConfiguration and we will get rid of this dto.

final readonly class Site
{
    public function __construct(
        // from SiteConfiguration
        public ContentRepositoryId $contentRepositoryId,
        public RoutingConfiguration $routingConfiguration,
        // original properties:
        /** Name of the site */
        public string $name, // todo name is actually bit confusing and doesnt declare that this might be a human readable label. So label or title would fit better.
        /** The node name of this site */
        public SiteNodeName $nodeName,
        /** The key of a package containing the static resources for this site */
        public string $siteResourcesPackageKey, // todo maybe just name it $packageKey?
        public ?Domains $domains,
        // todo how ???
        // public AssetCollection $assetCollection,
    ) {
    }
}

final readonly class Domains implements \IteratorAggregate
{
    private function __construct(
        public Domain $primaryDomain,
        private array $rest,
    ) {
        // deduplication logic
    }

    public static function create(Domain $primaryDomain, Domain ...$rest)
    {
        return new self($primaryDomain, $rest);
    }

    public function getIterator(): Traversable
    {
        yield $this->primaryDomain;
        yield from $this->rest;
    }
}

final readonly class Domain
{
    public function __construct(
        public string $hostname,
        public ?string $scheme,
        public ?int $port
    ) {
    }
}

final readonly class RoutingConfiguration
{
    public function __construct(
        public string $contentDimensionResolverFactoryClassName,
        public array $contentDimensionResolverOptions,
        public DimensionSpacePoint $defaultDimensionSpacePoint,
        public string $uriPathSuffix,
    ) {
    }
}

migrating things:

all setters will be removed so no mutation via the repository nor the value objects is indented. This has to be manually implemented behind the scenes ;)

// the state will be removed -> if someone needs it this he can implement by returning from the SiteRepository only the online ones. By that we reduce complexity a lot.
getState()
isOnline()
isOffline()
STATE_ONLINE = 1;
STATE_OFFLINE = 2;

// this internal signal will be removed
emitSiteChanged();

// simple properties will persist
getName() -> $site->name
getNodeName() -> $site->nodeName
getSiteResourcesPackageKey() -> $site->siteResourcesPackageKey
getAssetCollection() -> $site->assetCollection

// 9.0 specific configuration stuff
getConfiguration()->contentRepositoryId -> $site->contentRepositoryId
getConfiguration()->contentDimensionResolverFactoryClassName -> $site->routingConfiguration->contentDimensionResolverFactoryClassName
getConfiguration()->contentDimensionResolverOptions -> $site->routingConfiguration->contentDimensionResolverOptions
// contentDimensions.defaultDimensionSpacePoint must be actually required to be set otherwise there is trouble as [] is not always valid
getConfiguration()->defaultDimensionSpacePoint -> $site->routingConfiguration->defaultDimensionSpacePoint
getConfiguration()->uriPathSuffix -> $site->routingConfiguration->uriPathSuffix

// domain related
getDomains() -> $site->domains
hasActiveDomains() -> $site->domains !== null
getActiveDomains() -> iterator_to_array($site->domains)
getFirstActiveDomain() -> $site->domains->primaryDomain
getPrimaryDomain() -> $site->domains->primaryDomain

[mhsdesign]

The above read model would be backed primarily out of the box by a SiteConfigurationSource which implements the SiteRepository.

This requires us to adjust how sites are registered in Neos.Neos.sites.
For example as the configuration is currently only used as enrichment but not as primary source we introduced the wildcard *. But this when using the settings as site source we are required to explicitly state the nodeName and not use the wildcard * configuration.

Naturally this would mean having to duplicate all the "basic" options every time. But with the Site Config Presets this will not be necessary #4582

Currently in 9.0 the configuration might look like:

Neos:
  Neos:
    sites:
      '*':
        uriPathSuffix: '.html'
        contentRepository: default
        contentDimensions:
          resolver:
            factoryClassName: Neos\Neos\FrontendRouting\DimensionResolution\Resolver\AutoUriPathResolverFactory

But with the Site Config Presets and the SiteConfigurationSource we would write:

Neos:
  Neos:
    sitePresets:
      'default':
        uriPathSuffix: '.html'
        contentRepository: default
        contentDimensions:
          resolver:
            factoryClassName: Neos\Neos\FrontendRouting\DimensionResolution\Resolver\AutoUriPathResolverFactory
    sites:
      'my-site-node-name':
        preset: 'default'
        name: "My Site Titel"
        siteResourcesPackageKey: "Neos.Demo"
        domains:
          primaryDomain:
            hostname: "localhost"
            scheme: "http"
            port: 8080
          additionalDomains:
            -
              hostname: "www.lol.de"
        assetCollection: null # todo how to actually reference an assetCollection??? via Persistence_Object_Identifier???

---

Discussions

Alternative Neos.Neos.sites configuration with site node aggregate ids

alternatively we could already open the door and allow nodeAggregateId's to linking to the node. This is much faster and less quirky ^^. So possibly one would register a node id or enter an absolute nodepath. The downsides of this is that the fetched node has first to be checked if it is of type Neos.Neos:Site and cannot be blindly trusted. And also the configuration name "some-random-name..." will have no meaning and thus not forbid configuring two sites with the same node... though that might need to be supported with multiple content repositories???. A way out would be to insert the node id or absolute node path into the configuration name.

Neos:
  Neos:
    sites:
      some-random-name-only-for-configuration-readability:
        preset: 'default'
        node: my-site-node-aggregate-id
        # or "legacy" node names via:
        # node: <Neos.Neos:Sites>/my-site-node-name

Site creation?

As sites will be defined in the settings, they will have to be created like a content repository needs to be setup.
So instead of

flow site:create neosdemo Neos.Demo Neos.Demo:Document.Homepage

it might looks like:

flow site:setup neosdemo Neos.Demo:Document.Homepage

Command controllers

All site and domain command controllers will only allow reading from the new site model. No creation will be possible.

[mhsdesign]

Last Friday we discussed the above idea in the dev meeting:

• Configure sites & domains via settings.yaml and remove all mutability from the site model
  • -> no more site:create but its all in the settings
• Optionally: Move the site backend module and doctrine site entity could to a separate optional installable package to enable the old functionality.

Following points were made:

Pro (Bastian & Marc Henry):

• one single source where the sites reside, instead of a site entity + site configuration mix
• less "moving" parts and more easier to maintain and more stable in general (the current site <-> site node relation is messy and can lead to invalid states)
• the cr:import module would need no additional neos extension, as the site is provided via settings
• for development and production it seems more straight forward to have the sites / domains configured via settings and not in the db

Con (Martin & Denny):

• this will remove an easy to use feature for site owners (and possibly even editors)
  • -> on first sight we removed the neos multi site functionality
  • some neos installations need the feature to dynamically create sites on demand
• we will most likely still need a high level site:import command for neos metadata, so no need to avoid it for the site
• we gain nothing when providing an optional package which might be unstable
• we should strive to make the site <-> site node relation more stable
• we dont have the time for such a fundamental more optional change right now

Jain - in between - (Christian)

• if we were to do this, setting domains via environment variables must be thought of and yaml + env might not be as dynamic as it has to be for variating number of domains on stage vs live

Conclusion

As this was discussed super controversially without a clear voting in favour we decided against this.

[mhsdesign]

@mficzel and me are currently discussing how to reduce the ambiguity in fusion as having the siteNODE available as site is not ideal. And doesnt fit together with node and documentNode

The change to add site was done ages ago where the documentNode was not available in the context and so the naming did fit at that point: 85fa242

Deprecating site now in favour of siteNode might be a good step for Neos 9

---

Additionally we discussed that when we want to allow creating sites on the fly as they are a entities its unhelpful that the contentRepository is only configurable by setting and not on the creation in the modal.
Keeping the link to the contentRepository also in the setting has the disadvantage that this is not persistent and any change here can too easy create mismatches which i tried to detect here: #4900.

Also the SiteConfiguration configuration on the site is a huge problem for testing and shows the immense complexity:

neos-development-collection/Neos.Neos/Tests/Behavior/Features/Bootstrap/RoutingTrait.php

Line 117 in c34456b

public function theSiteConfigurationIs(\Behat\Gherkin\Node\PyStringNode $configYaml): void

A way out could be to leverage the new sitePresets differently than anticipated:

• The link from SiteEntity to preset is done via SiteEntity::getPresetName() which would be part of the entity's persisted state.
• The siteResourcesPackageKey will be moved to the configuration
• The SiteEntity knows its site node by aggregate id (which would allow a full nullable site node system!!!)
• The SiteEntity knows its content repository id:

Leaving us with the following structure

new Site(
    contentRepositoryId: ...,
    nodeAggregateId: ...,
    presetName: ...,
    name: ..., // rather a label?
    domains: ...,
    primaryDomain: ...,
    state: ...,
    assetCollection: ...,
)

Neos:
  Neos:
    sitePresets:
      'my-preset':
        resourcesPackageKey: 'Neos.Demo'
        uriPathSuffix: '.html'
        contentDimensions:
          resolver:
            factoryClassName: Neos\Neos\FrontendRouting\DimensionResolution\Resolver\UriPathResolverFactory
            options:
              segments:
                -
                  dimensionIdentifier: language
                  dimensionValueMapping:
                    # Dimension Value -> URI Path Segment
                    de: deu
                    en: uk

class SiteService
{
    public function getPresetForSite(Site $site): SitePreset;
    
    // ... further fetching and creation api
}

cc @kitsunet