Scheduler integration with core Content moderation

Hi kriboogh,
This is an interesting idea - thanks for starting off the discussion. Yes, it would be good to look at this. I've not used the core content moderation module yet, but it would make sense to integrate with it directly if possible.

In 7.x we have had discussions on if/how to make Scheduler more generic (see the related issues). Obviously what we do not want to do is replicate lots of code, which had been suggested for things like scheduling the change of promoted/demote and sticky/non-sticky. It would have to be done in an adaptable and extendable way. I am not certain exactly how to achieve this yet, but it would definitely be worth persuing.

Do any of the followers of this thread have ideas on how this might be worked on?

Jonathan

Looks like content moderation alters the (un)publish action to skip for nodes that are in moderation.

/**
 * Implements hook_action_info_alter().
 */
function content_moderation_action_info_alter(&$definitions) {

  // The publish/unpublish actions are not valid on moderated entities. So swap
  // their implementations out for alternates that will become a no-op on a
  // moderated node. If another module has already swapped out those classes,
  // though, we'll be polite and do nothing.
  if (isset($definitions['node_publish_action']['class']) && $definitions['node_publish_action']['class'] == PublishNode::class) {
    $definitions['node_publish_action']['class'] = ModerationOptOutPublishNode::class;
  }
  if (isset($definitions['node_unpublish_action']['class']) && $definitions['node_unpublish_action']['class'] == UnpublishNode::class) {
    $definitions['node_unpublish_action']['class'] = ModerationOptOutUnpublishNode::class;
  }
}

Might be the best way to create a publish action based on each state.
So that we can configure which states are allowed to be automatically published.

That will also impact vbo and other functionalities, maybe it would be better to create another module for that?

I created a sandbox module that allows content moderation to work with scheduler:
https://www.drupal.org/sandbox/svdhout/2827074

The module replaces the PublishNode action with it's own ModerationPublishNode.
If the entity that this action is performed uppon has moderation enabled, it will perform it's own logic. If not, it will use the parent (PublishNode) execute method.

I subscribed to the SchedulerEvent to get every revision that has a scheduling date.
That makes it possible to schedule multiple revisions of content.

This looks really interesting and helpful. I have skimmed your code and it will take me a while to work out exactly what you are doing, as I have not used the Content Moderation module yet and don't know how it works or what exactly it does. Thank you for doing this work - I now need to start understanding it properly ...

Does your sandbox works with the new changes (using core workflow module) of Content Moderation? Anyway, I think lots of people are waiting on a solution to properly schedule moderation, so thanks for having taken the time to build it!

@Simon Georges - looking at the code in @svdhout's sandbox, it won't work with Workflows module in 8.3.x

Damn it ;-)
I'll work on that on my next D8 project if needed.

At 7.x the Scheduler Workbench module provided integration betweem Scheduler and Workbench Moderation. There is not yet a version of this at 8.x but it might be useful to look at it, as that may be the best place to make the integration between Scheduler 8.x and Core Content Moderation. See #2820026: Drupal 8 version of Scheduler Workbench Integration

I might be oversimplifying this but it seems like you could patch the publish() method in ScheduleManager.php to do something to the effect of:

If content moderation is enabled for this $node object ($node->get('moderation_state') ?), $node->set('moderation_state', 'published');

+1 - I would like this functionality as well.

I guess my follow-up question is if 'published' is a universal 'end' state for moderation OR if that will vary based on the workflow configuration?

My use case would require a varied end state. It might not always be Published.

Hi ,

I am also looking for integration between scheduler and workbench moderation modules for drupal 8.x
I would basically need the automatic scheduling functionality implemented in my site
Can someone confirm when can we expect the solution to this ? or is there any workaround available ?

Thanks,
Radha.

I haven't been able to navigate exactly how you would figure out the 'end' state for a workflow. Here is some code that gets you part of the way there.

/** @var \Drupal\content_moderation\ModerationInformation $moderation_info */
        $moderation_info = Drupal::service('content_moderation.moderation_information');

        if ($moderation_info->isModeratedEntity($node)) {
          $workflow = $moderation_info->getWorkflowForEntity($node);

In the meantime I am attaching a patch that will work if your end state is 'published.'

Attaching patch that works if the end state is 'published.'

I can confirm that the patch in #16 works, but I added validation to make sure the moderation transition is valid and moved the snippet earlier in the publish() method so that the scheduled date isn't lost if invalid. I also added logging for invalid transitions so that the publishing failure isn't silent.

I considered putting the validation in the isAllowed() method for use by unpublish() as well, but that looked a lot more complicated as we'd have to somehow know how the "unpublish" action maps to a moderation state that might not be "archive".

I added a check for the content_moderation module being enabled. Let's see if it passes the tests.

Doh! It helps if I upload the correct files.

Thanks for this @PapaGrande.

Just to state the obvious, the tests pass now because the new code is not executed due to the addition of if ($this->moduleHandler->moduleExists('content_moderation'). So what we really need is additional tests that enable the content_moderation module and check this new functionality, much in the same way that I have done for our Rules testing.

I am not in a position to do this right now, so can you or anyone else in this thread help with tests?

Jonathan

Yes indeed, tests are needed. I'll get to those if I can.

As it turns out my patch #23 fails on manual testing so I've updated it to be more like #15 for now. The issue is that validation should happen when the date is set in the node form (or via a REST API call) and not when publish() is run during a cron job. I haven't figured out yet how to invoke the content_moderation validation plugin, with the 'published' moderation state temporarily set, from within or along side the scheduler validation plugin when the node is validated. If anyone has any suggestions of how to do that, I'd appreciate it.

Hey,

Me and a colleague (@postovan-dumitru) has been working on this and we created a custom module as part of the scheduler module that integrates with the content moderation and workflow modules.

The module adds new base fields to nodes just like the scheduler module does but for state transitions instead, the base fields have a custom constraint validator to make sure that the selected transition is valid.

The options for the state transitions only contains the default revision states, which means that the node will be published or un-published only if it's in one of those states. Adding any other default states will populate the options automatically.

There are a few @todo comments in the code for things that might need a look, for example the way we set the states to display might need a review since we're not sure if it's the proper way to do it.

We've also not had the time to implement any tests so if anyone is good at writing tests please do, otherwise we will hopefully get time to do it ourselves soon.

Please review the patch and make any improvements or comments on things we could improve ourselves.

Best regards

I've updated the path with some improvements related to the constraint validators, the publish state and un-publish state fields now use their own validators and the validation and error messages are much better.

I also added Kernel tests for both the constraint validators, there are some more assertions that should probably be done, I've added some @todo tags.

I'm currently working on improving the way options are added to the publish state and un-publish state fields since currently options that a user might not have access to are being added which could lead to a user being able to publish content even if he/she should not be able to.

So to solve this I'm working on adding custom field widgets for each of the select lists that takes into account the current user and the current state of the content.

I will probably not be able to finish the custom field widgets this week but probably sometime the next week.

Applying updated patch with permission checks and field widget has been removed since it's not required and not having a widget allows the end user to select the widget plugin they want to use and still have the options visible.

There are still some work left to do, the constraint validators should probably check to make sure that both a date and scheduling state has been set, currently it only validates the state transitions.

There should also be more tests added to cover more of the possible user cases. It would be great to have some functional tests where the functionality is tested from a users point of view.

Great work!

Haven't tried the patch yet, will give it a try soon. In the meanwhile it looks like patch on #28 is a merge of two or more patches?
This makes review a bit harder. Would be good to have a patch file with all the changes merged.

Leaving Needs Review so we can gather more feedback.

This is a merged version of #28 with some Coding Standard issues been fixed (a couple still left).

I haven't tried yet, but approach looks good.
My only question is instead of using our own widget, why don't we extend ModerationStateWidget?
Not super important.

I'm going to do some manual tests.

Probably attaching the files would help :p

Attached path setting @group scheduler, as the standard looks like using the root module.

Worth mentioning at some point I couldn't uninstall scheduler due the following error:

[error] SQLSTATE[42S22]: Column not found: 1054 Unknown column 'publish_state' in 'where clause': SELECT 1 AS expression
FROM
{node_field_data} t
WHERE publish_state IS NOT NULL
LIMIT 1 OFFSET 0; Array
(
)

The only way to fix it was to run drush ev '\Drupal::service("entity.definition_update_manager")->applyUpdates();', not even drush cr could help.

But I tried to reproduce it again unsuccessfully. Do we need to add the applyUpdates() to scheduler_content_moderation_integration.install?

Probably the testbase needs to be abstract. All tests are green locally, just testbot being peaky. :)

Hey,

Thanks for continuing work on this since I'll probably not be able to do anything more with the integration since I'm no longer working with the customer that wanted me to do it.

Regarding the widget, i decided to not have a custom widget since it's not possible to select the style you want like radio or select when using a custom widget. With the current approach you are free to select how you want the options displayed and validation etc. will still work.

Hey @NormySan ,
thanks for your answer.

i decided to not have a custom widget

No, what I meant is why we haven't extended \Drupal\content_moderation\Plugin\Field\FieldWidget\ModerationStateWidget, which comes out of the box with pulling current content available states and transitions.
It's not that important, I can see the widget and contraints you've implemented are brilliant, just trying to avoid duplicating the effort of maintaining 2 widget instead of use (and improve if needed) the one in core.

I'm testing this in a project and it's working very well so far. A part from the question above I agree a Functional test covering the whole journey from a user point of view would be ideal.
Leaving Needs Review in order to gather more feedback.

@gambry Ah yes, now i get what you mean :)

