!!! FEATURE: Add `workspaceName` to relevant events #5002

bwaidelich · 2024-04-19T08:56:43Z

Adds the workspaceName to the data of all content stream related events.

Breaking change

This is a breaking change because it changes the event payload.
Exports need to be replaced and events can be migrated with the provided event migration:

Event migration

This PR comes with a event migration that allows for fixing already published events:

./flow migrateevents:migratePayloadToWorkspaceName

Resolves: #4996

Neos.ContentRepository.Core/Classes/Feature/Common/PublishableInterface.php

# Conflicts: # Neos.ContentRepository.Core/Classes/Feature/NodeMove/Event/NodeAggregateWasMoved.php

…gregateId` interface

bwaidelich

I left some comments for my fellow reviewers..

Apart from those I wonder if we need to take care of previous CR exports, e.g. via some on-the-fly-migration during import!?

bwaidelich · 2024-05-17T13:18:28Z

Neos.ContentRepository.Core/Classes/Feature/Common/NodeAggregateEventPublisher.php

@@ -38,13 +38,13 @@ public static function enrichWithCommand(
 foreach ($events as $event) {
 if ($event instanceof DecoratedEvent) {
 $undecoratedEvent = $event->innerEvent;
- if (!$undecoratedEvent instanceof PublishableToOtherContentStreamsInterface) {
+ if (!$undecoratedEvent instanceof PublishableToWorkspaceInterface) {


I renamed the interface PublishableToOtherContentStreamsInterface to PublishableToWorkspaceInterface

oh yes that was missed with bernhards change :D

bwaidelich · 2024-05-17T13:21:04Z

Neos.ContentRepository.Core/Classes/Feature/NodeTypeChange/NodeTypeChange.php

+ $contentGraph->getWorkspaceName(),
+ $contentGraph->getContentStreamId(),


Previously the contentStreamId was taken from the node aggregate – but since that does not contain the workspaceName I changed both to use the info from the ContentGraph.
Not 100% sure about this, but in practice it should not make a difference

actually that seems like the best option as NodeAggregate::contentStreamId will be removed.

bwaidelich · 2024-05-17T13:23:08Z

Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/Features/NodeCreation.php

+ if (!isset($eventPayload['workspaceName'])) {
+ $eventPayload['workspaceName'] = 'some-workspace';
+ }


I used this in all the "The event [...] was published with payload" steps to fill the workspaceName if it wasn't specified (we already do that for dimension space points etc).

An alternative would be to adjust the behat tests themselves.. not sure about it

auto fill seems fine. dont we do this with the csid as well?

We used to fall back to currentContentStreamId and could do the same with currentWorkspaceName. I'm not too fond of autofill here and would prefer an exception if neither is given

oh lol sorry i missed that we literally hardcode 'some-workspace' here. I was under impression we would fallback to currentWorkspaceName which i also prefer ;)

it's because it does not matter in the cases. But I agree that we should be specific here, I'll adjust the tests and require the workspace name to be set

phew, that was tedious but I think I adjusted all affected tests (let's see what the CI says)
I love our tests, but IMO we test too much from the _event_s perspective (there are 83 hits for /the event .* was published with payload/). IMO we should mostly execute commands and expect events and/or state as a result (except for the rare low-level cases that are not feasible to recreate with commands).. But that's for another time :)

Thx ❤️, Yes i have an issue as reminder for that: #4336

but i was under the impression that most things now use commands already.

mhsdesign

Tested with migrating the Neos Ui e2e events and running the ui e2e tests as well.

We also have to adjust our 3 event export:

Neos.Demo
Neos ui testcase twodimension
Neos ui testcase onedimension

i would propose to just edit the workspace live in there so its easy to review the changes.

Neos.ContentRepository.LegacyNodeMigration/Classes/NodeDataToEventsProcessor.php

Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php

bwaidelich · 2024-05-17T15:46:37Z

@mhsdesign thanks for the feedback, I'll take care of a follow-up.

We also have to adjust our 3 event export

Not sure if I got that.. Is that something that can be done in this PR directly or is that Neos UI?

Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php

…pacename-to-events

mhsdesign · 2024-05-17T17:43:42Z

Okay, so the migration will def take some time like a good minute for 19k events.
but what surprises me that it says Migration applied to 13320 events, while i do have 19.433?

mhsdesign · 2024-05-17T17:44:30Z

nevermind. I oversaw the continue;

mhsdesign

Oke seems to work. There will definitely be a lot of missing's depending on how many node migrations you did ... or maybe there is something wrong. Hard to tell because the events dont tell me why this change was made.

There is an initiating user id set? So not cli + node migration? If that is true and you also have preservations we should maybe reconsider.

On the otherhand ill +1 this and well see if there are bugs once this is in use by projections at first.

bwaidelich · 2024-05-17T18:04:01Z

There is an initiating user id set? So not cli + node migration? If that is true and you also have preservations we should maybe reconsider.

I read that three times but still don't understand it ;)

mhsdesign · 2024-05-17T18:22:34Z

what i meant @bwaidelich, what do you think lead to this event:

payload:

{
  "contentStreamId": "9cc426e4-3520-4e48-8272-609267850177",
  "nodeAggregateId": "6b6e1251-4346-494f-ac56-526a30a5741d",
  "originDimensionSpacePoint": {
    "language": "pl"
  },
  "affectedDimensionSpacePoints": [
    {
      "language": "pl"
    }
  ],
  "propertyValues": {
    "uriPathSegment": {
      "value": "laszelechow",
      "type": "string"
    }
  },
  "propertiesToUnset": [],
  "workspaceName": "missing"
}

metadata

{
  "commandClass": "Neos\\ContentRepository\\Core\\Feature\\NodeModification\\Command\\SetSerializedNodeProperties",
  "commandPayload": {
    "nodeAggregateId": "6b6e1251-4346-494f-ac56-526a30a5741d",
    "originDimensionSpacePoint": {
      "language": "pl"
    },
    "propertyValues": {
      "uriPathSegment": {
        "value": "laszelechow",
        "type": "string"
      }
    },
    "propertiesToUnset": [],
    "workspaceName": "missing:9cc426e4-3520-4e48-8272-609267850177"
  },
  "initiatingUserId": "3e5ed99c-16e4-4a12-a23b-4489563e85e7",
  "initiatingTimestamp": "2023-11-07T23:16:15+00:00"
}

bwaidelich · 2024-05-17T18:46:25Z

what do you think lead to this event

@mhsdesign Uh, no idea. But if no corresponding workspace was created before, it should have never happened to begin with.
I guess that's no longer possible with the current version.
And it's not directly related to this PR, is it?

mhsdesign · 2024-05-17T19:15:32Z

And it's not directly related to this PR, is it?

only kindof, i feared we did something wrong that i have so many "workspaceName": "missing" were in my routes.

But as said i think there is no bug on our side. I just kindof fear that there might be the need to be some $workspaceName === 'missing' in php? But we cant do anything against it.

Every code must assume that the workspace name is no longer valid either way so +1.

…eAggregate`

