Make WordPress Core

Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#32127 closed defect (bug) (fixed)

Broken support for utf8mb4 with the mysql extension

Reported by: kovshenin's profile kovshenin Owned by: dd32's profile dd32
Milestone: 4.2.3 Priority: high
Severity: blocker Version: 4.2
Component: Database Keywords: has-patch fixed-major
Focuses: Cc:

Description

I didn't dig very deep but I didn't find a mention that mysqli was a requirement for utf8mb4, yet in r31349 we short-circuit init_charset if we don't have an instanceof mysqli. Seems like this was an oversight.

Attachments (3)

32127.diff (417 bytes) - added by kovshenin 10 years ago.
32127.2.diff (2.5 KB) - added by pento 10 years ago.
32127.3.diff (2.6 KB) - added by pento 10 years ago.

Download all attachments as: .zip

Change History (32)

@kovshenin
10 years ago

#1 @kovshenin
10 years ago

  • Keywords has-patch added

Sorry, I should clarify. By "broken" I mean indices will be stripped, but the tables won't be converted to utf8mb4 because the charset is never set to utf8mb4, even though has_cap() returns true.

#2 @nacin
10 years ago

So what do we need to do to fix this in an upgrade routine? Re-run the conversion? We already considered re-running that with each upgrade, or rather checking to see if we should run it, in case something changed with the PHP or MySQL versions or configuration. Might as well consider that now.

By indices "stripped", do you mean altered?

#3 @kovshenin
10 years ago

Yes, I mean the change in length, I guess I was looking for the word "trimmed" and not stripped. With regards to the upgrade, I think it would make sense to check again for utf8mb4 support and convert any remaining utf8 tables.

#4 @laserjobs
10 years ago

I was wondering why my databases were not converting with the update to version 4.2. I changed the code in /wp-includes/wp-db.php and downgraded the database version number to 31349 and voila, it worked. So it looks like 4.2.1 will need to recheck with this code change. Thanks!

#5 @DrewAPicture
10 years ago

  • Keywords needs-testing added

#6 @pento
10 years ago

#32212 was marked as a duplicate.

@pento
10 years ago

#7 @pento
10 years ago

32127.2.diff checks if the index has been altered, then tries to alter it again.

It also re-runs maybe_convert_table_to_utf8mb4() on everything, which will do nothing if the table is already utf8mb4.

@pento
10 years ago

#8 follow-up: @pento
10 years ago

32127.3.diff adds an is_multisite() check for wp_usermeta, because that's upgraded in upgrade_network(), instead.

This ticket was mentioned in Slack in #core by pento. View the logs.


10 years ago

#10 @samuelsidler
10 years ago

  • Milestone changed from 4.2.2 to 4.2.3

#11 @chriscct7
10 years ago

  • Priority changed from normal to high
  • Severity changed from normal to major

Raising priority/severity of ticket per implications of this bug given #32308 via #32212

#12 follow-up: @Azbuildstuff
10 years ago

  • Version set to 4.2.2

Catchable fatal error: Object of class WP_Error could not be converted to string in /home/ /public_html/wp-includes/kses.php on line 1038

So I take it that this is still an outstanding issue? Multisite will not create new site.

#13 in reply to: ↑ 12 @chriscct7
10 years ago

Replying to Azbuildstuff:

Multisite will not create new site.

Yes, this is the root cause of #32308 which is that issue

#14 @chriscct7
10 years ago

  • Severity changed from major to critical

