Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EmbedsContentStreamAndNodeAggregateId interface for ESCR Events #5152

Open
mhsdesign opened this issue Jun 19, 2024 · 0 comments
Open

Fix EmbedsContentStreamAndNodeAggregateId interface for ESCR Events #5152

mhsdesign opened this issue Jun 19, 2024 · 0 comments
Labels

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Jun 19, 2024

With adding the workspace name to the events #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:


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 #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:


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 #5153

/**
 * @property WorkspaceName $workspaceName
 * @property ContentStreamId $contentStreamId
 * @property NodeAggregateId $nodeAggregateId
 */
interface EmbedsWorkspaceNameAndNodeAggregateId
{
}
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue Jun 19, 2024
The property was previously named `$sourceNodeAggregateId` which is a total anomaly to all other events.

To simplify code working with a range of events by interface (`EmbedsContentStreamAndNodeAggregateId`)  we should align the name property. See also:
neos#5152
@mhsdesign mhsdesign changed the title What to do with the partly obsolete EmbedsContentStreamAndNodeAggregateId interface? Fix EmbedsContentStreamAndNodeAggregateId interface for ESCR Events Jun 19, 2024
@mhsdesign mhsdesign added the 9.0 label Jun 23, 2024
neos-bot pushed a commit to neos/contentgraph-doctrinedbaladapter that referenced this issue Jul 8, 2024
The property was previously named `$sourceNodeAggregateId` which is a total anomaly to all other events.

To simplify code working with a range of events by interface (`EmbedsContentStreamAndNodeAggregateId`)  we should align the name property. See also:
neos/neos-development-collection#5152
neos-bot pushed a commit to neos/contentgraph-postgresqladapter that referenced this issue Jul 8, 2024
The property was previously named `$sourceNodeAggregateId` which is a total anomaly to all other events.

To simplify code working with a range of events by interface (`EmbedsContentStreamAndNodeAggregateId`)  we should align the name property. See also:
neos/neos-development-collection#5152
neos-bot pushed a commit to neos/contentrepository-core that referenced this issue Jul 8, 2024
The property was previously named `$sourceNodeAggregateId` which is a total anomaly to all other events.

To simplify code working with a range of events by interface (`EmbedsContentStreamAndNodeAggregateId`)  we should align the name property. See also:
neos/neos-development-collection#5152
neos-bot pushed a commit to neos/neos that referenced this issue Jul 8, 2024
The property was previously named `$sourceNodeAggregateId` which is a total anomaly to all other events.

To simplify code working with a range of events by interface (`EmbedsContentStreamAndNodeAggregateId`)  we should align the name property. See also:
neos/neos-development-collection#5152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant