Problem/Motivation

In #3550942: Add the ability to move content between workspaces we need to use workspaces.manager in workspace.association, but we can't because at the moment we have the reverse dependency in place (the workspace manager requires the workspace association service).

Additionally, #3486378: [Plan] Allow for / implement simplified content workflow with workspaces will enable the Workspaces module on a lot more sites, and, even if it's just working under the hood, we need to ensure that it's as fast and least disruptive as possible.

Proposed resolution

Deprecate WorkspaceManagerInterface::purgeDeletedWorkspacesBatch() and move the revision purging code to WorkspacesHooks::cron() - this allows to remove the WorkspaceAssociation and State dependencies

Add WorkspaceSwitchEvent and move clearing various static caches to an event subscriber - this allows us to remove the EntityMemoryCache and WorkspaceInformation dependencies; we do need to add event_dispatcher as a new dependency, but we use a service closure to ensure that it's only instantiated when needed

Remove the logging when a workspace could not be activated (the access exception is kept) - this log message is useless and it allows us to remove the AccountProxyInterface and LoggerInterface dependencies

Remaining tasks

Review.

User interface changes

Nope.

Introduced terminology

N/A

API changes

- a deprecation: \Drupal\workspaces\WorkspaceManagerInterface::purgeDeletedWorkspacesBatch()
- an API addition: \Drupal\workspaces\Event\WorkspaceSwitchEvent

Data model changes

Nope.

Release notes snippet

TBD.

Issue fork drupal-3553581

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
amateescu’s picture

Issue summary: View changes
smustgrave’s picture

Just going to post questions

1. I'm seeing the deprecations are changing in WorkspaceManager but believe these changes have already been in a release so is that okay?
2. I see we are switching to cron and limiting to 50, which makes sense. My question is if those workspaces are large with lots of content each any concern using cron then?
3. Do we need a CR for the new Event and Subscriber?

Manual tests I did

Base setup.
1. Latest 11.x
2. Standard profile install
3. Apply MR
4. cleared cache

Managing spaces
I created a new workspace (dev)
Switched to it
Switched to stage
Deleted dev

Working

Publishing content

Switched to stage
Created an Article (Test stage)
Switched to live
Made sure I could not edit Test stage
Switched to stage
Published content
Seeing the Test stage page in live space

Working

Functionality wise appears to be working.

amateescu’s picture

I'm seeing the deprecations are changing in WorkspaceManager but believe these changes have already been in a release so is that okay?

I think it is. BC for constructors is done on a best-effort basis, and I was actually surprised I was able to do this change while preserving full BC :) The change here just means that classes extending WorkspaceManager that were updated for 11.2 will need to be updated again for 11.3 before 12.0.0 lands.

I see we are switching to cron and limiting to 50, which makes sense. My question is if those workspaces are large with lots of content each any concern using cron then?

The method we're deprecating was already ran by cron, we just moved the code around so there's no actual behavior change.

Do we need a CR for the new Event and Subscriber?

Right, I was waiting for the review thread for it to reach a conclusion. Added an additional CR: https://www.drupal.org/node/3553871

smustgrave’s picture

In that case I would be a +1 but would be more comfortable is maybe @catch could give it a look. I'm happy to mark though so he can still merge.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

I'm told he already did so ignore #7 :)

  • catch committed fa1f9710 on 11.x
    Issue #3553581 by amateescu, smustgrave: Slim down WorkspaceManager
    
catch’s picture

Status: Reviewed & tested by the community » Fixed

I had one last comment about the entity type manager switch to Entity::load() - I think we still need it injected, @amateescu pointed out it's the earliest point we load it, brought it back as a service closure instead.

Committed/pushed to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

amateescu’s picture

Thanks! Published the new CRs.

amateescu’s picture

Issue summary: View changes

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.