Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: FEATURE: Extract workspace metadata and user-assignment to Neos #5146

Draft
wants to merge 14 commits into
base: 9.0
Choose a base branch
from

Conversation

bwaidelich
Copy link
Member

@bwaidelich bwaidelich commented Jun 17, 2024

Introduces WorkspacePublishingService as replacement for the current Neos Workspace "active record" model.

Introduces WorkspaceService as central authority to manage Neos workspaces.

Todos

  • Replace \Neos\Neos\Ui\ContentRepository\Service\WorkspaceService
  • Replace \Neos\Neos\Domain\Service\WorkspaceNameBuilder
  • Cleanup \Neos\Neos\Ui\Fusion\Helper\WorkspaceHelper
  • Fix \Neos\Neos\Command\WorkspaceCommandController

Related: #4726

Introduces `WorkspacePublishingService` as replacement for the current Neos `Workspace` "active record" model.

Introduces `WorkspaceService` as central authority to manage Neos workspaces.

Related: #4726
@github-actions github-actions bot added the 9.0 label Jun 17, 2024
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some early thoughts thanks for the progress so far ❤️

Notes from my side:

  • remove \Neos\Neos\Domain\Service\UserService::deletePersonalWorkspace and \Neos\Neos\Domain\Service\UserService::removeOwnerFromUsersWorkspaces
  • deprecate \Neos\Neos\Service\UserService::getPersonalWorkspaceName
  • remove WorkspaceNameBuilder again

Comment on lines 105 to 106
): WorkspaceName
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
): WorkspaceName
{
): WorkspaceName {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but this is just a draft. Please don't add comments re cosmetics just yet

/**
* @return array<ContentRepositoryId>
*/
public function getContentRepositoryIds(): array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks i also need this for listing all content repositories via cli or for example for the new 9.0 neos setup :)

Comment on lines 65 to 73
// Check if name already match name pattern to prevent unnecessary transliteration
if (self::hasValidFormat($name)) {
return self::fromString($name);
} catch (\InvalidArgumentException $e) {
// Okay, let's transliterate
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ this part always bummed me :D


$this->workspaceService->createWorkspace(
$contentRepositoryId,
\Neos\Neos\Domain\Model\WorkspaceTitle::fromString($title),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be used instead of fq

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a temporary work around, as the \Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceTitle will be replaces in the next step.
But you're right, I should probably rather import the new namespace if the other one is to be removed.

This is just a draft though

@@ -359,16 +367,18 @@ public function listCommand(string $contentRepository = 'default'): void
}

$tableRows = [];
$headerRow = ['Name', 'Base Workspace', 'Title', 'Owner', 'Description', 'Status', 'Content Stream'];
$headerRow = ['Name', 'Classification', 'Base Workspace', 'Title', 'Description', 'Status', 'Content Stream'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we dont want to show the user id? Well i guess that makes sense as its too technical anyway and the human readable name with classification personal will already suffice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll certainly re-add it, but with the new Workspace <-> Role assignment, maybe rather in some new showCommand() that lists all contributors and their role

Comment on lines +39 to +42
/**
* @var string
*/
protected $Persistence_Object_Identifier;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* @var string
*/
protected $Persistence_Object_Identifier;
/**
* This property will be introduced via Flows persistence magic aspect
* We declare it here already as workaround to not have to use {@see PersistenceManager::getIdentifierByObject()} to access this identifier.
* @var string
*/
protected $Persistence_Object_Identifier;

nice "hack" idea ... :D I approve ... probably better than injecting the persistence manager to do the same? But the above property should get a nice doc comment

*
* @api
*/
final readonly class UserId implements \JsonSerializable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this replace the cr's user id thing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably won't replace it because the CR also has a concept of users (currently only used to store the initiatingUserId in the event metadata).
But IMO it makes sense to have a dedicated "Neos UserId" as well.. Maybe we can rename one to make the distinction clearer..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Sebobo there are quite some changes in this workspace controller ... just to inform you as youre working on this as well :D

),
WorkspaceClassification::ROOT => WorkspacePermissions::create(
read: true,
publish: $this->privilegeManager->isPrivilegeTargetGrantedForRoles($userRoles, 'Neos.Workspace.Ui:Backend.PublishAllToLiveWorkspace'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the privilge should belong to neos/neos or neos/neos-core still and not be moved via #5118

Copy link
Member

@mhsdesign mhsdesign Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or as alternative of moving the Neos.Workspace.Ui:Backend.PublishAllToLiveWorkspace target back to neos, or creating a new empty target for this which will be interpreted here, we could check for the LivePublisher role?

$this->securityContext->hasRole('Neos.Neos:LivePublisher')

edit: we cant move the target back to Neos.Neos:Backend.PublishAllToLiveWorkspace because it protects Ui\Controller\WorkspaceController->publishWorkspaceAction which is in the other package.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I was wondering the same.. There is some mismatch between workspace <-> user/role assignment and the global Flow roles & privileges..
I'll think about it tomorrow!

Comment on lines 84 to 90
/**
* @return array<ContentRepositoryId>
*/
public function getContentRepositoryIds(): array
{
return array_map(ContentRepositoryId::fromString(...), array_keys($this->settings['contentRepositories']));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* @return array<ContentRepositoryId>
*/
public function getContentRepositoryIds(): array
{
return array_map(ContentRepositoryId::fromString(...), array_keys($this->settings['contentRepositories']));
}
/**
* @return iterable<ContentRepositoryId>
*/
public function getContentRepositoryIds(): iterable
{
return array_map(ContentRepositoryId::fromString(...), array_keys($this->settings['contentRepositories'] ?? []));
}

this way we can easily introduce a collection dto at some point and it wont crash if nothing is configured.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fyi since you wondered and i stumbled upon it ... we have this interface for the following reason:

#165 (comment)

and actually this thing is now useless with the new cr.

bwaidelich added a commit that referenced this pull request Aug 2, 2024
Extracted from #5146 this just improves stability of the `WorkspaceName` value object by
- Restricting the allowed value range to 30 lower case characters and properly enforce it
- Adding a `tryFromString()` constructor
- Exposing the `MAX_LENGTH` and use that for the corresponding database schemas
- 100% test coverage

Related: #4726
{
/**
* This prefix determines if a given workspace (name) is a user workspace.
*/
public const PERSONAL_WORKSPACE_PREFIX = 'user-';

/**
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The following properties have been turned into actual fields to improve IDE support:
Constructor promoted arguments of an @internal constructor are all "internal", but in this case they are not. Besides the @deprecated tags are ignored otherwise

$workspaceNameCandidate = WorkspaceName::transliterateFromString($candidate);
$workspaceName = $workspaceNameCandidate;
while ($contentRepository->getWorkspaceFinder()->findOneByName($workspaceName) instanceof Workspace) {
$workspaceName = WorkspaceName::fromString(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the lowercase enforcement generateRandomString is not safe and will crash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants