!!! TASK: Throw exceptions in unnecessary rebase and publish cases #5337

mhsdesign · 2024-11-01T13:10:57Z

based on #5396

new `WorkspaceCommandSkipped` exception:

Exception to denote that the workspace operation was skipped. No events were published.

Workspace publishing/discarding

Command skipped if there are no (selected) publishable changes.

To avoid this exception being thrown into the outer world it should be caught and logged or the case were its
likely that the operation might be a noop can easily be determined beforehand via Workspace::hasPublishableChanges():

if ($workspace->hasPublishableChanges()) {
    $contentRepository->handle(...);
}

Workspace rebase

Command skipped if the workspace is not outdated.

Workspace base change

Command skipped when attempting to change the base workspace to the currently set base workspace.

Note:

The case is not handled gracefully with a no-op as there would be no traces (emitted events) of the handled command,
and the original content stream id is kept (for publish operations).
This exception denoting the operation is obsolete should harden the interaction and make behaviour more explicit.

Upgrade instructions

Review instructions

Checklist

Code follows the PSR-2 coding style
Tests have been created, run and adjusted as needed
The PR is created against the lowest maintained branch
Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
Reviewer - The first section explains the change briefly for change-logs
Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

bwaidelich

You're a workhorse, thanks for taking care.
+1 by reading

mhsdesign · 2024-11-01T13:39:09Z

It was @kitsunet work i downloaded the patch as i thought it would go to waste :D

bwaidelich · 2024-11-01T14:36:49Z

It was @kitsunet work i downloaded the patch as i thought it would go to waste :D

you are both work horses 🐴 🐴

...ContentRepository.Core/Classes/Feature/WorkspacePublication/Exception/NoChangesException.php

Too ez

mhsdesign · 2024-11-03T18:35:03Z

Okay well its not thaaaat easy ...

Following case have to be thought out:

