Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.x] Improve handling of scheduled entries #10966

Merged
merged 11 commits into from
Oct 22, 2024
Merged

Conversation

jasonvarga
Copy link
Member

@jasonvarga jasonvarga commented Oct 16, 2024

This PR adds a scheduled task that runs every minute and dispatches an event for every entry due to go live during the previous minute.

The reason it checks the previous minute is so that if the scheduler runs at for example 30s into the minute, and you have an entry with date defined at 40s into that minute, when the event gets dispatched at 30s the entry would technically still be considered scheduled. By checking the previous minute we can know that all the entries would have already become published.

Heavily inspired by the Scheduled Cache Invalidator addon by @martyf. Unlike that addon, this only supports dated collections. You may want to continue using the addon if you have more complex requirements like custom date fields.

  • Search will make use of this to insert entries into appropriate indexes. Scheduled entries do not get included in indexes, but need to be included when the time is right.
  • Static caching will make use of this to invalidate appropriate pages at the right time. The same rules would be triggered if you clicked save on the entry.
  • GraphQL and REST APIs will trigger their invalidation the same was as if you were to click save on an entry. For the APIs the default invalidator just clears the whole thing.

Todo:

  • Event name doesn't feel quite right. EntryScheduleFulfilled? EntryScheduleResolved? EntryScheduleCompleted? If/when we add entry expiration, a suitable name for that would be EntryExpired. I don't want to use "Published" since we already use that for a separate concept - the opposite of a draft.
  • Static cache invalidation
  • API invalidation
  • Tests

@jasonvarga jasonvarga marked this pull request as draft October 16, 2024 21:20
@martyf
Copy link
Contributor

martyf commented Oct 16, 2024

Looking good Jason. Is this something that could extend to the cache invalidators to help (and potentially make the addon no longer required)? Or is that outside of the scope at this stage?

@ryanmitchell
Copy link
Contributor

Love this.

Event name doesn't feel quite right. EntryScheduleFulfilled? EntryScheduleResolved? EntryScheduleCompleted?

Seeing as its not actioning anything, just firing the event, maybe EntryScheduleReached or EntryScheduleTriggered might be a more suitable name.

If/when we add entry expiration, a suitable name for that would be EntryExpired. I don't want to use "Published" since we already use that for a separate concept - the opposite of a draft.

If that happens it could just fire the same event, seeing as its not actioning anything.

@jasonvarga
Copy link
Member Author

s this something that could extend to the cache invalidators to help (and potentially make the addon no longer required)?

It's on my list 👍

@jasonvarga jasonvarga changed the title [5.x] Scheduled entry events [5.x] Improve handling of scheduled entries Oct 17, 2024
@jasonvarga
Copy link
Member Author

This is ready to try out if anyone wants to give it a whirl. Simply running the scheduler should be enough with artisan schedule:work - you don't need to add anything to any providers.

@martyf
Copy link
Contributor

martyf commented Oct 18, 2024

Works a treat.

Curious for the logic as to why it checks the previous minute, and not the current minute? Is it because technically the current minute, if using seconds, might still be in the future?

I think avoiding words like "expired" for the event is good too - it could sometimes be about scheduling new content, or old content, so it isn't always an "expire" which sounds like it covers only one of those.

My only thought would be if EntryScheduled is cleaner than EntryScheduleReached?

But otherwise, working as expected, including refreshing static caches.

@jasonvarga jasonvarga marked this pull request as ready for review October 18, 2024 13:40
@jasonvarga
Copy link
Member Author

Thanks for testing!

Curious for the logic as to why it checks the previous minute, and not the current minute? Is it because technically the current minute, if using seconds, might still be in the future?

Yes, exactly.

If the entry has a datetime of 12:00:35, and the scheduler runs at 12:00:10, the event would be dispatched, but the entry would technically still be in the future. I don't think we have an accurate enough way to say "get all the entries whose date lies between now and the exact second the last scheduled task ran" so grabbing entries from the previous minute is the best solution I can think of.

In your addon a comment mentions that Statamic doesn't use seconds for publishing. I didn't understand this. It absolutely can.

I think avoiding words like "expired" for the event is good too

Maybe I'm misunderstanding what you're saying here. I was just saying if in the future we add a feature where you can "expire" entries, we'd have the same problem and would need to use this scheduled task to detect when entries expire. (With a separate query + event)

My only thought would be if EntryScheduled is cleaner than EntryScheduleReached?

EntryScheduled says to me "the entry has been assigned a schedule" more than "we've hit the date that the entry was scheduled for".

@robdekort
Copy link
Contributor

Feels like Christmas every day with you guys. So basically you just turn on the default Laravel cron every minute and all is great? That is wonderful!

@jasonvarga
Copy link
Member Author

Yep, in Forge or whatever hosting environment you need that single cron job that runs artisan schedule:run every minute. If you already had that set up, you don't need to do anything.

Locally, you can run artisan schedule:work once and it'll just run forever until you cancel it.

@robdekort
Copy link
Contributor

Yep, in Forge or whatever hosting environment you need that single cron job that runs artisan schedule:run every minute. If you already had that set up, you don't need to do anything.

That is awesome!

Locally, you can run artisan schedule:work once and it'll just run forever until you cancel it.

Just read up on that. Thanks Jason!

@robdekort
Copy link
Contributor

This also means I can probably get rid of those (commented) scheduled commands here: https://github.com/studio1902/statamic-peak/blob/main/app/Console/Kernel.php

Lovely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants