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

[mhsdesign]

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]

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

[bwaidelich]

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

you are both work horses 🐴 🐴

Too ez

[mhsdesign]

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]

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]

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]

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!