kitsunet · 2024-05-18T11:42:23Z

For the shown event were does the workspaceName come from there? As in, where is this event created code wise?

mhsdesign · 2024-05-18T12:29:13Z

For the shown event were does the workspaceName come from there? As in, where is this event created code wise?

that is the problem due to missing metadata we dont surely now but i suspect from previous structure adjustments or more likely node migrations (@dlubitz mentioned they operate currently on cs instead of ws and that is what he is about to change: #4685)

kitsunet · 2024-05-18T13:34:15Z

Neither of those would have a user ID though? I rather meant what event is it and which command handler creates it...

mhsdesign · 2024-05-18T14:31:59Z

Neither of those would have a user ID though? I rather meant what event is it and which command handler creates it...

yeah thats the thing that makes me rethink this as well. Its a properties was set event: (but you should have the same db dump as well)

17,Workspace:user-jon,2,WorkspaceWasRebased
18,ContentStream:33edf226-c93d-4243-8769-619977fb0c39,1,NodePropertiesWereSet
19,ContentStream:9cc426e4-3520-4e48-8272-609267850177,0,ContentStreamWasForked
20,ContentStream:9cc426e4-3520-4e48-8272-609267850177,1,NodePropertiesWereSet
21,ContentStream:90bcfbf8-c444-48f8-9911-ba0954ac795a,0,ContentStreamWasForked
22,ContentStream:63d38bd9-453e-4af1-8625-848716678038,10,NodePropertiesWereSet
23,ContentStream:9cc426e4-3520-4e48-8272-609267850177,2,ContentStreamWasRemoved

(the above is event 20)

kitsunet

So much details, thank you for going through it all.

mhsdesign · 2024-05-19T07:37:05Z

We also have to adjust our 3 event export

Not sure if I got that.. Is that something that can be done in this PR directly or is that Neos UI?

I meant these three json l:

all adjusted ;)