Given the recent implications of this ticket and the ramifications that this has on multisite (#32308), I decided to raise the severity to critical only because I feel this issue is a hair below what an absolute blocker should be.

#15 in reply to: ↑ 8 @BearlyDoug
10 years ago

  • Severity changed from critical to major

On Wordpress 4.2.2, there was a double issue when trying to add a site into a brand new freshly configured WP Installation. Through Chris' research in Ticket #32308, which pointed me here, I was able to fix my WP installation.

That being said, 32127.3.diff probably needs to be adjusted to account for Wordpress 4.2.2. The fix indicated for line 530 needs to be changed to start on line 536.

Lines 527 through lines 535 in src/wp-admin/includes/upgrade.php are quoted below:

Don't harsh my mellow. upgrade_422() must be called before
upgrade_420() to catch bad comments prior to any auto-expansion of
MySQL column widths.
if ( $wp_current_db_version < 31534 )

upgrade_422();

if ( $wp_current_db_version < 31351 )

upgrade_420();

Version 0, edited 10 years ago by BearlyDoug (next)

#16 @BearlyDoug
10 years ago

  • Severity changed from major to critical

For some reason, as I was replying above, I did not see Chris' change to the priority of this ticket, so I'm re-setting it from Major back to Critical

Clarification on the above. I did an initial install of Wordpress 4.2 and then upgraded to 4.2.2.

Last edited 10 years ago by BearlyDoug (previous) (diff)

#17 follow-ups: @ilovewpsomuch
10 years ago

This is so cool, how fumbling of a character set issue and little flaws in this hugely complicated upgrade engine, results in my company having to discontinue sales and me being not deal with the fallout. If I had a vote I would vote for making the upgrade process short. With such a long and intricate process, it is inevitable that breakdowns would be routine, which indeed they are, as shown Google searchng on WP upgrades causing problems.

What is the process for doing a review on the upgrade architecture? Is this discussion the primary control?

#18 in reply to: ↑ 17 @BearlyDoug
10 years ago

Replying to ilovewpsomuch:

This is so cool, how fumbling of a character set issue and little flaws in this hugely complicated upgrade engine, results in my company having to discontinue sales and me being not deal with the fallout. If I had a vote I would vote for making the upgrade process short. With such a long and intricate process, it is inevitable that breakdowns would be routine, which indeed they are, as shown Google searchng on WP upgrades causing problems.

What is the process for doing a review on the upgrade architecture? Is this discussion the primary control?

If you do both of the patches I outlined in Comment 15, this will restore the functionality needed to add sites. Take care to note the location change for one of the patches (You might have to patch upgrade.php manually, completely).

As additional character sets get added in, this might be an issue going forward... I'm not sure that the bug listed in this issue is entirely WP's fault.

#19 in reply to: ↑ 17 @chriscct7
10 years ago

Replying to ilovewpsomuch:

This is so cool, how fumbling of a character set issue and little flaws in this hugely complicated upgrade engine, results in my company having to discontinue sales and me being not deal with the fallout. If I had a vote I would vote for making the upgrade process short.

With such a long and intricate process, it is inevitable that breakdowns would be routine, which indeed they are, as shown Google searching on WP upgrades causing problems.

Actually given how complex WordPress core is, they're relatively simple. The number of search results on Google isn't a good way to gauge "issues" as WordPress now powers 23% of all websites roughly. There will be issues even if there wasn't an upgrade routine, simply because of things out of WordPress's control (like hosting issues, etc).

What is the process for doing a review on the upgrade architecture? Is this discussion the primary control?

This is a far more complicated matter than a simple characterset upgrade. The release also fixed a pair of security issues, and solved some long standing table schema issues. The schema updates were discussed at length for several years before they happened, along with many people testing the numerous alpha (trunk), and formal beta and rc releases. The best way to get involved with helping with this, if you'd like, is to help test beta and release candidates. They are announced on make.wordpress.org when they are available.

In the future, you should test upgrades for plugins, themes and WordPress core using a test install. Sean Davis wrote a great guide on why and how here: https://easydigitaldownloads.com/blog/staging-site-e-commerce/

#20 @lmartins
10 years ago

If anything my only critic on this is that it probably shouldn't have been released on a point release, especially considering the implications we are now facing. Many thanks for the devs who are trying to fix this.

#21 @pento
10 years ago

  • Keywords needs-refresh added
  • Version changed from 4.2.2 to 4.2

I wrote 32127.2.diff at the tail end of 4.2.2, so it really does need testing before it's ready to go in. It also needs updating, as mentioned earlier.

I'm still AFK, so won't be able to help with that.

#22 @chriscct7
10 years ago

  • Severity changed from critical to blocker

#23 @BearlyDoug
10 years ago

Additional follow up: Tickets #32560 (my own issue; recently posted) and #32308 seem to go hand in hand with this ticket.

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


10 years ago

#25 @miqrogroove
9 years ago

#32308 was marked as a duplicate.

#26 @dd32
9 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

In 33055:

Enable utf8mb4 for MySQL extension users. Previously utf8mb4 was limited to MySQLi users only unintentionally.
This change does the following things

  • Allows utf8mb4 for the MySQL extension
  • Re-runs the utf8->utf8mb4 conversion for single sites, this will do nothing for tables already converted
  • Re-runs the utf8->utf8mb4 conversion for global tables in multisite when the environment supports utf8mb4
  • Removes upgrade_420() calling as upgrade_430() will perform those changes now instead

The index shortenings should have still taken place on utf8 sites previously, so there's no need to run those again.

Props kovshenin, pento, dd32
Fixes #32127 for trunk.

#27 @dd32
9 years ago

  • Keywords fixed-major added; needs-testing needs-refresh removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

I've opened #32868 to track running the utf8->utf8mb4 conversion on each update.

[33055] should handle this for 4.3, backporting to 4.2 will be needed.

Last edited 9 years ago by dd32 (previous) (diff)

#28 @dd32
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 33063:

Enable utf8mb4 for MySQL extension users. Previously utf8mb4 was limited to MySQLi users only unintentionally.

Ports [33055] to the 4.2 branch
Fixes #32127 for 4.2.3

#29 @dd32
9 years ago

[33063] could've potentially skipped doing the utf8 to utf8mb4 conversion, leaving it for the 4.3 upgrade to kick in again.

I went ahead and ported it anyway, for the simple fact that hopefully this will reduce any chance of the 4.3 term splitting upgrade routines running into trouble.

Note: See TracTickets for help on using tickets.