If I remember correctly we decided to not use that widget because we needed to display moderation states that can be published to based on the current state of the node and states that can be un-published to based on the publish to state.

I'm not 100% sure that it actually does this currently but if I recall correctly there should be some code that limits the available publish/un-publish states that you can select.

Yes it does, but I guess if we extend ModerationStateWidget and we add this bit of functionality it may work.

Thanks for your thoughts!

Hey this is great! Thank you very much gambry and NormySan for you work.

@gambry - if you want to make the change you suggest, to extend ModerationStateWidget please go ahead. It might well be a good choice, so do try it and provide a patch (and interdiff) so we can see what you have done.

Jonathan

Hello,

To be more specific, the initial idea was to populate the options with allowed state transitions.

This has nothing to do with the functionality, just a preference. Instead of extending the state moderation widget and filtering the states via a hook alter, I opted for extending a base select widget. This way, I could provide a clean way to populate values in the select element and also attribute the constraints the same way, instead of altering.

Me and @normysan were developing this while working for the same client. If you feel like extending the moderation widget insted, let me know and I will try to attend to this in my spare time.

Hope you guys find this useful :)

Have a nice day!

Hey @Postovan Dumitru and @jonathan1055 . Just to clarify I'm super happy with current approach. I'm using it with clients' projects and it works brilliantly.

I was just wondering if doing the same by extending an existing widget was worth, so we don't have to bother about maintaining a whole widget.
I will give it a try and post my feedbacks.
I will try to work on this at DrupalCamp London, so if anyone is going to attend give me a shout and we'll work together!

Thanks everyone for the hard work.

Hello,

I have tested the patch from comment 35 on Drupal core 8.5.0 and the last dev version of scheduler and it does not work.

Steps :
- enable scheduling and content moderation on content type article
- create an article, published
- edit the article and save the draft
- edit the draft, add a publishing date and set publish state to "published"
- run cron when the publishing date is passed
- result : the draft is not published and there is still a publishing date programmed (indicated in the scheduler fieldset)

#43 can you try to debug and give more info?

I've just gone through your steps and content is published for me after running cron, on 8.5.0.

Is it possible to hide the select fields for choosing the publish and unpublish state when only one state exists?

I installed the 8.x-1.x-dev branch, applied the patch from #35, enabled the module "Scheduler Content Moderation Integration" and configured one of my content types. I'm able to see the dropdown and set the time for publish/unpublish, but on the node save it's throwing this error:

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Component\Plugin\Exception\PluginNotFoundException</em>: The &quot;SchedulerPublishState&quot; plugin does not exist. in <em class="placeholder">Drupal\Core\Plugin\DefaultPluginManager-&gt;doGetDefinition()</em> (line <em class="placeholder">52</em> of <em class="placeholder">core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php</em>).

On Drupal core 8.5.0... any thoughts?

@nsciacca clean the cache. Depending by how you enable modules (i.e. through `drush en`) that may not clean the cache.

Hi nsciacca,

at #46, you should clear the cache after applying the patch.

@gambry & @szato I have cleared the cache, via drush cr, and on the Performance page, with no luck.

The only thing when I was installing the module was it was throwing this error as well, until I manually removed those rows from my config table:
Unable to install Scheduler Content Moderation Integration, scheduler.settings, views.view.scheduler_scheduled_content already exist in active configuration.

This was the output of the patch application - all good until it hit the "SchedulerManager.php" which I had to supply the path to:

patching file scheduler_content_moderation_integration.info.yml
patching file scheduler_content_moderation_integration.module
patching file SchedulerModerationWidget.php
patching file ConstraintValidatorBase.php
patching file PublishStateConstraint.php
patching file PublishStateConstraintValidator.php
patching file SchedulerModerationConstraint.php
patching file SchedulerModerationConstraintValidator.php
patching file UnPublishStateConstraint.php
patching file UnPublishStateConstraintValidator.php
patching file ConstraintTestBase.php
patching file PublishedStateConstraintTest.php
patching file UnPublishedStateConstraintTest.php
can't find file to patch at input line 906
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/src/SchedulerManager.php b/src/SchedulerManager.php
|index de17b18..2d5eb3c 100644
|--- a/src/SchedulerManager.php
|+++ b/src/SchedulerManager.php
--------------------------
File to patch: src/SchedulerManager.php
patching file src/SchedulerManager.php

Unable to install Scheduler Content Moderation Integration, scheduler.settings, views.view.scheduler_scheduled_content already exist in active configuration.

That's weird. However those config files are not related to this patch, but part of Scheduler. Drupal + Sheduler's hook_uninstall() take care of them when uninstalling. Looks like enabling the module try to re-install scheduler?

This was the output of the patch application - all good until it hit the "SchedulerManager.php" which I had to supply the path to:

Patch has not been applied completely? How are you patching the module?

Which version of Scheduler are you using?