With neos#5002 `\Neos\Neos\FrontendRouting\Projection\DocumentUriPathFinder::isLiveContentStream` is obsolete.

With neos/neos-development-collection#5002 `\Neos\Neos\FrontendRouting\Projection\DocumentUriPathFinder::isLiveContentStream` is obsolete.

WIP: FEATURE: Add workspaceName to relevant events

6f3be98

Resolves: #4996

github-actions bot added the 9.0 label Apr 19, 2024

bwaidelich commented Apr 19, 2024

View reviewed changes

Neos.ContentRepository.Core/Classes/Feature/Common/PublishableInterface.php Outdated Show resolved Hide resolved

Merge branch '9.0' into feature/4996-add-workspacename-to-events

c3019e6

# Conflicts: # Neos.ContentRepository.Core/Classes/Feature/NodeMove/Event/NodeAggregateWasMoved.php

dlubitz mentioned this pull request May 17, 2024

AssetUsage needs to switch from contentStreamId to WorkspaceName #5060

Closed

bwaidelich added 7 commits May 17, 2024 12:53

Merge branch '9.0' into feature/4996-add-workspacename-to-events

dfd1007

Fix NodeVariationInternals

ce99c0b

Remove unused getWorkspaceName() from `EmbedsContentStreamAndNodeAg…

85d2922

…gregateId` interface

Rename and tweak PublishableInterface

f4d2f41

Forward contentGraph workspace name to events

02624aa

Fix behat tests

8140490

Provide event migration

0f46361

bwaidelich changed the title ~~WIP: FEATURE: Add workspaceName to relevant events~~ !!! FEATURE: Add workspaceName to relevant events May 17, 2024

bwaidelich marked this pull request as ready for review May 17, 2024 13:14

bwaidelich requested review from mhsdesign, kitsunet and dlubitz May 17, 2024 13:16

Remove unused namespace imports and add missing type annotation

ded6c13

bwaidelich commented May 17, 2024

View reviewed changes

mhsdesign reviewed May 17, 2024

View reviewed changes

bwaidelich added 2 commits May 17, 2024 18:01

Remove obsolete todo comment

4f3dcc5

Fix event migration

d4f40e3

mhsdesign reviewed May 17, 2024

View reviewed changes

Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php Outdated Show resolved Hide resolved

bwaidelich and others added 2 commits May 17, 2024 19:32

Tweak event migration

b4e533d

Merge remote-tracking branch 'origin/9.0' into feature/4996-add-works…

7db53b6

…pacename-to-events

mhsdesign approved these changes May 17, 2024

View reviewed changes

TASK: Use $contentGraph->getContentStreamId() instead of from `$nod…

f635c9e

…eAggregate`

mhsdesign requested a review from nezaniel May 18, 2024 07:01

bwaidelich added 2 commits May 18, 2024 15:26

Add workspaceName to all relevant events in behat tests

eabb990

Merge branch '9.0' into feature/4996-add-workspacename-to-events

104b2a1

bwaidelich added 4 commits May 18, 2024 15:35

Fix namespace import in TetheredNodeAdjustments

a545c21

Add more workspaceNames to tests..

