Add Stats Schema #2719
Add Stats Schema #2719
Conversation
includes/class-stats.php
Outdated
| [ | ||
| "CREATE TABLE {$wpdb->job_manager_stats} ( | ||
| `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT, | ||
| `date` DATE NOT NULL, |
There was a problem hiding this comment.
I had to remove the CURRENT_DATE here because apparently it is not supported by MySQL older than the 8.0.20
includes/class-stats.php
Outdated
| private function initialize_wpdb() { | ||
| global $wpdb; | ||
| $wpdb->job_manager_stats = $wpdb->prefix . 'job_manager_stats'; | ||
| $wpdb->tables[] = 'job_manager_stats'; |
There was a problem hiding this comment.
The reason for including the table name on tables is so WP automatically updates the attribute with the proper name with the prefix when switching to another blog on a multisite installation.
| global $wpdb; | ||
| $collate = $wpdb->get_charset_collate(); | ||
| require_once ABSPATH . 'wp-admin/includes/upgrade.php'; | ||
| \dbDelta( |
There was a problem hiding this comment.
This function will run automatically a diff for us on the SQL and do any changes on the database side.
Also register it on the tables array so it is handled by WP automatically too when switching to another blog in a multisite install
There was a problem hiding this comment.
It worked well! I left some comments, mainly around compat and for moving some things around a bit.
Edit: Forgot to mention something important: The table name we use is the same table name used by Astoundify stats. I think that this might be a problem, specially since enabling the plugin might dbDelta an existing astoundify table and possibly corrupt existing stats.
includes/class-stats.php
Outdated
| require_once ABSPATH . 'wp-admin/includes/upgrade.php'; | ||
| \dbDelta( | ||
| [ | ||
| "CREATE TABLE {$wpdb->job_manager_stats} ( |
There was a problem hiding this comment.
| "CREATE TABLE {$wpdb->job_manager_stats} ( | |
| "CREATE TABLE IF NOT EXISTS {$wpdb->job_manager_stats} ( |
There was a problem hiding this comment.
I know this isn't clear at first, but dbDelta already handles that for us, and I think it doesn't support having IF NOT EXISTS on the table name (it will consider the IF NOT EXISTS part of the table name, which I don't think we want to have). See how it handles that here: https://github.com/WordPress/WordPress/blob/27348531f3fdf048809da977fced3f40b90259de/wp-admin/includes/upgrade.php#L2836-L2840
You can see here that no WP tables use IF NOT EXISTS when creating the schema (those are passed to dbDelta, too). So I'm inclined to not add them. WDYT?
| * | ||
| * @var $custom_tables | ||
| */ | ||
| private static $custom_tables = [ |
There was a problem hiding this comment.
All the other properties in the class are private static, hence why I put this as private static too.
I agree that changing everything to private const might make more sense, but I'd change all the other properties too. Do you think it is worth it, @pgk?
There was a problem hiding this comment.
I think that if this property is always constant it makes sense for it to be const.
There was a problem hiding this comment.
I changed that specific property in dd028a7. As mentioned on Slack, I'll submit another PR to change the other properties too. :)
includes/class-stats.php
Outdated
| private function initialize_wpdb() { | ||
| global $wpdb; | ||
| $wpdb->job_manager_stats = $wpdb->prefix . 'job_manager_stats'; | ||
| $wpdb->tables[] = 'job_manager_stats'; |
Co-authored-by: Panos Kountanis <[email protected]>
To avoid conflict with third-party plugins
Great catch! I renamed it in 5f63042 . Thanks! |
Fixes #2715
Changes Proposed in this Pull Request
Testing Instructions
Check if the plugin can be activated on a new WP install just fine, without any errors on the error log. Also check if the
wp_job_manager_statsis created as appropriateRelease Notes
New or Updated Hooks and Templates
Deprecated Code
Screenshot / Video