All error looks like a localised problem, as none of them relates to anything inside the patch. Also the error "The 'SchedulerPublishState' plugin does not exist" is when drupal can't load the plugin, which is a file so either the cache is old OR you don't have the file in the right position scheduler_content_moderation_integration\src\Plugin\Validation\Constraint\PublishStateConstraint.php.
Or scheduler_content_moderation_integration is not installed, which is even weirder.

@gambry - bingo, thanks! It turns out that when I applied the patch it dumped all the files that should have been inside a subfolder of "scheduler_content_moderation_integration" into the /scheduler folder. I manually moved the all to the correct place and cleared caches and it seems to be working, at least not err'ing out anymore. I had applied the patch from the /scheduler directory using "patch < [patchname]" - which I thought was the proper way, but I suspect maybe my directory wasn't writeable and it couldn't create the folders. Anyways, thanks for your help!

Edit: after testing I can confirm the same bug as in #43 above:
If you have an existing published node, create a new draft and set a publishing date, the new draft is not transitioned into published. It does print the message "This post is unpublished and will be published..." but it doesn't show up on Scheduled tab and after cron runs the scheduled date remains populated in the fieldset. It seems to only move the latest revision through the state transitions, but not pending drafts when there is an existing published revision. Publishing drafts that do not have any published revisions before it is working correctly.

Glad it worked and kudos++ for the additional details for #43. I'm going to have a look at this today.
Thanks!

Applied patch on
drupal core 8.5.1
Scheduler 8.x-1.0

Tested all the steps as mentioned by #43 , looks like it works perfectly

@arijits.drush have you tested what described in #51 (second part)?

@gambry, transitioning draft->published when the node was previously published did not work in my testing either (#43). This is because ScheduleManager is not querying or loading the latest revisions. I'm looking at this today, so if you're already working on something let me know.

Updated to address #43 by querying and loading for the latest revision if using this submodule.

Thanks for your work @seanpclark !

I haven't manually tested yet, however:

1. Passing the whole container as argument makes me a bit nervous. :(
   It would be much better to alter the scheduler.manager service from within scheduler_content_moderation_integration submodule. It should be pretty safe and at that point we know content_moderation is available as its a dependency. Check the documentation about altering services.

   BUT

   That ship has probably already sailed so we are stuck with the main service. So I do suggest we call the \Drupal::get()method, instead of injecting the whole container.
   If we standardise most of the changes - so they work despite content_moderation been enabled - and we just call \Drupal::get('content_moderation.moderation_information') where needed that would avoid injecting the whole container.
   It's not best practice, but 1) we are already doing it in the class and 2) best practice would be to alter the service anyway. Thoughts?

2. +++ b/src/SchedulerManager.php
   @@ -40,6 +48,20 @@
   +   * ¶
   

   Here and somewhere else in your changes you've got space issues.

3. +++ b/src/SchedulerManager.php
   @@ -98,6 +126,10 @@
   +      // Query by latest revision if using content moderation integration.
   +      if ($this->schedulerModerationEnabled) {
   +        $query->latestRevision();
   +      }
   

   I think entities (nodes in this case) are revisionable despite content_moderation being enabled.

   Would be enough to use EntityTypeInterface::isRevisionable() in the condition?

4. +++ b/src/SchedulerManager.php
   @@ -255,6 +285,10 @@
   +      // Query by latest revision if using content moderation integration.
   +      if ($this->schedulerModerationEnabled) {
   +        $query->latestRevision();
   +      }
   

   Same as above.

5. +++ b/src/SchedulerManager.php
   @@ -476,2 +508,29 @@
   +    // When working with content moderation we want the latest revision.
   +    if ($this->schedulerModerationEnabled) {
   +      // Load nodes by latest revision id.
   +      foreach ($nids as $nid) {
   +        $nodes[] = $this->moderationInformation->getLatestRevision('node', $nid);
   +      }
   +    }
   

   Again as above... if loadMultiple() doesn't already return latest revision, we can do the loop always despite content_moderation been on? Thoughts?

Again I'm just trying to clean-up a bit the code so we don't depend by anything not needed.

And we do need test coverage for this case.

Thanks a lot!

@gambry, thanks for reviewing. I'll make updates today. Those points sound good to me and you're right that the check for content moderation when querying by latest revision isn't needed.

@gambry I made some changes based on your comments, and did an interdiff against the previous patch in #35 thinking that'd be easier to review. I removed the content moderation service for getting the latest revision. Also, I was going to inject entity.query since I was already updating the query, but I see that's deprecated so I'm using entity_type.manager. I'll see if I can get some time to create a test covering the transition steps.

Hi @seanpclark, thanks for your work. All on #59 looks good. Replacing entityManager with entityTypeManager could have wait for a follow-up, but if maintainers like the work done here that's good.
Soon or later it would have had to be done anyway.

I've done a sprint during the weekend, so I spent a bit of time writing the tests. I hope that's not causing issues to your work.
Also #59 has some Coding Standard issues I've also fixed.

Attached patch add test coverage for bug described from #43 and #51 and fixed on #56 and #59.

I've refactored what was ConstraintTestBase test class to SchedulerContentModerationTestBase, because otherwise they would have been pretty much the same.

Test-only patch is tricky, as it's work up to #32 with test from attached patch. Tests are red with previous work and green with new work, confirming the bug was genuine and has been fixed.

I hope the work in this issue is pretty much complete so it can be RTBC and merged soon, as this is an important piece of feature for scheduler.
Big thanks to anyone who has worked on this issue!!!

I missed the @group annotation. Adding this to patches on #60

Hi,

Firstly, HUGE thank-you to everyone here for this work! It's critical for what I'd imagine is a common use case of the module.

I am trying the non 'test-only' patch from #63 against 8.x-1.0. Everything is working beautifully but there is one small thing that seems off - when the scheduled publish/unpublish occurs, it ends up creating two new revisions, instead of one.

At first I thought perhaps this was because (a) core content moderation requires creating a new revision upon a state change; (b) Scheduler module provides the option to create a new revision with the scheduled change - and I had that enabled. However, even when disabling the Scheduler option to create a new revision, I was still getting two new revisions on a content type that's also using workflow/moderation. (With scheduler applied but not workflow/moderation, or with workflow/moderation applied but not scheduler, I get only one revision, so I figure it's happening somewhere within this integration.)

All that said, this doesn't appear to actually break any of the intended functionality.

@tea.time

Hello,

I do remember something similar when I was initially developing this with @NormySan.
I might be wrong but I think it's due to the transition, as in, it creates a revision when it goes from ex: draft->published(this creates the first published one), then after the node is saved with the desired moderation state, it might create another one. I am not sure about this statement, but if it's like this, it's not affecting anything, but we will have a lot of unwanted revisions.

@gambry spent a lot of his time on this patch, so he should know a lot more than me regarding the process here :)
I will try having a look at this either today or tomorrow after work.

Thank you for posting your observation!

I pinged timmillwood to have his thoughts. I personally remember the same behaviour with Content Moderation in general: two new revisions when the content is published.

I wasn't expecting "with workflow/moderation applied but not scheduler, I get only one revision" (from #65). I will have a look as well.

Thanks!

Yeah I confirm two revisions are created if content is processed by scheduler.
Looks like one revision has field_revision.publish_state set, the other one has it empty. Both are published.

