- 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.
- ๐จ๐ฆ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() - ๐จ๐ฆ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.
- ๐ฉ๐ช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.
- ๐ฌ๐ง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 itI 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 cronThe 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.
- ๐ฌ๐งUnited Kingdom jonathan1055
From #25 and #26 I have raised ๐ New require-dev dependencies in composer.json are not loaded in composer pipeline job Active
- ๐ฌ๐ง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.
- ๐จ๐ฆ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.