what should happen if nodesToPublish is empty?
- Should it even be allowed to be set to empty? Or enforced by the command that this CANNOT happen?
- ... same for discard individual nodes...
- line:
  
  neos-development-collection/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php
  
  Line 438 in 3e7035f
  
  if ($command->nodesToPublish->isEmpty() || !$workspace->hasPublishableChanges()) {
what should happen if the node id in nodesToPublish did not match anything?
- Eg either if the node never contained change
- doesnt exist at all
- or was concurrently published and doesnt contain changes anymore?
- ... same for discard individual nodes...
- line:
  
  neos-development-collection/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php
  
  Line 465 in 3e7035f
  
  // almost a noop (e.g. random node ids were specified) ;)
what should happen on full or partial publish if there are no pending changes in the workspace?
- line:
  
  neos-development-collection/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php
  
  Line 182 in 3e7035f
  
  if (!$workspace->hasPublishableChanges()) {
what should happen on full or partial discard if there are no pending changes in the workspace?
- line:
  
  neos-development-collection/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php
  
  Line 673 in 3e7035f
  
  if (!$workspace->hasPublishableChanges()) {
what should happen on rebase if the workspace is not outdated?
- line:
  
  neos-development-collection/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php
  
  Line 347 in 3e7035f
  
  // no-op if workspace is not outdated and not forcing it

kitsunet · 2024-11-04T10:34:54Z

Okay well its not thaaaat easy ...

Following case have to be thought out:
* what should happen if `nodesToPublish` is empty?

I think we could/should prevent this in the command?! If we want to throw we can decide further up front (aka UI).

* what should happen if the node id in `nodesToPublish` did not match anything?

IMHO that is a fair possibility that can happen with race conditions on shared workspaces for example, so I would handle it gracefully and accept this as no-op. BUT given next point I probably opt for exception and graceful handling further up the chain.

* what should happen on full or partial publish if there are no pending changes in the workspace?

Yeah this is the tricky case compared to the above, IF you had a race condition and send identifiers to a partial publish that are no longer changed this is similar to starting a full publish if there are no pending changes (anymore).

* what should happen on full or partial **discard** if there are no pending changes in the workspace?

Same as above I guess. If we start to throw.

* what should happen on rebase if the workspace is not outdated?

Probably again same as above.

For all of this though we should make sure there is a way out even for unexpected situations. Which could e.g. be the rebase or discard even if things look unchanged.

mhsdesign · 2024-12-05T11:47:04Z

Todo rebase on #5385
and in the constructor of partial publish and discard require the node ids to be NOT empty.

As the $nodeIdsToPublish is rather to be seen as filter than as expectation - imagine you enter a non-existing node aggregate id - id say that its okay if this is then a noop? one cannot be mad if a filter doesnt match anything? Also what if one of the nodes in the filter is unmatched but there is one node found to be published? If we throw it would only be consistent to make it required that all node ids specified MUST translate to a change.

That is somehow not really the behaviour a filter is made for. Thought especially throwing would at least give feed back to users in the Ui instead that the publish button keeps being orange like shown here: #4614
With the simplified publishing that cannot happen anymore but yeah ... im not fully sure.

mhsdesign · 2024-12-12T10:47:49Z

Todo: Also other noops like in change base workspace are not desired and have to be prevented in \Neos\Neos\Domain\Service\WorkspacePublishingService::changeBaseWorkspace instead!

by enforcing this via "non-empty-array" we can better catch runtime type errors beforehand via phpstan. As otherwise the event store will fail with > Writable events must contain at least one event Some internal utility methods previously also returned possibly empty "Events" instances which were then transformed via iterator_to_array back to arrays and merged. This was not simplified to just work with arrays directly to denote that they might be empty. In cases were we work with fully sure filled intermediate event arrays we use non-empty-array. With this now fine-grained type checking aside from the original found set properties error following cases will also break with 0 events: - handleSetSerializedNodeProperties (known) - handleSetSerializedNodeReferences - handlePublishWorkspace and handlePublishIndividualNodesFromWorkspace if there were no applied events that implement PublishableToWorkspaceInterface -> goes into direction of neos#5337 - structure adjustment TETHERED_NODE_WRONGLY_ORDERED

mhsdesign · 2025-01-22T08:34:04Z

Okay i polished the change and adjusted the tests.

Also i checked in the command handling

neos-development-collection/Neos.ContentRepository.Core/Classes/ContentRepository.php

Line 110 in 47fc20b

foreach ($toPublish as $yieldedEventsToPublish) {

that when we use yield that we never return and no-op without issuing any events. This is basically the followup of the philosophy from #5357 and #5396 to ensure that each command maps to at least one event.

… conflict resolution Attempting to publish 0 changes after the adjustments in the core neos/neos-development-collection#5337 will throw an error and is also needless: > The command "PublishIndividualNodesFromWorkspace" for workspace admin-admington must contain nodes to publish To avoid unnecessary interaction - which is also tested by this test: > Publish + Syncing: Create a conflict state between two editors, then try to publish and choose "Drop conflicting changes" as a resolution strategy during automatic rebase We dont continue publishing if the publish button is not orange basically ;) In the future i think this should be improved that another phase and screen is added so this is done more transparently.

kitsunet · 2025-01-27T11:38:43Z

The behavior test errors are valid as the expectation changed now (we gracefully handled the empty rebase before).

Unlike what I wrote in December, I am pretty convinced now that we are making a mistake with all this, but as I wrote in another place, we can revert from this stance easier than the other way around so I am ok with merging it given the majority seems to be in favor of doing it this way.

kitsunet

Looks fine, test error still left to fix but that should be easy enough.

mhsdesign · 2025-01-27T14:24:16Z

Fyi i also followed your idea to have one exception for these things instead of the NoChangesException (which i interpreted as no publishable changes) and BaseWorkspaceUnchangedException. Its not all part of the WorkspaceCommandSkipped which makes this behaviour now even more clear.

…djust tests

…f noop

…angeBaseWorkspace

…PublishingService

separate more complex discard commands into own tests as attempting to discard 0 nodes fails now and its harder to test that way in batches.

Instead of keeping it specific to publishing and naming it `NoPublishableChanges` we use it as generic no-op exception for workspace commands. Especially because `becauseWorkspaceToRebaseIsNotOutdated` already deviates from the 'PublishableChanges' kind.

…dSkipped`

by enforcing this via "non-empty-array" we can better catch runtime type errors beforehand via phpstan. As otherwise the event store will fail with > Writable events must contain at least one event Some internal utility methods previously also returned possibly empty "Events" instances which were then transformed via iterator_to_array back to arrays and merged. This was not simplified to just work with arrays directly to denote that they might be empty. In cases were we work with fully sure filled intermediate event arrays we use non-empty-array. With this now fine-grained type checking aside from the original found set properties error following cases will also break with 0 events: - handleSetSerializedNodeProperties (known) - handleSetSerializedNodeReferences - handlePublishWorkspace and handlePublishIndividualNodesFromWorkspace if there were no applied events that implement PublishableToWorkspaceInterface -> goes into direction of neos/neos-development-collection#5337 - structure adjustment TETHERED_NODE_WRONGLY_ORDERED

github-actions bot added the 9.0 label Nov 1, 2024

mhsdesign mentioned this pull request Nov 1, 2024

FEATURE: Introduce Workspace::hasPublishableChanges #5332

Merged

6 tasks

bwaidelich previously approved these changes Nov 1, 2024

View reviewed changes

kitsunet reviewed Nov 2, 2024

View reviewed changes

...ContentRepository.Core/Classes/Feature/WorkspacePublication/Exception/NoChangesException.php Outdated Show resolved Hide resolved

mhsdesign force-pushed the task/publishing-v3-followup branch from 8848678 to 8117aeb Compare November 3, 2024 18:27

mhsdesign mentioned this pull request Nov 12, 2024

BUG: Cut & paste in different dimension leads to unpublishable change #4614

Closed

1 task

This was referenced Jan 13, 2025

BUGFIX: Ensure that its not attempted to publish 0 events #5396

Merged

BUGFIX: Re-introduce stricter checks for subtree en/disabling and tagging #5357

Merged

mhsdesign force-pushed the task/publishing-v3-followup branch from 8117aeb to a134031 Compare January 22, 2025 08:31

mhsdesign marked this pull request as ready for review January 22, 2025 08:34

mhsdesign requested review from bwaidelich and kitsunet January 22, 2025 08:34

mhsdesign mentioned this pull request Jan 22, 2025

After resolved conflict, don't attempt to re-synchronise neos/neos-ui#3908

Closed

kitsunet approved these changes Jan 27, 2025

View reviewed changes

mhsdesign added 3 commits January 27, 2025 15:56

!!! TASK: Throw exceptions in unnecessary rebase and publish cases

1ffc995

TASK: Prevent publish/discard commands to exist with empty filter

2b45bdf

TASK: Throw NoChangesException consistently also during discard and a…

cc8329c

…djust tests

mhsdesign added 8 commits January 27, 2025 15:57

PATCH: Restore workspace rebase and throw skipped exception instead o…

2f8c0ab

…f noop

TASK: Introduce BaseWorkspaceUnchangedException to avoid no-op for Ch…

5d7e5ef

…angeBaseWorkspace

TASK: Improve documentation of WorkspaceRebaseFailed exception

4116558

TASK: Document possible occurrence of NoChangesException in Workspace…

d33aeb8

…PublishingService

TASK: Fix asset usage tests by removing obsolete rebase

fbbe05c

TASK: Fix cr security tests

070c3e3

separate more complex discard commands into own tests as attempting to discard 0 nodes fails now and its harder to test that way in batches.

TASK: Combine BaseWorkspaceUnchangedException with `WorkspaceComman…

6bd31ee

…dSkipped`

mhsdesign force-pushed the task/publishing-v3-followup branch from 7cf488a to 6bd31ee Compare January 27, 2025 14:58

mhsdesign merged commit 52ea360 into neos:9.0 Jan 27, 2025
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

!!! TASK: Throw exceptions in unnecessary rebase and publish cases #5337

!!! TASK: Throw exceptions in unnecessary rebase and publish cases #5337

mhsdesign commented Nov 1, 2024 •

edited

Loading

bwaidelich left a comment

mhsdesign commented Nov 1, 2024

bwaidelich commented Nov 1, 2024

mhsdesign commented Nov 3, 2024

kitsunet commented Nov 4, 2024 •

edited

Loading

mhsdesign commented Dec 5, 2024

mhsdesign commented Dec 12, 2024

mhsdesign commented Jan 22, 2025

kitsunet commented Jan 27, 2025

kitsunet left a comment

mhsdesign commented Jan 27, 2025

!!! TASK: Throw exceptions in unnecessary rebase and publish cases #5337

!!! TASK: Throw exceptions in unnecessary rebase and publish cases #5337

Conversation

mhsdesign commented Nov 1, 2024 • edited Loading

new WorkspaceCommandSkipped exception:

bwaidelich left a comment

Choose a reason for hiding this comment

mhsdesign commented Nov 1, 2024

bwaidelich commented Nov 1, 2024

mhsdesign commented Nov 3, 2024

kitsunet commented Nov 4, 2024 • edited Loading

mhsdesign commented Dec 5, 2024

mhsdesign commented Dec 12, 2024

mhsdesign commented Jan 22, 2025

kitsunet commented Jan 27, 2025

kitsunet left a comment

Choose a reason for hiding this comment

mhsdesign commented Jan 27, 2025

mhsdesign commented Nov 1, 2024 •

edited

Loading

new `WorkspaceCommandSkipped` exception:

kitsunet commented Nov 4, 2024 •

edited

Loading