Problem/Motivation
In the service definition content_moderation.services.yml we have
content_moderation.workspace_subscriber:
class: Drupal\content_moderation\EventSubscriber\WorkspaceSubscriber
arguments: ['@entity_type.manager', '@?workspaces.association']
tags:
- { name: event_subscriber }
Notice that @?workspaces.association is optional.
In the class itself \Drupal\content_moderation\EventSubscriber\WorkspaceSubscriber we have
/**
* Constructs a new WorkspaceSubscriber instance.
*
* @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
* The entity type manager service.
* @param \Drupal\workspaces\WorkspaceAssociationInterface $workspaceAssociation
* The workspace association service.
*/
public function __construct(
protected readonly EntityTypeManagerInterface $entityTypeManager,
protected readonly WorkspaceAssociationInterface $workspaceAssociation
) {}
Notice that $workspaceAssociation is required.
Proposed resolution
Make $workspaceAssociation optional in \Drupal\content_moderation\EventSubscriber\WorkspaceSubscriber.
Remaining tasks
Implement the proposed resolution.
User interface changes
Nope.
API changes
Nope.
Data model changes
Nope.
Release notes snippet
Nope.
Issue fork drupal-3411308
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
Comment #3
ndf commentedComment #5
ndf commented@Keshev Patel, thanks for the MR. Can you please explain why you opted for the required parameter solution? This is helpful for other people to review your MR.
Comment #6
smustgrave commentedYes an explanation is needed. Also a test case
Comment #7
pooja saraah commentedIn the original service definition, the second argument is specified as @?workspaces.association. The ? indicates that this argument is optional, meaning that the WorkspaceSubscriber class can accept either the workspaces.association service or no service at all.
In the modified service definition, the optional operator ? has been removed from the second argument. This means that the WorkspaceSubscriber class now explicitly expects the @workspaces.association service as a required argument.
Purpose of the Change:
The modification aligns the service definition with the actual constructor requirements of the WorkspaceSubscriber class. By removing the optional operator, the service definition accurately reflects that the WorkspaceSubscriber class expects the @workspaces.association service to be provided during instantiation.
Comment #9
dstorozhukAfter apply the patch from merge request !5978 I have that error when trying to enable/disable any module:
The service "content_moderation.workspace_subscriber" has a dependency on a non-existent service "workspaces.association".Comment #10
amateescu commentedIn the service definition, the
workspaces.associationservice must be marked as optional because it might not exist (for example when the Workspaces module is not enabled).Note that this is not a bug because
\Drupal\content_moderation\EventSubscriber\WorkspaceSubscriberis an event subscriber class, and it shouldn't be instantiated unless it needs to react to theWorkspacePrePublishEventevent, which is only invoked when the Workspaces module is enabled. If some other module instantiates it.. well, it shouldn't :)If we want to make a change in core though, we need to mark the argument as optional in
\Drupal\content_moderation\EventSubscriber\WorkspaceSubscriber::__construct(). Updated the issue summary to reflect that.Comment #12
stefan.kornThanks for the explanation.
As mentioned in #3242564: Workspaces can't be published from the command line devel quits with WSOD on "Event info". And yes, this is happening if the content moderation module is installed and the workspaces module is not.
Basically
is failing then.
It seems to me that getListeners() without event argument is a valid call, therefore I am wondering if this should really end in error here (no exceptions for the method pointing in that direction either) even if the workspaces module is not installed.
And the difference between service definition (optional) and implementation (required) does also seem not really straightforward.
I have amended the MR to make the change in the implementation and not the service definition.
Comment #13
amateescu commentedOk, let's do this, the change is harmless and allows the class to be instantiated :)
Comment #16
catchCommitted/pushed to 11.x and cherry-picked to 10.3.x, thanks!
Comment #17
stefan.korn