7d97ebc

Fix typo in behat test

e47bac8

one more "workspaceName"...

7caba3c

kitsunet approved these changes May 18, 2024

View reviewed changes

kitsunet merged commit d10bc0f into 9.0 May 18, 2024
10 checks passed

kitsunet deleted the feature/4996-add-workspacename-to-events branch May 18, 2024 19:39

This was referenced May 19, 2024

TASK: Adjust to workspace name in events neos/neos-ui#3788

Merged

TASK: Adjust to workspace name in events neos/Neos.Demo#193

Merged

This was referenced May 20, 2024

TASK: Remove usages of $node->subgraphIdentity->contentStreamId in structure adjustments #5081

Merged

Adjust AssetUsageProjection to use workspace instead of content stream id #5084

Closed

mhsdesign mentioned this pull request Jun 18, 2024

TASK: Check against LiveWorkspaceName Instead Of Comparing CS Ids neos/redirecthandler-neosadapter#78

Merged

mhsdesign mentioned this pull request Jun 19, 2024

TASK: Check against LiveWorkspaceName Instead Of Comparing CS Ids #5149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

!!! FEATURE: Add `workspaceName` to relevant events #5002

!!! FEATURE: Add `workspaceName` to relevant events #5002

bwaidelich commented Apr 19, 2024 •

edited

Loading

bwaidelich left a comment •

edited

Loading

bwaidelich May 17, 2024

mhsdesign May 17, 2024

bwaidelich May 17, 2024

mhsdesign May 17, 2024

bwaidelich May 17, 2024

mhsdesign May 17, 2024

nezaniel May 18, 2024 •

edited

Loading

mhsdesign May 18, 2024

bwaidelich May 18, 2024

bwaidelich May 18, 2024

mhsdesign May 18, 2024 •

edited

Loading

mhsdesign left a comment

bwaidelich commented May 17, 2024

mhsdesign commented May 17, 2024

mhsdesign commented May 17, 2024

mhsdesign left a comment

bwaidelich commented May 17, 2024

mhsdesign commented May 17, 2024

bwaidelich commented May 17, 2024

mhsdesign commented May 17, 2024 •

edited

Loading

kitsunet commented May 18, 2024 •

edited

Loading

mhsdesign commented May 18, 2024

kitsunet commented May 18, 2024

mhsdesign commented May 18, 2024

kitsunet left a comment

mhsdesign commented May 19, 2024

		$contentGraph->getWorkspaceName(),
		$contentGraph->getContentStreamId(),

!!! FEATURE: Add workspaceName to relevant events #5002

!!! FEATURE: Add workspaceName to relevant events #5002

Conversation

bwaidelich commented Apr 19, 2024 • edited Loading

Breaking change

Event migration

bwaidelich left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezaniel May 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign May 18, 2024 • edited Loading

Choose a reason for hiding this comment

mhsdesign left a comment

Choose a reason for hiding this comment

bwaidelich commented May 17, 2024

mhsdesign commented May 17, 2024

mhsdesign commented May 17, 2024

mhsdesign left a comment

Choose a reason for hiding this comment

bwaidelich commented May 17, 2024

mhsdesign commented May 17, 2024

bwaidelich commented May 17, 2024

mhsdesign commented May 17, 2024 • edited Loading

kitsunet commented May 18, 2024 • edited Loading

mhsdesign commented May 18, 2024

kitsunet commented May 18, 2024

mhsdesign commented May 18, 2024

kitsunet left a comment

Choose a reason for hiding this comment

mhsdesign commented May 19, 2024

!!! FEATURE: Add `workspaceName` to relevant events #5002

!!! FEATURE: Add `workspaceName` to relevant events #5002

bwaidelich commented Apr 19, 2024 •

edited

Loading

bwaidelich left a comment •

edited

Loading

nezaniel May 18, 2024 •

edited

Loading

mhsdesign May 18, 2024 •

edited

Loading

mhsdesign commented May 17, 2024 •

edited

Loading

kitsunet commented May 18, 2024 •

edited

Loading