Also as we are now altering the service, we need a empty hook_post_update_NAME() in order to re-fresh the container.
I'm still a big fan of #57.1 , so if anyone is going to tackle this keep an eye on whether going through that way may be less messy and avoid the hook_post_update_NAME().

Setting this to Needs Work.

Another Issue I am seeing is that nodes that do not have a date/time set still require the states to be chosen.
Here is a screenshot.

This results in a Choose the unpublished state for the node. during the Widget Validation.

I was testing this patch and was able to locate the issue with the duplicate revisions.

The current code for the SchedulerManager publish function is:

// Use the actions system to publish the node.
$this->entityTypeManager->getStorage('action')->load('node_publish_action')->getPlugin()->execute($node);

// Invoke the event to tell Rules that Scheduler has published the node.
if ($this->moduleHandler->moduleExists('scheduler_rules_integration')) {
	_scheduler_rules_integration_dispatch_cron_event($node, 'publish');
}

// If scheduler_content_moderation_integration is enabled, set to
// published state.
if ($this->schedulerModerationEnabled && $node->get('moderation_state')) {
	$state = $node->publish_state->value;
	$node->set('moderation_state', $state);
	$node->publish_state->value = NULL;
}

// Trigger the PUBLISH event so that modules can react after the node is
// published.
$event = new SchedulerEvent($node);
$dispatcher->dispatch(SchedulerEvents::PUBLISH, $event);
$event->getNode()->save();

$this->entityTypeManager->getStorage('action')->load('node_publish_action')->getPlugin()->execute($node);
This triggers a save when it is ran.

$event->getNode()->save();
This saves it again after the event trigger for PUBLISH.

I changed the code to be this:

// If scheduler_content_moderation_integration is enabled, set to
// published state.
if ($this->schedulerModerationEnabled && $node->get('moderation_state')) {
	$state = $node->publish_state->value;
	$node->set('moderation_state', $state);
	$node->publish_state->value = NULL;
}

// Use the actions system to publish the node.
$this->entityTypeManager->getStorage('action')->load('node_publish_action')->getPlugin()->execute($node);

// Invoke the event to tell Rules that Scheduler has published the node.
if ($this->moduleHandler->moduleExists('scheduler_rules_integration')) {
	_scheduler_rules_integration_dispatch_cron_event($node, 'publish');
}

// Trigger the PUBLISH event so that modules can react after the node is
// published.
$event = new SchedulerEvent($node);
$dispatcher->dispatch(SchedulerEvents::PUBLISH, $event);

I Moved the set moderation state above the node_publish_action.
Removed the save after the Trigger the PUBLISH event.

I tested this, and I only get one revision.

Applied feedback from #70, testing.

From a UX perspective it would make sense if we could configure the default Publish and Unpublish states.

@mpp,

About the configuration of the default states:
Should it be set on a content type level?

Should it be set on a content type level?

Hi @Postovan Dumitru, a third party setting on the entity would make sense, yes:

A global setting would be better than no setting at all but having it on the content entity would allow to configure a different workflow per type.
Also, we already have the UI in place on the content entity so adding it there should be easy.

@mpp

Thanks for the fast reply!

As far as I remember developing the initial stage of the module with @normysan, the publish and unpublish here are set as fields in the node.
We have 2 options here, either we provide the third party settings for each content type, either we could provide configuration for the fields themselves, like default values.

I might be completely wrong though. I could pick this up during Friday or the weekend. But I want to say that your suggestion is really good.

I would really like to hear from @gambry though, as he was the person to move this module from an initial skeleton to the one we're having today.

On a side note: There is a set of modules in D8 bundled as Lightning, and I think they have Scheduler there as well. What are the chances we're working in vain here?

@Postovan Dumitru, there's indeed a field widget for both Publish state and Unpublish state. Adding the settings there could make sense but I'd prefer not to show these fields on the content form. So we'd need default settings anyhow.

Thanks for the tip on lightning_scheduler, unfortunately it seems to be developed as part of the lightning_workflow project which renders it useless as a separate project and there's similar functionality with this project...

There's an issue in the lightning_workflow queue where I suggest we discuss their approach. Perhaps lightning_workflow can use scheduler in the future.

$this->entityTypeManager->getStorage('action')->load('node_publish_action')->getPlugin()->execute($node);
This triggers a save when it is ran.

Mmmh... publish action should be disabled when content_moderation is ON. But looking at the code and node_publish_action is now deprecated and replaced by 'entity:publish_action:node' which we should probably use (both during publishing and unpublishing) to avoid the double saving AND use the right API.

I would really like to hear from @gambry though, as he was the person to move this module from an initial skeleton to the one we're having today.

TBH the decision is not easy. We currently don't have a proper configuration where to save this info.
Best option is probably being consistent with scheduler base module and adding a global default on scheduler.settings and/or a per-content type override on on the entity third party settings. See scheduler_form_node_form_alter() for details.
This require a config/schema/scheduler_content_moderation_integration.schema.yml definition of node.type.*.third_party.scheduler_content_moderation_integration

@Postovan Dumitru are you happy to give it a try to both actions?

@gambry

Sorry for the late response, overloaded at work. I will happily give a try to both of the actions, will have a look during the weekend, this time for sure.

Thanks for your input, really appreciate it! :)

Hey guys, still working on this, have a couple of thoughts I would like to share.

@jonnyeom, about your comment #69, I would like to point out that perhaps you should set the widget for the publish_state and unpublish_state to the one scheduler_content_moderation_integration is providing. This way, the fields would only display values which are usable considering the transition you want to make via content_moderation.

Considering my statement above, should we consider setting the custom widget(SchedulerModerationWidget) by default for these fields in scheduler_content_moderation_integration_entity_base_field_info()? I'm open for ideas about setting it to default for people who have the module already installed.

Let me know if I'm going crazy or I have deviated too far. For now will keep my current idea of setting the custom widget as default, and the mini-tasks @gambry talked about in #79.

As a last note, I have wrapped the execution of entity:publish_action:node in a conditional checking if content_moderation is not enabled and at first glance, we don't have any more repeating revisions upon publishing. scheduler_content_moderation_integration does have a dependency on the core module, so perhaps use our module instead in the conditional?

As a last note, I have wrapped the execution of entity:publish_action:node in a conditional checking if content_moderation is not enabled and at first glance, we don't have any more repeating revisions upon publishing.

TBH it should work - as not repeating revisions upon publishing - without the condition. If content_moderation is enable it overrides publish/unpublish actions for moderated entities. Can you try without the condition and see?

scheduler_content_moderation_integration does have a dependency on the core module, so perhaps use our module instead in the conditional?

As said above the condition shouldn't be needed, but if it is I suggest we use scheduler_content_moderation_integration.

About all the other points, I think this issue has produce an amazing work thanks to everyone. Additional optimisation will be easier when this is in and used by developers and site-builders. Guessing now what the best solution is for a particular scenario is a bit too early.

I suggest we figure out if we need this condition or not for avoiding the duplicated revisions and we RTBC.
(and if you @Postovan want to add the default widget fine too!)

Thanks!

Hi,

I am testing patch from comment #71, it was working good, but as an administrator I saved an archived article (going to draft) without scheduling date, Mysql stopped.

I had to restart the container.

I am in sprint room 103 at the Drupaldevdays. Is anyone here so I can show it?

