_pum_subscribers table fails to create on MySQL 8.0.19+ due to VALUES keyword

[danieliser]

Describe the bug

Subscribers table not created when MySQL version is over 8.0.19 it appears due to the values field name being a reserved keyword.

https://dev.mysql.com/doc/refman/8.0/en/values.html

Site information

MySQL 8.0.19+

Errors

[12-Aug-2020 20:05:16 UTC] Error You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ‘values longtext NOT NULL,
uuid varchar(255) NOT NULL,
consent varchar(25’ at line 10 de la base de datos de WordPress para la consulta CREATE TABLE wp_pum_subscribers (
ID bigint(20) NOT NULL AUTO_INCREMENT,
email_hash varchar(32) NOT NULL,
popup_id bigint(20) NOT NULL,
user_id bigint(20) NOT NULL,
email varchar(191) NOT NULL,
name varchar(255) NOT NULL,
fname varchar(255) NOT NULL,
lname varchar(255) NOT NULL,
values longtext NOT NULL,
uuid varchar(255) NOT NULL,
consent varchar(255) NOT NULL,
consent_args longtext NOT NULL,
created datetime NOT NULL,
PRIMARY KEY (ID),
KEY email (email),
KEY user_id (user_id),
KEY popup_id (popup_id),
KEY email_hash (email_hash)
) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_520_ci realizada por require_once(‘wp-admin/admin.php’), do_action(‘popup_page_pum-subscribers’), WP_Hook->do_action, WP_Hook->apply_filters, PUM_Admin_Subscribers::page, PUM_Admin_Subscribers_Table->prepare_items, PUM_Abstract_Database::instance, PUM_Abstract_Database->__construct, PUM_DB_Subscribers->create_table, dbDelta

Additional context

Tickets:

• https://wordpress.org/support/topic/php-error-log-23/

[fpcorso]

@danieliser Do you think this is the source of all the subscriber-related tickets?

I looked through the code to see what the values column is for and it seems to just be a serialized version of all the other columns as well as any extra values attached to the form:

Popup-Maker/classes/Newsletters.php

Lines 179 to 195 in bd1541b

	public static function record_submission( $values = array() ) {
	$data = wp_parse_args( $values, array(
	'uuid' => self::uuid(),
	'user_id' => get_current_user_id(),
	'popup_id' => 0,
	'email_hash' => '',
	'email' => '',
	'name' => '',
	'fname' => '',
	'lname' => '',
	'consent' => 'no',
	'consent_args' => '',
	) );
	$data['values'] = maybe_serialize( $values );
	$subscriber_id = PUM_DB_Subscribers::instance()->insert( $data );

Do we use that data anywhere? What was its intended purpose? Is it still necessary?

If we keep the field, what would be a better name? Maybe data or entry?

[danieliser]

Yea it needs renamed, but that is 3 step process:

1. Add new data field, using dbDelta
2. Run query to copy all values fields to data, manual $wpdb query.
3. Remove values field, manual $wpdb query.

[danieliser]

And no I'm not sure its used. Going forward we will likely be moving that all to a saas form so not sure its worth saving overall or just drop it for now.

[fpcorso]

@danieliser I looked through the PUM_Newsletters class and the PUM_Admin_Subscribers_Table class and we do not seem to use the values column anywhere. On my local site, I deleted the data in the values column for my entries and received no errors when viewing the subscriber page or adding more subscribers.

So, what should we do here? Do you want to drop the column or do the renaming process? We had several people post about this issue in the FB group just this weekend so we probably need to get this solved quickly.

[danieliser]

@fpcorso I say lets just drop it for now. It was likely put there for some future proofing, but never ended up being useful.

[fpcorso]

@danieliser Ok. The easy first step is to remove that column from the install query which I can add to 1.12 now. Since those who experienced the error couldn't create the table anyways, do we need to build out a check/process for dropping the column on the existing tables or is removing it from the install query enough for this case?

[fpcorso]

Removed all the values related code in a recent commit on develop. Was still able to subscribe using the forms and the "subscribers" tab works well. So, all that's left is my question above.

[danieliser]

@fpcorso I think just removing it and most importantly bumping the table version should do the trick for now.

While you are at it, convert the version from a semantic style to a date style for better consistency and tracking of changes.

For example given todays date.

public $version = 3.1;

Would become the following.

public $version = 20200916;

In the case there are multiple version changes in a single day, you simple append a .1 and so on.

[fpcorso]

@danieliser Updated the version in PUM_DB_Subscribers class.