Make WordPress Core

Opened 7 years ago

Last modified 4 years ago

#38921 new defect (bug)

Defining filter pre_get_table_charset causes errors

Reported by: haoran's profile haoran Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.2
Component: Database Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

In wp-db.php::get_table_charset(), a filter is defined, pre_get_table_charset.

However, if this filter is defined, wp-db.php::get_col_charset() throws errors. This is because the latter assumes that it can find cached results cached in:

$this->table_charset[];

The assumption seems to be that if you add the pre_get_table_charset filter, you will also define the pre_get_col_charset filter.

Code reference: https://github.com/WordPress/WordPress/blob/master/wp-includes/wp-db.php#L2437

Note: defining this filter lets you hard-code the charset of tables, rather than doing a database lookup every time. This can be done quite safely because the most recent upgrade forced our DB tables to be utf8mb4.

Attachments (1)

38921.diff (1.7 KB) - added by birgire 6 years ago.

Download all attachments as: .zip

Change History (5)

#1 @pento
7 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 4.2

Thanks for the bug report @haoran!

We can definitely get this fixed up. :-)

#2 @cjbj
7 years ago

I can confirm this bug. We discussed it in more detail on wpse: http://wordpress.stackexchange.com/q/247109/75495

#3 @birgire
6 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

In 38921.diff there's a suggestion how to set $this->table_charset, when the the table charset is short-curcuited with the pre_get_table_charset filter.

There's an existing test for the wpdb::get_table_charset() method, with the pre_get_table_charset filter, in (Source):

Tests_DB::test_pre_get_table_charset_filter()

but it doesn't test it with the wpdb::get_col_charset() method as well.

38921.diff extends that test with a new one in:

Tests_DB_Charset::test_get_table_and_column_charset_with_pre_get_table_charset_filter()

@birgire
6 years ago

#4 @kovshenin
4 years ago

Yeah, definitely a bug.

I have a table with some ascii columns and a couple of utf8mb4 columns, and the default wpdb behavior treats my entire table as ascii, so I get the CONVERT(CONVERT()) bullet when inserting into my utf8mb4 column via strip_invalid_text(). That pre_get_table_charset filter's definitely a life saver for me and did exactly what I expected it to do, though I was a bit sad about a whole bunch of warnings from get_col_charset().

A workaround is to filter pre_get_col_charset as well.

The patch from @birgire looks good though.

Note: See TracTickets for help on using tickets.