Sorry for the noise, I can't reproduce the error from comment 83.

I'm testing this patch at the moment. One thing that stumped me a bit was that the validation of transitions doesn't seem to take the current user into account and whether they would be allowed to publish from the current state.

Let's say an author can edit drafts, but can't publish them. An editor can publish, so the transition draft>publish exists and is therefore valid. The author can still schedule publishing with this, though, essentially bypassing what they're allowed to do in that context. I know I can take the permission for scheduling off altogether for authors, but in more complex workflows they might be allowed to publish from some states but not others.

Would it make sense to only allow scheduling in the context that the current user would also be allowed to publish immediately?

@nicolash, yeah, you're right, I didn't spot any current user consideration, basically we're jumping over permissions here. I'll add it to my TODO list. Thank you very much for this observation!

I've added this to the IS Remaining Tasks, together with the investigation for the publish/unpublish action.

Would it make sense to only allow scheduling in the context that the current user would also be allowed to publish immediately?

TBH I remember access related role is respected when building the dropdown with the state. But if you guys checked then this should probably be a blocker for this to get in.

Thanks!

Great, glad I wasn't missing something :)

The check in scheduler_content_moderation_integration module seems to only validate whether the transition exists, without taking the user into account:

private function isValidTransition($moderation_state, ContentEntityInterface $entity) {
    $valid_transitions = $this->stateTransitionValidation
      ->getValidTransitions($entity, $this->account);

    foreach ($valid_transitions as $valid_transition) {
      if ($moderation_state === $valid_transition->to()->id()) {
        return TRUE;
      }
    }

    return FALSE;
  }

So maybe using something like the content_moderation module validation that checks against the current user AND the states that they can transition to from the current context could just be re-used?

public function getValidTransitions(ContentEntityInterface $entity, AccountInterface $user) {
    $workflow = $this->moderationInfo->getWorkflowForEntity($entity);
    $current_state = $entity->moderation_state->value ? $workflow->getTypePlugin()->getState($entity->moderation_state->value) : $workflow->getTypePlugin()->getInitialState($entity);

    return array_filter($current_state->getTransitions(), function (Transition $transition) use ($workflow, $user) {
      return $user->hasPermission('use ' . $workflow->id() . ' transition ' . $transition->id());
    });
  }

And from a UX perspective it would be good to not present the option to schedule publishing on the entity form if the user can't actually do it. That could happen in the form_alter? Maybe not removing it entirely, but leave a clarifying message?

function scheduler_content_moderation_integration_form_node_form_alter(&$form, FormStateInterface $form_state) {
...

Happy to have a go with this if you guys agree conceptually...

Hey @NicolasH , on #37 I mention re-using functionality already provided by content_moderation would be ideal, so feel free to go ahead and exploring your thoughts on #89.
I suppose @Postovan is working on this as well, so you may find a bit of clashes somewhere at some point.

I suggest we keep a "only-small-changes" approach in order to fix bugs and having this committed, rather than adding new functionality now. So for example better to add the user access validation and the message if no states are available, and leaving the content_moderation widget and constraint extension for a follow-up when this work is committed in.

Thanks!

Just wanted to say thanks to you all for working on this.

Jonathan

Kinda overwhelmed with work, although it's just a couple of lines which need to be added, I'm still struggling to find some spare time. I will however tend to the changes required in the nearest time possible.

Sorry for the inconvenience.

@Postovan that's fine.
I've done some experiment myself to speed up the investigation process.

Patch from #71 breaks a test, so can't be accepted. The intent in there - suggested by #70 is OK, we need to polish the logic.

Attached patch extends #63 just adding the test coverage for double-revision issue discovered from #65. It's supposed to fail on testing the revisions increment by 1 only.
Settings Needs Review only to trigger the testbot.

Also on #82 I said something incorrect - sorry about that.
Error 1) The action id is indeed 'node_publish_action', and not 'entity:publish_action:node'. That's the derivative ID.
Error 2) If content_moderation is enable alter indeed publish/unpublish actions, but only the access() method, not the execution.

As suggested above the best option is currently to check if scheduler_content_moderation_integration is enabled.
Updating the IS remaining task.

The whole SchedulerManager service needs a bit of love. It currently does save things twice anyway, with or without moderated entities, which is not really performant.But this a really minor issue and should be fixed upstream in a separated issue.

This fixes the multiple creations of revisions.

I also added some logic to check which transitions are available for the user. This breaks some tests. I will take a look at them tomorrow.

Here is a patch to fix the last failing tests

Looks quite nice. Really a lot of work looks to be spent here. It does work for me, and in fact it works better than the main module. The main module actually creates 2 revisions, 3 if the " Create a new revision on publishing" option is enabled.

Really good work! I was already happy with #96. Just two notes:

+++ b/scheduler_content_moderation_integration/tests/src/Kernel/UnPublishedStateConstraintTest.php
@@ -23,8 +39,7 @@ class UnPublishedStateConstraintTest extends SchedulerContentModerationTestBase
-      'moderation_state' => 'draft',
-      'publish_state' => 'published',
+      'moderation_state' => 'published',

Why have these been removed? Don't they change the meaning of the test?
That's supposed to be a common scenario where a node is still in draft, but has a publish_state and unpublish_state scheduled?

Also I noticed our field widget "scheduler_moderation" applies to any "list_string" field_types?
If that's correct we should limit to where it applies by implementing SchedulerModerationWidget::isApplicable()?

Hi @gambry!

You're right, I made some checks, seems like the widget is available everywhere if the field type is a "list_string".

I've meddled around and came up with this code:

  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    if ($field_definition instanceof BaseFieldDefinition) {
      if ($field_definition->getProvider() === 'scheduler_content_moderation_integration') {
        return TRUE;
      }
    }
  }

Don't have time to provide a patch, so if anyone can pick this up - I will be really grateful.

Basically, we check if it's a base field first and then we check who's providing the field. Works on my end, if anyone thinks it should be done differently, let me know.

@gambry, you are right. It was because I limited the widget to only show valid transitions from the current state. But I agree, it's a valid use case to schedule publishing and unpublishing right from the beginning.
So I changed the widget to show all states which are allowed for the user to change in and added some logic to the SchedulerManager to check if the state change is valid.

@Postovan Dumitru, thanks for the snippet. I added it to the FieldWidget

Also fixed some coding style issues.

I had an issue with the above patches, which I am not able to reproduce with the default scheduler without moderation. I have the very simplest of workflows - the default that comes with content moderation: Draft -> Published -> Archived.

If I update a content and make the new revision Published, and at the same time schedule an unpublish (Archive) for later, things work as expected at first. But later I want to revert to my previously published revision, and this schedules the unpublish (Archive) action again:

I have made changes the the revisions tab to show the state, but I have not otherwise changed its behavior. The revert makes a new revision that is Published, which is a copy of the previous Published one, but that revision had the schedule field containing the scheduling of unpublish (Archive), and that is why the content is re-scheduled for unpublish (Archive).

Now I am not sure I should make it "Needs work", maybe this is not in scope for this ticket, and can be fixed in follow-up.

@miroslavbanov Hi! Thanks for the info!

When you're reverting some content, you're expecting the old value to be in the fields as well. This is what we're getting with the reverting of scheduled content. Although this is expected to happen will all fields, perhaps we should provide an exception for the scheduled revisions.

