Scheduler fails if publish_state or unpublish_state are empty

Can anyone confirm this issue in 2.x please.

Following up if anyone has checked 2.x? If not there we cna close.

I cannot replicate this in 2.x. If the state if left as 'None' we get a validation warning

and

But both of the reports were from 1.x so maybe I will try that too. It really would help those of use who give our time to support and maintain issue, if when we ask for more details from the original poster, they actually gave feedback and helped us. There could be a problem here, but unless we can replicate it we will not be making any code changes.

I can't replicate this in 8.x-1.4 either. Same validation errors

I'm running into this issue running Drupal v9.5.8 and SCMI v2.0.0-beta1.

1. The test is done with a user that has no publishing rights. User creates record, saved with status of Draft, a Publish On value, but no Publish Status.
2. When the cron job is executed:
   • The content is updated with a new revision with the message: `Published by Scheduler. The scheduled publishing date was 10/09/2023 - 10:10. The previous creation date was 10/09/2023 - 10:08, now updated to match the publishing date. (Draft)`
   • The cron job logs the following error: `Publishing failed for [TEST] Scheduled Page - 1. scheduler_content_moderation_integration_scheduler_publish_process returned a failure code.`
3. Because the new revision is saved with the same scheduled time, the issue will repeat the next time the cron job is executed. In our case this has resulted in hundreds of revisions getting created and going unnoticed because the status was Draft and not meant to be reviewed.

In our case our intention was to allow users that don't have publishing rights to "Schedule" a time with the idea of helping signal when a content is to be scheduled for the reviewer (since the tab will be displayed when there's a time set). I'm (97%) sure I tested this case a year ago without this issue. I'm not sure when it started happening.

Thanks!

I had experienced a similar issue to #17 where it was creating multiple revisions, except I had set the published state and unpublished states. The issue was caused by my workflow settings where I had a 'Ready to publish' status that could not change to 'Archived'. It was set to only allow 'Draft' and 'Published' to go to 'Archived'.

Although this issue is caused by my configuration, it would have been helpful to have seen an error message in the logs to help me identify the issue. Is it possible to log the exception message within the catch of the try/catch statements to help identify why it failed? Thanks.

catch (\InvalidArgumentException $exception) {
  Drupal::logger('scheduler_content_moderation_integration')->error($exception->getMessage());
  // If transition is not valid, throw exception.
  return -1;
}

cballenar → closed merge request !52

I was also looking at a similar option as the one described by @chrisrhymes we could take his recommendation a step further and pass this value to the revision log of the entity.

What's happening now is that even though the scheduler_content_moderation_integration_scheduler_publish_process hook returns '-1' for invalid transitions, the scheduler publish/unpublish function maintains the revision log it had before which assumes the process was completed successfully. Resulting in the messages previously mentioned.

With the change below we instead get an accurate message: "The transition from 'draft' to '' does not exist in workflow. (Draft)"

function scheduler_content_moderation_integration_scheduler_publish_process(&$entity) {
  ...
  catch (\InvalidArgumentException $exception) {
    $entity->setRevisionLogMessage($exception->getMessage());
    return -1;
  }

Now in the example above, this exception is thrown due to an empty state; to me, this handling is also a problem because it will result in recurring messages of this type until the record is corrected, if the record happens to fall on a state that is not being watched (say Draft) then this could easily turn into hundreds of revisions.

Instead, we could treat a record with no target state as 'invalid' and remove it from the list of records to be processed by Scheduler, as follows:

/**
 * Implements hook_scheduler_list_alter().
 *
 * This hook is called from schedulerManger::publish() and
 * schedulerManger::unpublish(). It passes the list of entity ids to be
 * processed, the process (publish or unpublish) and the entity type id.
 */
function scheduler_content_moderation_integration_scheduler_list_alter(&$ids, $process, $entityTypeId)
{
  /** @var \Drupal\content_moderation\ModerationInformationInterface $moderation_information */
  $moderation_information = \Drupal::service('content_moderation.moderation_information');

  // Fetch all entities and check if they have a moderation state.
  // This will prevent the processing of 'incomplete' entities by Scheduler.
  $entities = \Drupal::entityTypeManager()->getStorage($entityTypeId)->loadMultiple($ids);
  foreach ($entities as $entity) {
    // Check if entity is moderated and has no target state
    $is_moderated = $moderation_information->isModeratedEntity($entity);
    $has_no_target_state = !($process === 'publish' ? $entity->publish_state->value : $entity->unpublish_state->value);
    if ($is_moderated && $has_no_target_state) {
      // Find the key of the entity in the list of ids and remove it
      $entityKey = array_search($entity->id(), $ids);
      if ($entityKey !== false) {
          // unset($ids[$entityKey]);
      }
    }
  }
}

I've attached a patch containing both solutions since they cover different situations.

Ran into cases where instead of null, '_none' is returned. The update here addresses both cases and improves readability.

Fixes will need to be against 3.0.x

Create a MR against 3.0.x based on the patch in #22.

Also hiding the patches so that work can continue in the MR.

Still needs work as tests are failing.

If you have entities with publish state's that are unintentionally null or none, then those are actually errors that need to be fixed. With this patch, scheduling just fails silently. I know 100s of logged messages isn't ideal, but how would we distinguish between

@cballenar What permission configuration gets "a user that has no publishing rights. User creates record, saved with status of Draft, a Publish On value, but no Publish Status"?