TASK: Remove SerializedCommandInterface again and use `RebasableToO…

[mhsdesign]

…therWorkspaceInterface` directly

Review for #5348

hmm why not CommandInterface|RebasableToOtherWorkspaceInterface :D

The SerializedCommandInterface doesnt add anything actually

its the same as if a command would not extend CommandInterface but only implements RebasableToOtherWorkspaceInterface

id say CommandInterface should be PublicCommandInterface or sth and then we have the RebasableToOtherWorkspaceInterface commands too which are not handlebar from outside

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

[kitsunet]

I was looking in a similar direction earlier and I like the signatures a lot more this way. But I find then RebasableToOtherWorkspace is probably the wrong name as it is a functional interface in that it actually declares an API and also says nothing about commands. Also this implies that all "internal" commands must be rebasable and I am not sure we can make that assumption for the future?

[bwaidelich]

Yep, makes a lot of sense, thanks!

Also this implies that all "internal" commands must be rebasable and I am not sure we can make that assumption for the future?

I think we can, because the CommandBus will only accept one or the other

Personally I don't really care about the RebasableToOtherWorkspace, it's completely internal. But we could rename it to RebasableCommandInterface maybe?

But I'm wondering: Do we really need to rename the CommandInterface?

[mhsdesign]

But I'm wondering: Do we really need to rename the CommandInterface?

i did it because now now every command implements it ... and i would be fine with naming it RebasableCommandInterface :)

though as said earlier, it should not leak to the neos ui right?

[kitsunet]

I don't understand this could still be CommandInterface and be implemented by every command. which actually would make sense...

[mhsdesign]

Just to put everything together:

This is an adjustment pr for #5348 because i didnt want to push the changes directly, it should be merged into the other.

Bastians pr introduces a separation of public CommandInterface and SerializedCommandInterface. We want the split so the cr only handles the public ones.

Do i get this correct that the question is to introduce a BaseCommandInterface that is implemented by all? But how should that help?

[mhsdesign]

Okay so naming is obviously hard again (as always)

so before doing any renames, to reduce diff and stuff and also walk problems out of the way like:

If we rename the interface to RebasableCommandInterface why is there a class RebaseableCommand that does NOT implement it? (my fault btw)

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

Line 19 in e9d576b

final readonly class RebaseableCommand

I would just suggest to move on with the essence of the change. Ill revert the PublicCommandInterface rename.

lets just get this done and focus on more important parts.
Dont get me wrong i love getting every detail right, but its impossible 😅

[mhsdesign]

so in the end i like to have the following structure (regarding the naming of things i have no preference)

/**
 * Common (marker) interface for all public api commands of the content repository
 *
 * Note that this interface does not mark all commands.
 * Complex public commands will not be serializable on its own and are required to be serialized into a {@see RebasableToOtherWorkspaceInterface}
 *
 * @internal sealed interface. Custom commands cannot be handled and are no extension point!
 */
interface CommandInterface
{
}

/**
 * Common (marker) interface for all **commands** that need to be serialized for rebasing to other workspaces
 *
 * If the api command {@see CommandInterface} is serializable on its own it will directly implement this interface.
 * For complex commands a serialized counterpart - which is not api - will be build which implements this interface.
 *
 * During a rebase, the command (either the original {@see CommandInterface} or its serialized counterpart) will be deserialized
 * from array {@see RebasableToOtherWorkspaceInterface::fromArray()} and reapplied via the {@see CommandSimulator}
 *
 * Reminder: a rebase can fail, because the target content stream might contain conflicting changes.
 *
 * @internal used internally for the rebasing mechanism of content streams
 */
interface RebasableCommandInterface extends \JsonSerializable
{
    public function createCopyForWorkspace(
        WorkspaceName $targetWorkspaceName,
    ): self;

    /**
     * called during deserialization from metadata
     * @param array<string,mixed> $array
     */
    public static function fromArray(array $array): self;
}

And for a followup (yet to be done finished in #5356) we will introduce a CommandSerializerInterface like this:

See TASK: Introduce CommandSerializerInterface to centralise serialisation and get hold of the $serializedCommand

And with the CommandSerializerInterface we can finally centralise the RebaseableCommand::enrichWithCommand logic:

TASK: Centralise RebaseableCommand::enrichWithCommand

<?php

declare(strict_types=1);

namespace Neos\ContentRepository\Core\CommandHandler;

use Neos\ContentRepository\Core\Feature\Common\RebasableToOtherWorkspaceInterface;

/**
 * Optional interface for all content repository command serializer
 *
 * See {@see RebasableToOtherWorkspaceInterface} for more details regarding serialized commands.
 *
 * @internal no public API, because commands are no extension points of the CR
 */
interface CommandSerializerInterface
{
    public function canSerialize(CommandInterface $command): bool;

    public function serialize(CommandInterface $command, CommandHandlingDependencies $commandHandlingDependencies): RebasableToOtherWorkspaceInterface;
}

edit, not so easy after all: #5356 (comment) the CommandSerializerInterface will likely not be that pretty but also needs a union of CommandInterface| RebasableToOtherWorkspaceInterface