What if we unset the date and state in the content base fields when it's being processed by the SchedulerManager? This way, we won't have any values in the fields for the old revisions and thus reverting them will not change their states on a new cron run. I'm open for suggestions and for somebody to pick this up. Otherwise I might dig into it but it's not gonna be earlier than on Monday evening.

@Postovan this might work.

I am more in favor of handling it in follow up task, and pushing this task to be fixed sooner.

This has been an amazing work, thank you very much everyone.

I must say I haven't re-tested #102 manually, so probably the maintainer should have a quick play around before committing. But from the changes I can see, and by re-looking at the patch it looks RTBC for me.

RE #103 and any new issue, they should really be resolved in follow-ups.
The sooner we get this patch in, the better we can work on fixing outstanding minor bugs and adding new issues.

Thanks again!

RTBC +1.

I am currently using #102. Other than some improvements that can done in follow-up, the patch works great.

Regarding the change in #102

It's difficult to reason about what happens when the transition is not allowed. The transition is not allowed but the node is still saved, but in the wrong state? I would rather that there is no validation in that case, or there is a way to disable it.

Also regarding the patch in general, I think it would be good if we automatically generate a revision log message.

So after a bit of testing I've found an issue with this patch, after enabling the module it assumes that every content type is under content moderation. This forces you to select a transition from an empty list on content type not under moderation and preventing you from saving a node. There should probably be an option at the node config level or a check run to see if the node requires a transition setting.

The patch in #102 does seems to work nicely.

However there is one behaviour that might not be logical?

For the published states, only workflows that have the 'default revision' flag and are set as 'published' are available.

However for the unpublished states, all states are available that are set as 'unpublished', also states that are not set without 'default revision' flag.

For example, this allows the content moderator to schedule a change from 'published' to 'draft'. This causes confusion because the moderator expects the content to be no longer available after the scheduled date.
This is not the case however, because a new (not default) draft revision is added, while the original content is still available. While technically correct this behaviour is confusing for moderators.

I think it would make more sense to only allow depublish transitions to workflow states that are set with 'default revision' flag.

Hello,

We have discussed the issue in #109, and providing the possibility to define this functionality per content type basis is on a todo list.

However, the #110 is something perhaps overlooked initially when this was developed. Since it creates a revision instead of actually unpublishing the content, means we're not doing this process right.

@gambry, do we move this to needs work or do we wait for the maintainers to have a look first? Perhaps they can also decide about the severity of the above 2 comments.

Hi all, thank you for this work here, #102 solved a big problem on my side.

However my use-case is a bit different, that is not handled with the patch.
My workflow here is, that an editor may create and edit articles and must pass it to an admin who approves the article. Only Admins may publish articles, but the editor may (and should) enter a publish date.

The current function _scheduler_content_moderation_integration_states_values checks whether the editing user is allowed to use a publish transition and only returns these states. This leaves the 'publish_state' empty and the article can't be save if an published date is entered.

scheduler_content_moderation_integration.module lines 128ff:

 $user = \Drupal::currentUser();
  $user_transitions = array_filter($type_plugin->getTransitions(), function (Transition $transition) use ($workflow, $user) {
    return $user->hasPermission('use ' . $workflow->id() . ' transition ' . $transition->id());
  });
  foreach ($user_transitions as $transition) {
    /** @var \Drupal\content_moderation\ContentModerationState $state */
    $state = $transition->to();
    if ($state->isDefaultRevisionState() && ($definition->getName() === 'publish_state' && $state->isPublishedState()) || ($definition->getName() === 'unpublish_state' && !$state->isPublishedState())) {
      $options[$state->id()] = $state->label();
    }
  }

For this use case I modified the function so that all published moderation states are returned, regardless of the user's rights to use this transition. I think both approaches are mutually exclusive. So maybe there is some configuration needed to decide which option generation is needed.

One more question arose: shouldn't the scheduler setting the configured moderation_state instead of just setting the published flag when the node has to be published?
Like this:

diff --git a/scheduler.module b/scheduler.module
index 9f86b4d..06086b6 100644
--- a/scheduler.module
+++ b/scheduler.module
@@ -361,7 +361,7 @@ function scheduler_node_presave(EntityInterface $node) {
         $node->setCreatedTime($node->publish_on->value);
       }
       $node->publish_on->value = NULL;
-      $node->setPublished(TRUE);
+      $node->set('moderation_state', $node->publish_state->getValue());
 
       // Trigger the PUBLISH_IMMEDIATELY event so that modules can react after
       // the node has been published.

I have to agree with mmbk in #112 and #113. I have a similar workflow that does not immediately go from unpublished to published. The patch in #102 _kind of_ works, but it's limiting in that it only seems to allow published or unpublished states and if we try going from unpublished to unpublished, then we get an error saying that the scheduled moderation is not allowed.

IMHO only users allowed to transition to the state(s) should be able to schedule the publishing. And that's because if we split the worfklows, i.e. "Some users can set the destination state, while others can set the dates" will only creates a lot of confusion.
Confusion not only for us writing the logic, but for the administrators too as one may set the date, the other sets the state but the date is now passed and the node may never be published. And this is just the first scenario I can think of.

The solution adopted on #112:

For this use case I modified the function so that all published moderation states are returned, regardless of the user's rights to use this transition.

It's dangerous. If a role doesn't have permission to transition and we do from this contrib we break the promise made by the permission settings. It may work for some workflows, but than I question why an editor can set the transition through scheduler, but not from the content_moderation widget? What if the editor sends an important content for approval, but sets the scheduler widget to publish immediately?
The content will be published without approval.

The Revision Log field (or any field you can manually add) is a better choice for communicating in-between editorial workflows, and a workaround for the issue in #112 and #114 from my understanding?
It's a business requirement, and it should agreed and kept in between the business.

Instead I see #109 as a serious bug, because it prevents saving nodes for content types not in a workflow, if I understand correctly?

My suggestions is if #109 is a real bug we should set this to Needs Work and fix it, and leave #110, #112 and #113 to followups, just because they will be much much much easier to tackle when this is in.

Setting "Needs review" for getting feedback and validating if #109 is a blocker.

I'm marking this back to "Needs work" because the implementation limits the workflow settings and moderation states/transitions. I'm currently working on a patch building off of #102.

I don't think that scheduling should be implied from the beginning of a workflow and that the scheduling should be left to the people who approve content for publishing so my work will instead only allow transitions that are available from the current state and only if the current user has the permission to set it.

Once I test and make sure that things are working the way they should I will upload the patch and interdiff, but I am making efforts

@doficus that is great!

After reading a bit more of the comments, I think it would make more sense to configure the available published and unpublished states manually instead of trying to deduce them from workflow logic?
There are many use cases in which you can not easily deduce the correct states.

For example if there is an 'archived state', that is both published and the default revision, logic would list this under the available publish states. However depending on the site's architecture it might feel more logical to actually list this under unpublish states.
So I think it would be better to use the site builders judgement to configure the available publish and ubpublish states.

Now that I am writing this, maybe it is even better to not assume to default publish and unpublish transitions? If you could just schedule one, two, three or more transitions without labelling them it would allow a lot of flexibility.

