Created on 31 May 2023, almost 2 years ago

I see this integrates with Rules - just wondering if there was any work in progress or any discussion on integrating with the newer ECA module?

๐Ÿ’ฌ Support request
Status

Active

Version

2.0

Component

Code

Created by

๐Ÿ‡บ๐Ÿ‡ธUnited States w01f

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @w01f
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom jonathan1055

    No discussion yet and no work in progress.

  • ๐Ÿ‡น๐Ÿ‡ทTurkey orkut murat yฤฑlmaz Istanbul

    It'd be great, if scheduler can provide integration for ECA.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom jonathan1055

    Hi,
    Can you give a link to ECA project, and suggest what type of integration would be useful?

  • ๐Ÿ‡น๐Ÿ‡ทTurkey orkut murat yฤฑlmaz Istanbul

    It would be so nice to define some event and action plugins of scheduler, for ECA, as seen on this source.

    To schedule of publishing an entity, would be better, if we can add some conditions for publishing too.

  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand heiket

    Yes :) that would be amazing! It is very useful to send notifications to editorial team via ECA when content is scheduled for publication or 'unpublication'. (similar to the rules integration.)

  • ๐Ÿ‡ฑ๐Ÿ‡ฐSri Lanka lakhithaD

    It would be really nice to see an scheduler integration to ECA ( https://www.drupal.org/project/eca โ†’ ) as many projects such as webform, commerce, flag, etc... supports integration to ECA. ECA is the future of Rules.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    I was actually looking into the rules integration of scheduler and I can't find anything that can't already be done with ECA. I can't see what extra features would be required to make scheduler and ECA work together.

    Can anyone tell me that I'm wrong? I'd be interested in getting this sorted, as many ECA users have asked for it.

    In other words, what feature are you expecting from an ECA/Scheduler integration?

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

    Thanks, Jรผrgen, for your thoughts on this, and for all the work your work on ECA and integrating submodules!

    Scheduler module comes with the following Rules events (it also comes with a few actions, but I'm only interested in the events since ECA can do those actions):

    • After a node has been published by Scheduler
    • After a node has been unpublished by Scheduler
    • After saving new content that is scheduled for publishing
    • After saving new content that is scheduled for unpublishing
    • After updating existing content that is scheduled for publishing
    • After updating existing content that is scheduled for unpublishing

    To paraphrase Jรผrgen from Drupal Slack #eca channel, Scheduler's Rules integration doesn't actually do what it says and simply acts on a generic entity updates. I think there is more to it, otherwise the Scheduler Rules integration's events seem to be bogus (and yet they work somehow to trigger Rules). I have looked at the code and see that events are being dispatched for the above list. Can ECA be made aware of these events?

    I tried and failed miserably to emulate the "After a node has been published by Scheduler" event in ECA, since there is no way to evaluate when Scheduler is involved in cron publishing the scheduled node (no event is dispatched that ECA can react to, which is what I hoped a Scheduler ECA integration would expose).

    Jรผrgen created an ECA publish/unpublish/scheduling model which could replace Scheduler module: https://ecaguide.org/library/simple/scheduled_publishing. The linked video us vert helpful in that it exposes Jรผrgen's thought process while creating a model of this nature: https://tube.tchncs.de/w/5cs5Du3579Nwau1m9Rv92G

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    I've had another thought on this: all the events would be a duplication of what ECA already does, i.e. subscribing to entity update events.

    A much simpler approach would be to provide a condition plugin in scheduler that tells us if the last update to the given entity was performed by scheduler or not. With that in place, we should be able to do everything I can find above.

    Any thoughts?

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

    @jurgenhaas Thanks very much for your time in thinking about this further. Yes, the only missing piece is knowing that Scheduler was responsible for the event. Would it be simpler to add a condition to Scheduler, or to create an ECA/Scheduler integration module?

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada bdunphy

    I would be very interested in better Scheduler integration. For the purposes of the ECA I've created, I've resorted to a series of essentially if-then-else type conditions.

    On entity save and validation that it's the proper node/entity that I want. Is Scheduled Open empty?

    • If yes, check if published. If yes, check if Scheduled Close exists. If yes - do a bunch of work. The was a scheduled publish.
    • If yes, check if published. If no, check if Scheduled Close empty. If yes - do a bunch of work. The was a scheduled unpublish.
    • If no, do some other work (housekeeping)

    This is a simplified version but you get the idea. Would be nice to have a definitive "Scheduler published/unpublished" type event.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    the only missing piece is knowing that Scheduler was responsible for the event. Would it be simpler to add a condition to Scheduler, or to create an ECA/Scheduler integration module?

    This could be a very simple ECA condition plugin which wouldn't do anything if ECA wasn't around. So, if the maintainers of the scheduler module would accept that, it would be best placed in this module. I'll provide an MR to show how simple that could be. That may help in making that decision.

    Would be nice to have a definitive "Scheduler published/unpublished" type event.

    Just for clarity, what I'm proposing is not a new event, it's just a condition that will tell the ECA model, if the latest update on an entity had been triggered by scheduler or not. All required events are already available in ECA and would be redundant if we added them here again.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    This was probably the simplest condition plugin that I've ever committed ;-)

    Please give it a try.

  • Pipeline finished with Failed
    25 days ago
    Total: 649s
    #436652
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada bdunphy

    Just for clarity, what I'm proposing is not a new event, it's just a condition that will tell the ECA model, if the latest update on an entity had been triggered by scheduler or not. All required events are already available in ECA and would be redundant if we added them here again.

    Works for me. As I review what I wrote and thinking about the ECA I've built, a condition makes sense. As you pointed out, required events already exist. I'll be giving your condition plugin a try today and report back.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

    Scheduler allows setting "publish" and/or "unpublish" seperately, so I believe we need separate conditions.

    I would suggest changing UpdateTriggeredByScheduler to PublishTriggeredByScheduler and adding an UnpublishTriggeredByScheduler condition plugin that checks !empty($entity->unpublish_on->value).

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

    Oops, that last commit throws a fatal
    ArgumentCountError: Too few arguments to function Drupal\scheduler\SchedulerManager::__construct()

  • Pipeline finished with Failed
    24 days ago
    Total: 637s
    #437187
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

    Actually, it looks like Jurgen's original MR fails phpstan.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

    I'm testing the MR and have found an issue with checking $entity->publish_on->value since Scheduler sets this to null when it's done publishing.

    This actually works in our favour, because this change is indicative of Scheduler acting on the entity.

    I've changed my model event to entity presave and changed the code to compare $entity->original->publish_on->value against $entity->publish_on->value, which works quite nicely.

    I'm also adding a check for the status changing from 0 to 1 to save an extra check in the model.

    Something like this ... could be more succinct, but I like stepping through my vars with breakpoints.

      public function evaluate(): bool {
        $entity = $this->getValueFromContext('entity');
        // Status is changing from unpublished to published
        // Scheduler is setting publish_on to null
        $publish_on_original = !empty($entity->original->publish_on->value);
        $publish_on = empty($entity->publish_on->value);
        $published_original = $entity->original->status->value;
        $published = $entity->status->value;
        $negation_check = $this->negationCheck($publish_on_original) && $this->negationCheck($publish_on) && !$published_original && $published;
        return $negation_check;
      }
    

    More testing, then I'll add to the MR for possible inclusion.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States SocialNicheGuru

    Could this be a submodule that depends on eca? Making the main scheduler module depend on eca seems more than is needed for a simple installation.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    @socialnicheguru by adding a condition plugin the scheduler won't depend on ECA. If ECA is not available, the plugin just doesn't do anything. Only once ECA would be enabled, the plugin becomes active. An extra module (or sub-module) just for a single plugin sounds overkill.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States SocialNicheGuru

    I just took a quick peak at the MR and saw:

     diff --git a/composer.json b/composer.json
    index f5f380a65bff59d87a62ee7ffb9928d1b317dd23..9ab54755ad21f72d612afe5a8dadcf28fb13277f 100644
    --- a/composer.json
    +++ b/composer.json
    @@ -10,7 +10,8 @@
             "drupal/devel_generate": ">=4",
             "drupal/workbench_moderation": "*",
             "drupal/workbench_moderation_actions": "*",
    -        "drupal/commerce": "^2 || ^3"
    +        "drupal/commerce": "^2 || ^3",
    +        "drupal/eca": ">=2"
         },
     

    Now I see that ECA is being added to require-dev not require.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada bdunphy

    I agree with @darkodev that we need two conditions for publish/unpublish.

    I have a very simplistic ECA model at the moment to test out the various scenarios that would mimic how we use scheduler. The code as found in the MRs doesn't behave as expected. I think part of this has to do with what @darkodev is working on in #20. Looking forward to testing more extensively once there is an update.

  • Pipeline finished with Failed
    22 days ago
    Total: 490s
    #438438
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    I've also added a verification that the entity type is supported by scheduler.

    The failing phpstan is an odd behaviour from GitLab MR pipelines that uses the composer.json of the target branch instead of the one from the MR, that's why ECA isn't installed for testing. When merging or when adding the require-dev to the target branch, the test will be OK.

  • Pipeline finished with Failed
    22 days ago
    Total: 777s
    #438444
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom jonathan1055

    This looks interesting, thank you for working on it. If the presence of the plugin has no effect when ECA is not installed then I'm ok with adding it here in the main module, no need for a sub-module. I've not used ECA but it seems to have support and as it is newer it does not have the backwards compatibility problems and long-standing core issues that Rules faces. The Scheduler Rules Integration sub-module was written before Scheduler 2.0 and hence also before Scheduler dispatched its own events. Therefore supporting ECA is a good addition here.

    1. From #24

    The failing phpstan is an odd behaviour from GitLab MR pipelines that uses the composer.json of the target branch instead of the one from the MR

    That's an interesting observation. Can you raise an issue in Gitlab Templates so we can investigate further, as that sounds like a bug which could/should be solved.

    2. It would be good to have a phpunit test to cover the new ECA conditions

    Setting to NW so we can discuss these two points.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

    I updated the MR with my proposed extra conditions that I think more accurately reflect what Scheduler does when it acts on a content entity, which is the following:

    1. Content entity is changing from unpublished to published (or vice versa)
    2. Entity's publish_on/unpublish_on setting is being removed after Scheduler processes it

    I am uploading two models to demonstrate the conditions.

    To test:

    Check out the latest MR version
    Run db updates if you had a previous version installed
    Import the two models attached
    Create some content with Scheduled publish and /or unpublish dates
    Run the Scheduler cron

    The result should be that Scheduler publishes/unpublishes the entity and that a log message is created by the appropriate model.

    Please test and provide feedback.

    A test is a great idea, so perhaps someone can contribute to that task when we agree on the approach.

  • Pipeline finished with Failed
    21 days ago
    Total: 669s
    #439160
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom jonathan1055
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom jonathan1055

    The composer.json problem is entirely a Scheduler fault which I am fixing in ๐Ÿ“Œ Remove tempoary D11 alternative composer.json Active

  • For the Scheduler content moderation integration sub-module we need a way to schedule the transitions.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada bdunphy

    I've had a chance to test @darkodev code in #27. The conditions work well for all my test scenarios. The only issue I found was that condition negation does not appear to ever fire. To be honest, I don't know if that is necessary.

    I've uploaded my very simple ECA that covers all conditions (publish, unpublish, negation on both).

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    I have reviewed the code and left some comments as that needs some more work.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

    Thanks for the feedback, @jurgenhaas. My bad for misunderstanding negationCheck. I'll try to find some time to integrate your comments.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

    As per @jurgenhaas' feedback, I refactored for proper use of negationCheck() and the unpublished plugin now extends the published plugin.

  • Pipeline finished with Failed
    10 days ago
    Total: 1030s
    #448871
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada bdunphy

    @darkodev - have tested and found the update and negation works as expected.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    I looked again as well. The code as such looks good to me. What I can't tell, if the 4 conditions that are chained together is really what tells us if the given entity got published or unpublished by scheduler. If it does, then the rest is OK, except for the failing PHPCS test.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada bdunphy

    @jurgenhaas - my tests included publish/unpublish not by scheduler and of course publish / unpublish by scheduler. These worked flawlessly. In order to determine if scheduler actually performed the publish, one has to look at the previous (original) values of both publish_on and status and compare to the current values. The logic looks correct to me. The same is true for unpublishing.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    @bdunphy that's OK, and I'm sure the logic of that part can be reviewed by maintainers of the scheduler module who know exactly how that works.

    I just wonder about 2 things:

    $entity->original->publish_on->value ?? NULL
    

    Wouldn't that throw a NULL pointer exception if publish_on didn't exist?

    $entity->status->value
    

    Similarly, the status field doesn't need to be called status. I guess the safe way of doing this is to get the property key for status from the entity annotation and then ask for the respective value of that field.

    As for an ECA point of view, the plugins looks good to me, I think I mentioned that before.

Production build 0.71.5 2024