Add database tables

[rrennick]

Fixes #77
Implements woocommerce/action-scheduler-custom-tables#48 .

As of 97337a8 the PR works with PHP 7.3.

Outstanding items

• PHP 5.3 compatibility Migration & custom tables are disabled in PHP versions <5.6.
• Unit tests
• Unit test failures
• Tweak/bugfix small inconsistencies (eg. the migration notice shows even when the migration is disabled by a custom data store).

Tests Scenarios

• Upgrade site from Action Scheduler 2.2.n and confirm migration completes correctly
• Upgrade site from Action Scheduler Custom Tables plugin and confirm no errors or interference with previously migrated actions

[todo]

use filter to get source class, if not default class then apply a filter to verify migration

https://github.com/Prospress/action-scheduler/blob/82d7f6b5646f7c765eceb30b94e9c723d43e1010/classes/ActionScheduler_Data.php#L105-L110

---

This comment was generated by todo based on a @todo comment in 82d7f6b in #259. cc @Prospress.

[rrennick]

The planned date for updating WP to PHP 5.6 minimum is April - https://make.wordpress.org/core/2018/12/08/updating-the-minimum-php-version/ . Rather than make the DB tables data store compatible with PHP 5.3, having a 5.6 dependency on enabling it would be better.

[rrennick]

@JPry Have you encountered the error in https://travis-ci.org/Prospress/action-scheduler/jobs/507947421 ? These tests run successfully locally so I'm wondering if this is a phpunit version issue?

[JPry]

@rrennick I haven't seen that particular error before. However, I can see that an older version of PHPUnit is being run: version 4.8.27. I believe we should be using version 5+. This may require downloading the correct version of PHPUnit as part of the pre-test setup.

[rrennick]

@JPry Thanks, that worked. The second issue is that the phpunit -c tests/phpunit.xml.dist --exclude-group migration does exclude the test in my local env that is failing at

https://travis-ci.org/Prospress/action-scheduler/jobs/508379185#L139

Could that be related to the version of the WP tests lib?

[JPry]

@rrennick It looks like that particular error is triggered on PHP 5.3 when using the short array syntax. I also see that the group on that file is tables, not migration

[rrennick]

I also see that the group on that file is tables, not migration

Both groups are excluded. I verified that the phpunit command in my previous comment does exclude that test in my local environment.

[JPry]

Perhaps it still has to parse the file in order to determine if it's part of the group, and it's the parsing that is triggering the error.

I think any files that require a certain PHP version need to be included in a separate test suite with a version dependency. This is described here: https://phpunit.readthedocs.io/en/8.0/configuration.html#test-suites

[rrennick]

Perhaps it still has to parse the file in order to determine if it's part of the group, and it's the parsing that is triggering the error.

That sounds like the ticket.

[rrennick]

This one is ready for review. If we could get this into the 3.0.0 branch then we could look at merging master into the branch, addressing conflicts with other PRs, get some testing going & fix any follow up issues.

[rcstr]

Hey @rrennick I have left some initial comments, I will take a look again once the conflicts are solved because I saw some pieces of code in this PR that are already in master 👍

[thenbrent]

This is looking good @rrennick! I've left some feedback & requested changes.

[thenbrent]

I've just added a couple of Tests Scenarios to the main description on this PR.

[rrennick]

What do you think? Do you think we can rename some of the classes and remove the prefix?

@rcstr Done in 78cf427

[rrennick]

@rcstr Updated PHPDocs in 5370f4b

[rrennick]

@rcstr @thenbrent Thanks so much for the recommendations. I've updated the PR with

• rename WP CLI command to migrate
• move CLI command to common register block
• split ActionScheduler_Data into data controller and migration controller
• move dependencies & migration complete to data controller
• no migration related code is loaded unless the site is eligible for migration
• rename migration hook group
• removed custom_ from a few hooks, strings, etc.
• fix help on CLI run command
• move free_memory to data controller
• use free_memory() in stop_the_insanity()
• add free-memory-on and pause parameters to CLI commands
• check for object cache object
• other suggested fixes
• unit test class name usage changes

The failed unit test is the one fixed in #300. Some of the unit tests should probably be moved to a new file for the data controller but that can be done in a follow up PR.

[rcstr]

Thank you @rrennick - I will look into the changes early next week 👍

[rcstr]

Hi @rrennick - thanks for applying the requested changes, I have requested a few minor changes.

I still have a few files left to review, then I will proceed with QA, do you have a recommendation for what would be the best way to test? I was thinking of setting up a gimme site or one of them to test this

[rrennick]

do you have a recommendation for what would be the best way to test?

What I do is make a backup of my DB so I can restore to re-run the migration as many times as needed. Then

• use a snippet of code like Add async queue processing #323 (for that snippet you need to switch to that branch) to generate a few thousand actions.
• make a second backup so you can re-run migrations
• migrate with WP CLI
• let another one process on the normal cron
• I'd be interested in seeing the time comparison of the migration on the async PR as well

[rrennick]

@thenbrent @rcstr This has had the merge conflicts fixed & unit tests updated to work with merged changes.

[thenbrent]

Merging this one. Nick work @rrennick! Thanks for the reviews @rcstr! 🎉

I gave all files a final review today, comparing all files to their original in the Custom Tables plugin and reviewing carefully all newly introduced code. I have a number of outstanding items I'd like addressed, but there was only one bug, the rest are just refactoring or tweaks, which we can submit in separate PRs rather than blocking this one (which has merge conflicts with other open PRs and is generally an enormous diff to keep coming back to for small changes).

I still have a few files left to review, then I will proceed with QA

@rcstr given the large number of other PRs open for 3.0.0, instead of spending QA time on this, I recommend from here, we focus on QA of the first beta, as it will help test other important changes too, like the async runner, and potentially refactor of recurring schedules #333.