So, for example you could schedule a node to be published on a certain date, moved to an archive state a year later and to unpublish it after two years.
I guess the challenge here would be that you can not just randomly schedule some transitions, they would have to follow the workflow logic for the allowed transitions.

@K3vin_nl I think that should fall on the site builder in the aspect of creating their own states or editing default states. If they don't want "archived" to be published then they can turn off the Published setting in admin/config/workflow/workflows/manage//state/archived. Trying to create multiple scheduling states seems like much outside the scope of this issue and may be better suited as a new feature request.

I agree with @dorficus . Although I understand @K3vin_nl , not following the workflow and the transitions permissions can facilitate the editors BUT have huge security implications.

I'm marking this back to "Needs work" because the implementation limits the workflow settings and moderation states/transitions

@dorficus do you mean the bug flagged by #109?
We are doing this sprint in UK next week, so let me know if you need help.

I've gotten this working for moderation without requiring it. At a high level, if the current state allows for a publishing state to be next and the user has appropriate permissions, then the publishing state can be selected. If not, "none" is a valid option that will prevent it from being published or scheduled.

Creating the transitions and states falls on the site builders for this patch because we can't come up with every possible scenario, but our workflow is something like Draft > Ready for Review > Ready for Publishing || Draft > Scheduled for Publishing || (Published > Archived) > Published > Archived.

I would appreciate more testing and feedback, but this is definitely more in line with the original ask.

Crud, left some client specific stuff in there.

Let's try that again.

I have fixed an error with PublishStateConstraintValidator, please see updated patch.

@yingtho can you upload with an interdiff so we don't need to sift through 50k of changes, please? https://www.drupal.org/documentation/git/interdiff for reference

I'm starting to look at this as a requirement for one of my projects. I very much like the direction in #102 but think the issue with it breaking scheduled publication of non-moderated node types would have to be resolved before this could be accepted.

I agree with @gambry's comments in #115 that scheduled publishing should not be allowed by users that do not have permission to publish content.

@dorficus with the approach in #123 it looks like content types that support scheduled publishing but are not part of a workflow will still have published/unpublished state select controls, but add the option to select -None-. Can I suggest it would be better UX to just not have these controls if the user cannot do anything with them? Apologies if this was considered but wasn't feasible.

#123 also adds the - None - option when a content type *is* subject to moderation, so we definitely shouldn't allow that.

And if you change the form settings to use Check Boxes/Radio Buttons via admin/structure/types/manage//form-display, you get both N/A and - None -.

#126 and #115

Regarding the user needing to have permission to set content to a particular state.

Unfortunately the existing workflow permissions are about setting particular Transition from State A to State B. Unfortunately, you don't know what the transition will be in advance, because you don't know what will be the "From state" when the scheduled transition will happen on cron. You can't assume that the state won't change in the meanwhile. In fact it is a most common workflow to have an unpublished content, and to schedule publish at a one date, and unpublish at a later date at the same time. So that assumption flies out the window. But currently everything is allowed to be set, but if the transition doesn't exist, the end result will be unexpected - the content is still saved but in the wrong state, and there is an error logged. So with all that in mind, I would remove the validation about whether the transition exists or not, and create an entirely separate set of permissions for setting of a state(s) through scheduler, that is only validated on submit.

@MiroslavBanov you can't guarantee what State A will be at the scheduled time, but it's probably going to be the state you left it in. For unpublishing, you again can't guarantee what the state will be at the scheduled unpublish time, but if you are setting a scheduled published state at the same time, then State A at unpublish time will probably be the State B in the publish transition. If you are not setting a scheduled published state at the same time, then State A at unpublish time will probably be the current state of the node.

If users are allowed to schedule a publishing event that they wouldn't be allowed to effect at the current state in time, that is no better than an aspiration. The system would be allowing them to set up an event in the future that without third party intervention is guaranteed to fail, without raising any kind of warning. That is unintuitive and may even lead users to believe the workflow has not been set up correctly.

Therefore it seems reasonable to me to check the permitted transitions based on the current state when working out what states to offer the user when setting publishing/unpublishing times, then raising an error if when the scheduled time is reached, the state of the node has changed and that transition would no longer be valid by the user that saved it.

This needs further consideration with respect to recording the user who scheduled the event so it can be checked at the event time, and how to deal with subsequent users saving the node before the scheduled time - do they then become responsible for the state change?

I also anticipate further discussion on this as others have stated they have workflows that would not be supported by this set of assumptions. Therefore it would be awesome to hear from the module maintainers on this point before a lot more work is done!

@Eli-T

Well I was thinking having something straightforward and easy to reason about, that's why I suggested what I suggested.

@MiroslavBanov I was trying to make it simpler to reason about, maybe I was over-verbose :)

In a nutshell:

1. Only let people schedule things for the future they would be able to do at the current time
2. If in the meantime, circumstances change so they wouldn't be able to take that action, don't make any changes

@Eli-T: There may be a misunderstanding or a comprehension error on my part, but there is logic in the new sub-module that only allows transitions that are available to the user that is saving the entity at the time.

  /** @var \Drupal\content_moderation\Plugin\WorkflowType\ContentModerationInterface $type_plugin */
  $type_plugin = $workflow->getTypePlugin();
  $user = \Drupal::currentUser();
  $user_transitions = array_filter($type_plugin->getTransitions(), function (Transition $transition) use ($workflow, $user) {
    return $user->hasPermission('use ' . $workflow->id() . ' transition ' . $transition->id());
  });

However, I can remove the None value from the scheduling moderation in the event that there are valid states, but that seems like we're enforcing that scheduling MUST occur. That kind of governance is not for us to determine, in my opinion. That should be left to the admins and maintainers of the sites using this module. As far as I can tell, having the None option in the sub module has little to no effect on the core moderation form.

As far as changing the form display, there's a reason we have a custom field widget.

I've included another patch + interdiff here from #121 - current to fix a couple of things i noticed that were borking my travis build.

I've been testing this patch, and I cant rule out that it's some other functionality on the site I'm working on, but this patch seems to assume that all content will be managed with content moderation. In our use case, we have content that is part of a workflow, as well as much simpler content that doesn't use or really need a workflow. It would be awesome if this would take a mixed use case into account.

So I worked on a patch based on #102

This patch includes:
- Scheduler still works with non-moderated content types
- Hide scheduler date widget if no valid states to move in are available
- If immediate scheduling is enabled, set the moderation state value
- It's only possible to select default states in the unpublished state widget
- Extend the tests

The tests are failing because they are still running on Drupal 8.5. The module maintainer should change the setting to use the latest version of Drupal for patch testing.

FYI you can manually queue a new test scenario, e.g.: https://www.drupal.org/pift-ci-job/1133158

The tests with PHP 7.1 and MySQL 5.5 against core 8.7.x passed a-ok.

Hi all,
Thank you for the effort to integrate this with Core Moderation.

Quick support request,

I want to publish node only if Moderation State of (Publish or Schedule Publish) is selected. If it's still sitting on "Needs Review" or "Draft" even if the scheduled publish date passes, I don't want it published.

How do I do that?

Note: My states are standard states like: Draft, Needs Review, Scheduled Publish, Publish, Archive.

We have tested this patch and it works as advertised.

it doesn't work when there are multilingual translations.

