Closed rrennick closed 5 years ago
@thenbrent Updated the PR per suggestions. Unit tests were also updated to use the new constructor. The one test failure was an instance of a timestamp being 1 second off.
@rrennick this is looking good. In answer to this:
do you think I should look at a way to re-hook the action created hooks after the batch is done?
Yes, definitely. The migration can be run by Action Scheduler, in which case, if we don't re-hook the logging, we won't get logs for both the remaining events, like completion, on the 'action_scheduler/custom_tables/migration_hook'
action, as well as other actions run in the same queue.
It's probably a good idea to add new unhook()
/hook()
methods to handle this, similar to Migration_Scheduler::unhook()
/Migration_Scheduler::hook()
.
It's probably a good idea to add new unhook()/hook() methods to handle this
See https://github.com/Prospress/action-scheduler/issues/247
I added re-hooking for now since the comment store functions need to be done in AS. When I migrate it over I'll update this code to use the new functions.
LGTM. 👍
Fixes #45
action created
log entry which uses the current time@thenbrent , @JPry do you think I should look at a way to re-hook the
action created
hooks after the batch is done?