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
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:
- 3553581-slim-down-workspacemanager
changes, plain diff MR !13550
Comments
Comment #3
amateescu commentedComment #4
amateescu commentedComment #5
smustgrave commentedJust 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.
Comment #6
amateescu commentedI 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
WorkspaceManagerthat were updated for 11.2 will need to be updated again for 11.3 before 12.0.0 lands.The method we're deprecating was already ran by cron, we just moved the code around so there's no actual behavior change.
Right, I was waiting for the review thread for it to reach a conclusion. Added an additional CR: https://www.drupal.org/node/3553871
Comment #7
smustgrave commentedIn 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.
Comment #8
smustgrave commentedI'm told he already did so ignore #7 :)
Comment #10
catchI 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!
Comment #13
amateescu commentedThanks! Published the new CRs.
Comment #14
amateescu commented