Here is my test case:

  Scenario: Basic page - new content revision on exising / published content.
    Given I am logged in as a user with the "administrator" role
    # Add English basic page
    Given I am on "/en/node/add/page"
    And I fill in "Title" with "Test EN"
    And I click on the text "Scheduling options"
    And I wait for the text "Publish on"
    And I fill in the last datetime field "Publish on" with "+10seconds"
    And I press "Save" in the "content" region
    And I wait for the page to be loaded
    And I should see "This post is unpublished and will be published"

    # Add French translation,
    And I click "Translate" in the "content" region
    And I click "Add" in the "content" region
    And I fill in "Titre" with "Test FR"
    And I click on the text "Scheduling options"
    And I wait for the text "Publish on"
    And I fill in the last datetime field "Publish on" with "+10seconds"
    And I press "Save (this translation)" in the "content" region
    And I wait for the page to be loaded
    And I should see "This post is unpublished and will be published"

    # Test scheduling (for newly added content)
    And I wait for "11" seconds
    # For some reason the drupal driver's cron doesn't publish, but drush cron works.
    And I run drush "cron"
    # Now EN / FR should have been published.
    And I break

    # Try schedule new version
    And I am on "/en/test-en"
    And I click "Edit" in the "content" region
    And I fill in "Title" with "Test EN v2"
    And I click on the text "Scheduling options"
    And I wait for the text "Publish on"
    And I fill in the last datetime field "Publish on" with "+10seconds"
    And I select "draft" from "Change to"
    And I press "Save"

    And I am on "/fr/test-fr"
    And I click "Edit" in the "content" region
    And I fill in "Titre" with "Test FR v2"
    And I click on the text "Scheduling options"
    And I wait for the text "Publish on"
    And I fill in the last datetime field "Publish on" with "+10seconds"
    And I select "draft" from "Change to"
    And I press "Save"

    # Make sure the drafts are not published yet
    And I am on "/en/test-en"
    #And I break
    And I should not see "Test EN v2"
    And I am on "/fr/test-fr"
    And I should not see "Test FR v2"

    # Test scheduling (for new revisions)
    And I am an anonymous user
    And I wait for "10" seconds
    And I run drush "cron"

    And I am on "/fr/test-fr"
    #And I break
    And I should see "Test FR v2"
    And I am on "/en/test-en"
    And I should see "Test EN v2"
    # ^^^ The above line failed, only French v2 is published, not English version.

It's almost like a core bug, that when you publish a French revision it is linked to an older English revision (the published English revision when the French draft is created).

I have a patch almost completely rewrote this module's core logic. Also only did publishing, not unpublishing, and needs a lot of polishing. But it passed this test case.

btw my patch is a merge of #136 (content moderation) and the change I made to publishing logic.

@yang_yi_cn, as I see your patch from #142 is almost two times smaller than #136.

Can you please re-apply your patch on top of #136 and please provide interdiff. It's way easier to review changes when interdiff is provided.

@yang_yi_cn
I suspect that scheduling of multilingual nodes won't work as expected even without content moderation.

interdiff is here

Also, RE: #145 from MiroslavBanov:

scheduling of multilingual nodes won't work as expected even without content moderation.

You are right it wasn't quite working, but seems to be working in some conditions, like first publish (instead of update) or when you scheduling EN/ FR in different times to publish. I believe it also depends on if you selected the option "create new revision", as the behaviour of translation + revision makes things more complicated.

Enabling "content moderation" makes the issue more obvious, as I believe the moderate state change will force a new revision and surface the issue. And it kind of makes the "create new revision" checkbox useless.

* Fix issues with users not having sufficient permissions, see #3018530: WSOD on scheduled nodes after saving.
* Add _none to allowed values and check for times in constraint validators.

fix tests.

I am still having an issue.

I do get "invalid choice" errors when I try to edit or create a node when the node type is configured to have scheduling information but the current user isn't allowed to schedule it.

IMO the access checks in function scheduler_content_moderation_integration_form_node_form_alter() need to be appended with:

&& \Drupal::currentUser()->hasPermission('schedule publishing of nodes');

The last comment can be disregarded.

A proposal for better UX:
- if "Publish state" dropdown is empty - do not show it and do not show "Publish on" field
- same for the "Unpublish state" dropdown
- if both dropdowns are empty, do not show "Scheduling options" fieldset at all

Sounds good?

Hello everyone, thanks for this great improvement of the scheduler module! We are relying heavily on this in the Thunder distribution, but maintaining this patch is quite hard.
As long as this does not get merged into the scheduler module, fixing bugs within the patch is quite hard to do, also new features and proposals for better UX - as the ones from Leksat - cannot be handled in a good fashion as long as we are still working with a big fat patch.
And the bigger and more featureful the patch gets, the less likely it will be, that we get it merged into the scheduler module.
To improve the situation I propose to use a new project for the submodule. I already created it: https://www.drupal.org/project/scheduler_content_moderation_integration and moved all the code from the last patch there. We can do the improvements for the integration there. Then we can solve remaining issues with proper separate patches.
We still need a patch for the scheduler module but that is very small and just contains the changes in the current scheduler codebase. I also extracted the necessary code for this and will attach it here.
If you think this is reasonable, then please use the new project for future improvements instead of changing the patch here!

Thank you, Daniel, for taking action.
I think it makes a lot of sense to improve the content moderation integration in the new repository and I encourage everyone to follow Daniels approach.

Leksat, I created a new issue for your UX proposal #3024715: Hide scheduling state options if there are no valid transitions

Hello all,
First off, thank you to everyone who has been contributing to this feature, I appreciate your work. Secondly, I apologise for not being very active here over the last few months, and not keeping up with commiting changes etc. It does seem sensible to move the develoment of the sub-module to a separate project, and I have no problem with that.

I will follow all the progress over on project/scheduler_content_moderation_integration

Jonathan
(Scheduler maintainer)

Thanks Jonathan!

Btw. I do not really like the patch, that I posted, this is a too tight coupling to the new module. We should come up with a better solution, that does not have the "scheduler_content_moderation_integration" name used in all the places.
For now, it just works, but i will try to come up with a better integration.

@yang_yi_cn: could you please create a new issue with your patch in the new modules issue cue? That would be awesome!

Hi @daniel.bosen,

I agree with a lot of what you say in #160, I've seen the same happen to og (which is a lot more complicated): resulting in different PR's that don't get closed and require each other and a lot of rerolls.

Instead of creating another module can't we create a new version 2.x-dev and commit the latest patch in there?

That way:
- we can collaborate incrementally on 2.x
- people can decide to use 2.x-dev at their own risk
- 2.x will eventually support content_moderation

I'd like to propose the third option: commit #153 as is to 8.x-1.x. Because the code looks good, tests are included, and the patch works in general.

I'd be happy with that too.

Sure, merging the big patch into scheduler would be the original idea. I just had the feeling, that this is less and less likely to happen, that's why I did the split. But that is Jonathan's call.

Thanks @mpp and @Leksat for those alternative suggestions. However I still prefer the idea from daniel.bosen to have the separate sub-module. This will mean that you can continue development and commit and make progress without needing my input directly. Also there is an advantage because for sites that do not use Content Moderation the Scheduler codebase is not unnecessarily increased. I do not have the capacity to maintain a 1.x and a 2.x version of Scheduler. A separate submodule project is my preference.

Jonathan