v2.1.12 #131
Merged
v2.1.12 #131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes a series of crashes encountered when
* SHOW TABLES now returns a flat list, not an array of objects – I'm not
sure here. It makes `$tables = $wpdb->get_results("SHOW TABLES LIKE
'{$prefix}%'", ARRAY_N);` work as expected
[here](https://github.com/WordPress/playground-tools/blob/128123e84e25cf0421770f6885c6f26aaf0b05dc/packages/playground/src/playground-db.php#L90-L91),
but is the right thing to return, or is this more about correct handling
of the fetch mode, like ARRAY_N?
* `SHOW tables` isn't all uppercase
* There's no table tho SHOW CREATE: `SHOW CREATE TABLE _no_such_table;`
* The table identifier passed to SHOW CREATE is quoted:
```
'SHOW CREATETABLE `_tmp_table`;'
```
This PR makes the [Sandbox site
plugin](https://wordpress.org/plugins/playground/) work in Playground
cc @bgrgicak @brandonpayton @wojtekn
There's more nuance to the type of the returned value than #117 antitipated. With that PR in, WooCommerce wouldn't work: ``` [06-Jun-2024 16:21:26 UTC] PHP Fatal error: Uncaught TypeError: get_object_vars(): Argument #1 ($object) must be of type object, string given in /wordpress/wp-includes/class-wpdb.php:3 Stack trace: #0 /wordpress/wp-includes/class-wpdb.php(3): get_object_vars('wp_wc_product_a...') #1 /wordpress/wp-content/plugins/woocommerce/src/Internal/ProductAttributesLookup/LookupDataStore.php(69): wpdb->get_var('SHOW TABLES LIK...') #2 /wordpress/wp-content/plugins/woocommerce/src/Internal/ProductAttributesLookup/LookupDataStore.php(650): Automattic\WooCommerce\Internal\ProductAttributesLookup\LookupDataStore->check_lookup_table_exists() ``` Let's revert and revisit later on cc @bgrgicak @wojtekn
…e` (#126) Fixes: #122 This fix will skip over tokens until it finds the closing bracket of the type modifier `)` instead of consuming exactly 3. This should account for data types that support more than one value as a modifier, such as: enum, float, decimal, double. Additionally, I've added support for enums. There are plugins that use enum fields and it makes sense to convert those to text in SQLite. ## Testing instructions Confirm the tests pass
In a table like ```sql CREATE TABLE `test` ( `id` INT, `name` VARCHAR(255), `other` VARCHAR(255), PRIMARY KEY (id), UNIQUE KEY (name) ) ``` the internal name for the unique key would be `(` because typically a key would be defined with ``` UNIQUE KEY `name` (name) ``` Usually this wrong key name is not a problem unless a `INSERT .. ON DUPLICATE KEY` statement is used where the resulting `ON CONFLICT( "keyname" )` would be transformed to `ON CONFLICT( "" )` which then fails. To fix this, this now derives a key name in case it is omitted.. cc @costasovo
Fixes: #123 In this PR i propose a refactor of the handling of `show create table` queries. The main issue was that it is possible for a primary key to be composite (more than one column). The existing implementation only supports a single PK which causes the `CREATE TABLE` statement to be invalid. An example of such a table is the `wp_term_relationships` found in WordPress: ```mysql -- original MySQL statement CREATE TABLE `wp_term_relationships` ( `object_id` bigint(20) unsigned NOT NULL DEFAULT 0, `term_taxonomy_id` bigint(20) unsigned NOT NULL DEFAULT 0, `term_order` int(11) NOT NULL DEFAULT 0, PRIMARY KEY (`object_id`,`term_taxonomy_id`), KEY `term_taxonomy_id` (`term_taxonomy_id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; -- statement produced by SQLite database integration CREATE TABLE wp_term_relationships ( `object_id` bigint(20) unsigned PRIMARY KEY AUTO_INCREMENT NOT NULL, `term_taxonomy_id` bigint(20) unsigned NOT NULL, `term_order` int(11) NOT NULL, KEY wp_term_relationships__term_taxonomy_id (term_taxonomy_id), UNIQUE KEY sqlite_autoindex_wp_term_relationships_1 (object_id, term_taxonomy_id) ); ``` SQLite also creates a unique key for any primary key (single or composite) which is something that is not necessary for MySQL and should be skipped from the final create statement. ## In this PR - Refactor the code to separate things into smaller methods. - Add the PK definition before the keys and remove it from the column definitions. - Use backticks around table names, column names and key identifiers. This is to prevent issues when names of identifiers are using reserved keywords, ensure case sensitivity and allow them to contain special chars or spaces. - Improve detection of AUTO_INCREMENT column. Now we falsely mark columns as AUTO_INCREMENT just because they happen to be the primary key and be of type integer. - Fix creating an invalid UNIQUE KEY for the PK. When we find that the unique key is identical to the primary key, we can skip the unique key as it is redundant. The final query output for `show create wp_term_relationships` looks as follows: ```mysql DROP TABLE IF EXISTS `wp_term_relationships`; CREATE TABLE `wp_term_relationships` ( `object_id` bigint(20) unsigned NOT NULL DEFAULT 0, `term_taxonomy_id` bigint(20) unsigned NOT NULL DEFAULT 0, `term_order` int(11) NOT NULL DEFAULT 0, PRIMARY KEY (`object_id`, `term_taxonomy_id`), KEY `wp_term_relationships__idx_term_taxonomy_id_x` (`term_taxonomy_id`) ); ``` ## Testing - Run the unit tests - Create a site with SQLite and then run `$wpdb->query('show create wp_term_relationships')` - The result should be a valid MySQL compatible query as shown above.
In this PR I propose that we always check if a wp core function is defined before attempting to use it, [as we do in the constructor of the Translator class](https://github.com/WordPress/sqlite-database-integration/blob/main/wp-includes/sqlite/class-wp-sqlite-translator.php#L564). Several hooks (do_action, apply_filters) are used in the `WP_SQLite_Translator` class. The class may be used in a context where WordPress is not loaded. We are currently working on adding support for SQLite to the WP Cli import and export commands and intend to use the translator to execute various SQL statements. Those commands run in a context where WordPress has not been loaded so `do_action` and `apply_filters` are unavailable.
Fixes: #124 This PR introduces some changes to how we determine the name of an index. SQLite requires that each index is uniquely named across the database (not just the table). This causes issues when multiple tables have the same key names when importing from MySQL. To avoid these issues, the index name is prefixed with the table name when it is created. This however results in the indexes having names that are too long for MySQL so when we run `show create table` the create statements have these prefixed keys that are too long. To solve this, we keep adding the table name as a prefix but we remove it when we generate the create statement. Furthermore, we ensure that the table name itself has no double underscores in it so we can be sure that we can split on `__` safely. The result is that each create table statement will have the original index name and prevents it from being too long.
See: #133 (comment) When creating new tables in SQLite, original index names are prefixed with the table name. This is because unlike MySQL, SQLite does not allow identical index names across the database. Adding a table prefix ensures the index names are unique. When running a `show create table` query, we restore the original index name by removing the table prefix. We split on double underscores but we did not take into account that the original index name can also contain `__`. This PR fixes an issue where we explode without a limit. We only want to explode on the first occurrence of `__`. Unit tests have been added to ensure this behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.