woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
47 stars 21 forks source link

WordPress 6.7 Compatibility: Avoid errors in the database where a TEXT type can't have a default value #2667

Closed eason9487 closed 1 week ago

eason9487 commented 2 weeks ago

Changes proposed in this Pull Request:

There are database errors when running PHP Unit Tests with WordPress 6.7-RC3.

WordPress database error BLOB, TEXT, GEOMETRY or JSON column 'categories' can't have a default value for query
ALTER TABLE wptests_gla_attribute_mapping_rules ALTER COLUMN `categories` SET DEFAULT ''
made by include('phpvfscomposer:///home/runner/work/google-listings-and-ads/google-listings-and-ads/vendor/phpunit/phpunit/phpunit'),
PHPUnit\TextUI\Command::main,
PHPUnit\TextUI\Command->run,
PHPUnit\TextUI\TestRunner->run,
PHPUnit\Framework\TestSuite->run,
PHPUnit\Framework\TestSuite->run,
PHPUnit\Framework\TestSuite->run,
PHPUnit\Framework\TestCase->run,
PHPUnit\Framework\TestResult->run,
PHPUnit\Framework\TestCase->runBare,
Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\Integration\WPCOMProxyTest->setUp,
Automattic\WooCommerce\GoogleListingsAndAds\DB\Table->install,
Automattic\WooCommerce\GoogleListingsAndAds\Proxies\WP->db_delta,
dbDelta

Considering the null categories will be sanitized to '' before inserting it into database, it may not be an issue to change the categories column in the gla_attribute_mapping_rules table to:

https://github.com/woocommerce/google-listings-and-ads/blob/4535b7124ca3ecd256c006945342d3faf983ca85/src/DB/Query/AttributeMappingRulesQuery.php#L42-L44

Screenshots:

📷 Before

image

📷 After

image

Detailed test instructions:

  1. Alter the table if needed
    • Before fix
      ALTER TABLE `wp_gla_attribute_mapping_rules` MODIFY COLUMN `categories` text DEFAULT '';
    • After fix
      ALTER TABLE `wp_gla_attribute_mapping_rules` MODIFY COLUMN `categories` text NOT NULL;
  2. Go to the plugin's Attributes page.
  3. Create and update some attribute rules to see if its CRUD work well.
  4. View the runs of PHP Unit Tests on GitHub Actions to confirm the errors no longer occur.

Changelog entry

Tweak - WordPress 6.7 Compatibility: Avoid errors in the database where a TEXT type can't have a default value.

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.4%. Comparing base (0383264) to head (a29a031). Report is 4 commits behind head on develop.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2667/graphs/tree.svg?width=650&height=150&src=pr&token=UROWUPF1LX&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2667?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) ```diff @@ Coverage Diff @@ ## develop #2667 +/- ## ============================================ + Coverage 62.7% 65.4% +2.8% - Complexity 0 4596 +4596 ============================================ Files 319 474 +155 Lines 5074 19268 +14194 Branches 1231 0 -1231 ============================================ + Hits 3180 12610 +9430 - Misses 1721 6658 +4937 + Partials 173 0 -173 ``` | [Flag](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2667/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [js-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2667/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `?` | | | [php-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2667/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `65.4% <ø> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2667?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [src/DB/Table/AttributeMappingRulesTable.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2667?src=pr&el=tree&filepath=src%2FDB%2FTable%2FAttributeMappingRulesTable.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL0RCL1RhYmxlL0F0dHJpYnV0ZU1hcHBpbmdSdWxlc1RhYmxlLnBocA==) | `46.7% <ø> (ø)` | | ... and [792 files with indirect coverage changes](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2667/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)
eason9487 commented 1 week ago

Hi @mikkamp, thanks for the review!

Although can you clarify how you are running into the error?

Originally, I haven't reproduced the issue locally either. After changing my local DB to MySQL 8.0.39, I can reproduce it.

So if the error is coming from MySQL I'm just wondering how this is specifically related to WP 6.7 and wouldn't occur in older versions of WP? I'm also not finding anything in WP 6.7 that made these checks stricter.

It was firstly noticed on GitHub Actions. After testing with different combinations of WP and WC versions, the error occurs when using WordPress 6.7-RC3. It won't make the testing fail but only output error messages: https://github.com/woocommerce/google-listings-and-ads/actions/runs/11701884828/job/32588901868#step:6:29

After further research, it appears that WordPress 6.7 changed wp-admin/includes/upgrade.ph to strict comparison, which might make this comparison produce a different result.

CREATE TABLE `wp_gla_attribute_mapping_rules` (
    `id` bigint(20) NOT NULL AUTO_INCREMENT,
    `attribute` varchar(255) NOT NULL,
    `source` varchar(100) NOT NULL,
    `category_condition_type` varchar(10) NOT NULL,
    `categories` text DEFAULT '',
    PRIMARY KEY `id` (`id`)
);

In the original loose comparison, the logical comparison matched from the DEFAULT '' statement in the above table creation SQL will be NULL != '', changing it to a strict comparison produces a different result, which is then later executed:

ALTER TABLE wp_gla_attribute_mapping_rules ALTER COLUMN `categories` SET DEFAULT '';

Therefore, I believe this PR could be categorized as a WordPress 6.7 compatibility adjustment.

mikkamp commented 1 week ago

Thanks for the clarification, that makes sense. I was looking for actual failures in the test runs, but I can see the output mixed in with the test results now.