Comments being lost and duplicate emails since upgrade from 3.0.151 to 3.0153

[adrianbj]

Short description of the issue

If the title of the page has an apostrophe in it, then I get two email notifications when a comment is posted for approval, once that has the title in the subject line correct and the other email where the apostrophe is converted to '

That's annoying, but the main problem is that often (not always), the comments are missing from the system entirely (email notification arrives, but comment is not in database). I noticed that this error is being logged:

"Error adding new comment on page 5676 field comments for xxxx@gmail.com: SQLSTATE[HY000]: General error: 1364 Field 'sort' doesn't have a default value"

[adrianbj]

Also, this error was logged at the same time:
"Error saving comment id 0: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'user_agent' at row 1"

Is there a reason that field is varchar(191)- is the "191" limit really intended?

[adrianbj]

OK, it seems there is a connection between moving from utf8 255 to utf8mb4 191, but clearly it's not enough to store user agent data so maybe it needs to be text or something else?

[adrianbj]

@ryancramerdesign - just wondering if you've had a chance to look at this yet. Having to manually recreate comments from the contents of the email notification is getting a bit tiresome :)

Note that I am getting two copies of the notification email regardless of whether there is an apostrophe in the title or not - turns out that wasn't relevant although still interesting that one copy of the email has the apostrophe in the title encoded and the other doesn't.

I have just changed the type for the user_agent field from varchar to text so that should hopefully fix the problem with the comments not being added to the database, but the double emails will presumably remain.

[adrianbj]

I can confirm that comments are now being saved (after changing the DB user_agent field type to text.

[adrianbj]

@ryancramerdesign - I looked into this further and it's another case of this error:

FieldtypeComments: Error adding new comment on page 5724 field comments for me@gmail.com: SQLSTATE[HY000]: General error: 1364 Field 'sort' doesn't have a default value

If I alter field_comments sort field to allow NULL then the duplicate email problem is gone. I know that sounds weird, but I checked the contents of $value in the ___sleepValue method and it was showing two comments (one a duplicate of the other). As soon as I allowed NULL, the problem was gone. Here are the entries for my test comments:

You can clearly see that one attempt at storing the comment is being broken by the MYSQL 1364 error noted above, which is why every second sort value is missing. But that lost attempt is still resulting in a notification email being sent.

Anyway, the key thing is to fix the issue with sort. Obviously allowing NULL is not the correct solution as you can see from the last entry because a proper value isn't being populated so I dived a little deeper into the addComment() method and was surprised to find that $value and therefore $binds never includes a sort value until the try/catch kicks in. The though is that with the "1364" error I am seeing, your check for $e->getCode() == 23000 doesn't result in the sort being added correctly.

I assumed you would get the last sort value for a page before trying to insert a new comment, rather than using a try/catch and try multiple times to insert, each time increasing the sort value. Could you explain the reasoning behind this please?

Anyway, as a quick fix, I have added:

$value['sort'] = 0;

after this line: https://github.com/processwire/processwire/blob/88e04129c72c1702926543c8bef555a9f0d1043f/wire/modules/Fieldtype/FieldtypeComments/FieldtypeComments.module#L1576

That fixes the sort not having default error and means only one admin notification email is received.

As you've noticed in the Github issues and also several forum posts I have made recently, this default sort value error is showing up everywhere and it's breaking lots of things so please consider this in urgent need of proper refactoring - perhaps getting the last sort value first and calculating, rather than attempting to insert a row with no initial value for it.

Thank you!

[Toutouwai]

@adrianbj, have you experimented to see if these SQL issues you've been experiencing can be resolved by changing the SQL mode settings?

PW already removes some SQL mode settings based on the MySQL version: https://github.com/processwire/processwire/blob/88e04129c72c1702926543c8bef555a9f0d1043f/wire/config.php#L1086-L1105
Maybe this could be adjusted to account for any changes to the default SQL mode in MySQL 8?

So long as the data PW sends to the DB is already valid there arguably isn't a need for MySQL to do additional validation. And it sounds like there is a performance benefit if strict modes are disabled: https://dev.mysql.com/doc/refman/8.0/en/faqs-sql-modes.html#faq-mysql-strict-impact

[adrianbj]

@Toutouwai - thanks for you thoughts!

I haven't changed the default SQL settings and I don't really think it should take that path. I was actually disappointed when PW removed that ONLY_FULL_GROUP_BY setting. When it was first introduced to MySQL I had to do a lot of work on some sites which do significant data manipulations via SQL queries to make them work with the new setting. I did a a lot of research and was convinced that the new setting was an important change to avoid errors due to ambiguity, so I deferred to the experts. But perhaps disabling strict mode is less offensive that the ONLY_FULL_GROUP_BY option.

I think the key thing with the sort value is that (at least for the comments field, which is what I have investigated most thoroughly) it is not being sent along with the initial row insert at all, which is why it's returning the "doesn't have a default value" error. I don't think PW should skip this during the initial insert - I think it should find the max relevant sort value and add one to it.

Also, note that the comments field also has the problem with the user_agent field type mentioned in my second post above, so there are several bugs at play here.

[ryancramerdesign]

Thanks @adrianbj this is a good find, I didn't realize the new addComment() was skipping over a sort value (was not intended) and it has now been fixed. Though if possible please confirm it fixes the issue there, as I was not experiencing the error here in testing, so just want to double check.

Regarding the user_agent being a TEXT field, I think it's maybe better to limit how much can be stored in it as it's not particularly important data. So the commit also runs it through a maxLength sanitizer which should hopefully prevent MySQL from complaining about a long user_agent.

[adrianbj]

Thanks @ryancramerdesign - that all seems to be working fine. Sorry I didn't notice the new getMaxSort() method - looks like you were already on it, just didn't quite implement it fully :)