!!! 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.