Problem/Motivation
Problem #1: Workspaces can no longer be published from the CLI (or via cron), because #3037136: Make Workspaces and Content Moderation work together introduced the requirement that a workspace can be published only if the current user has access to do that. The rationale for that change was that we needed a mechanism that would allow us to prevent the publication of a workspace if certain conditions are not met.
Problem #2: There is no way to react to the publication of a workspace. This issue affects current HEAD as well: the workspace publisher has to hard-code a call to WorkspaceAssociation::postPublish()
, which can be avoided if the workspace association would react to a post-publish event instead.
Proposed resolution
Instead of relying on access checking, introduce an event that allows any subscriber to prevent the publishing process.
Remaining tasks
Review.
User interface changes
Nope.
API changes
- new WorkspaceEvents::WORKSPACE_PRE_PUBLISH
and WorkspaceEvents::WORKSPACE_POST_PUBLISH
events added.
- \Drupal\workspaces\WorkspaceAssociationInterface::postPublish()
is deprecated in favor of reacting to the post-publish event.
Data model changes
Nope.
Release notes snippet
N/A.
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff-44.txt | 1.33 KB | amateescu |
#44 | 3242564-44.patch | 32.34 KB | amateescu |
| |||
#38 | interdiff-38.txt | 1.25 KB | amateescu |
#38 | 3242564-38.patch | 32.31 KB | amateescu |
| |||
#37 | interdiff-37.txt | 6.48 KB | amateescu |
Comments
Comment #2
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedThis should do it.
Comment #3
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedComment #5
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedFixed those fails.
Comment #6
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC + 1, I first had trouble understanding this patch and why it's needed, but after looking at it again at some time it made click:
- The code within content_moderation is mainly moved from the access hook to the new publish event
- Not using access means it's not bound to permissions (the IS makes that already clear)
- The publish event has a rich interface to stop the publish
- A once stopped publish cannot be restarted anymore (Is there a use-case for that? Probably not ...)
In addition to that you can now react to postPublish.
Comment #8
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedProbably a random fail.
Comment #11
SpokjeBack to RTBC per #6 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.
Comment #13
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedAnother random fail.
Comment #15
SpokjeRandom test failure
Comment #16
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedThis is now a Drupal 9.4.x issue.
Comment #18
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedRandom fail.
Comment #19
quietone CreditAttribution: quietone at PreviousNext commentedComment #20
quietone CreditAttribution: quietone at PreviousNext commentedRunning tests on D10
Comment #21
larowlanThis feels like a task rather than a bug
Comment #22
amateescu CreditAttribution: amateescu for Tag1 Consulting commented@larowlan the bug is problem #1 from the IS, workspaces can not be published from the CLI or cron, which is a regression introduced by #3037136: Make Workspaces and Content Moderation work together.
Comment #23
catchRe-titling to make it a bit clearer what this issue is about.
Comment #24
catchIt's not clear to me why we have to add an entirely new API via events to fix this bug. Why not add the logic that's being moved from access to the event subscriber, to a protected method on this class?
If there's a separate need for events, then that could be its own issue. However in general I've been pushing back against the addition of new events, due to issues like #2825358: Event class constants for event names can cause fatal errors when a module is installed and #2237831: Allow module services to specify hooks.
Also not sure why this needs a completely custom implementation, and couldn't use the entity validation API maybe? https://www.drupal.org/docs/drupal-apis/entity-api/entity-validation-api... although I realise publishing a workspace isn't the same as just saving it, but the entity validation API is decoupled from saving, so... maybe?
Also missing a test-only fail patch here - can't really see from reading the test coverage whether the original bug report is covered by the changes to coverage.
Comment #25
catchComment #26
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedThanks for reviewing this!
Because the workspace publisher shouldn't know why or how Content Moderation decides if a workspace can be published or not, that logic is solely CM's responsibility. Additionally, other modules will also need the ability to signal that a workspace can't be published, for example alternative workflow modules that are not based on CM.
There is definitely a need to for these events, at the moment there's no way for other modules to react on workspace publishing, for example to keep track of which entities were published as part of a workspace publication. That example is needed for the revert functionality, among others.
As for adding events in its own issue, I thought we generally introduce a new API with at least one usage, and this is a great use-case because we're fixing a bug as well.
I think the rich API provided by this event warrants their use here, but I'm happy to explore alternatives if really needed :)
I don't think so, wouldn't that prevent editing a workspace (e.g. its title) if CM says that it can't be published?
That's right, there was no explicit test coverage for the CLI publishing bug, so I added one.
Comment #28
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #29
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedRerolled and updated the patch with the new way of doing events, using class names instead of special constants.
Comment #30
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedThe testbot is really strict these days :)
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commented@trigger_error('The event dispatcher service should be passed to WorkspaceOperationFactory::__construct() since 9.4.0. This will be required in Drupal 10.0.0.', E_USER_DEPRECATED);
1, Will have to be updated. A 2nd change record may be needed to announce this service has a new parameter. And added to the message.
getSubscribedEvents
2. Should be typehinted
3. Any new functions that return should be typehinted
Comment #32
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedThanks for reviewing! Addressed #31, except adding an additional CR because
WorkspaceOperationFactory
is marked as@internal
.Comment #33
smustgrave CreditAttribution: smustgrave at Mobomo commentedThink this ready now.
Comment #34
longwaveWe need to add backward compatibility here; the $event_dispatcher argument could be the source workspace if callers have not been updated, in which case we need to issue a deprecation.
Comment #35
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedWorkspacePublisher
is marked as@internal
but I guess it can't hurt to add the BC layer. Fixed #34 and updated the CR to mention it.Comment #36
larowlanFunctionally this looks ready to go to me, however our standards for new code have changed since this was first created. I was tempted to fix these on commit but then it got to the point where a peer review felt appropriate.
Feel free to self RTBC after all these changes
I realise this patch predates the requirement, but as this is new code, we need to use constructor property promotion here now
And need a return type hint here
nit; >80 here
Can we get property type hints here too
and here too for constructor property promotion
And a void return type here
Should we make this fluent? if not can we add a void return type hint
Can we add void here too
Comment #37
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedThanks for reviewing @larowlan, fixed all the points from #36 :)
Comment #38
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedFixed #37.. still getting used to this stuff.
Comment #40
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedLooks like a random fail.
Comment #42
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedComment #43
larowlanShould we have publish exception extend access exception? Someone may have code that is catching the access exception that will no longer catch from here? The fact we had to update our code here seems to indicate this is a potential BC break. If we don't want to have them extend that, we should at least add a second change notice with that detail.
Can we typehint $reason at all?
string|\Stringable
perhaps?Comment #44
amateescu CreditAttribution: amateescu for Tag1 Consulting commented1. I think it makes sense for the publish exception to extent access exception, because the reason for not being able to publish a workspace could very well be access-related.
2. Sure thing, done :)
Comment #45
larowlanSaving issue credits
Comment #47
larowlanWhoops, didn't mean to update the status.
Committed to 10.1.x.
Not backporting because of the new API/deprecations.
Published the change notice