neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 218 forks source link

Fix `EmbedsContentStreamAndNodeAggregateId` interface for ESCR Events #5152

Open mhsdesign opened 1 week ago

mhsdesign commented 1 week ago

With adding the workspace name to the events https://github.com/neos/neos-development-collection/issues/4996 we are now more and more relying on $event->workspaceName making this interface partly obsolete.

bastian said there:

[...] all events implementing EmbedsContentStreamAndNodeAggregateId are affected, so maybe it makes sense to extend (and rename) that interface

For example here we would like to use the workspace name:

https://github.com/neos/neos-development-collection/blob/8796218cbcccbab34faf9032de3cd993fa1bd944/Neos.Neos/Classes/Fusion/Cache/GraphProjectorCatchUpHookForCacheFlushing.php#L184


The EmbedsContentStreamAndNodeAggregateId interface is implemented 16 times.

Where its methods are nearly always implemented like:

public function getContentStreamId(): ContentStreamId
{
    return $this->contentStreamId;
}

public function getNodeAggregateId(): NodeAggregateId
{
    return $this->nodeAggregateId;
}

With one exception, the NodeReferencesWereSet doesnt have a nodeAggregateId property but its named sourceNodeAggregateId (unfortunately) -> to be fixed via https://github.com/neos/neos-development-collection/pull/5153


But then again a bit of classification among the events would be great so we could easily allow them in canHandle when writing projections like we do already: https://github.com/neos/neos-development-collection/blob/f00bce78fe55ded4507c0a537109d29e57edf524/Neos.ContentRepository.Core/Classes/Projection/ContentStream/ContentStreamProjection.php#L167 See slack discussion: https://neos-project.slack.com/archives/C04PYL8H3/p1711392719268439?thread_ts=1711353783.863809&cid=C04PYL8H3

So i would propose to add the workspace to the interface and rename it to something like EmbedsWorkspaceNameAndNodeAggregateId. Ideally we would also not have to introduce a new getter everywhere (getWorkspaceName) as this is a lot of boiler plate, and b opens the question wether to use $event->workspaceName directly or the getter.

With Php8.4 property hooks https://wiki.php.net/rfc/property-hooks we could write

interface EmbedsWorkspaceNameAndNodeAggregateId
{
    public WorkspaceName $workspaceName { get; }
    public ContentStreamId $contentStreamId { get; }
    public NodeAggregateId $nodeAggregateId { get; }
}

maybe we can use this already in a similar fashion leveraging phps @property and phpstan? But in that case we will stumble over that NodeReferencesWereSet as a anomaly and doesnt call it $nodeAggregateId -> to be fixed via https://github.com/neos/neos-development-collection/pull/5153

/**
 * @property WorkspaceName $workspaceName
 * @property ContentStreamId $contentStreamId
 * @property NodeAggregateId $nodeAggregateId
 */
interface EmbedsWorkspaceNameAndNodeAggregateId
{
}