Deprecate the $check_key_exists parameter from AssetDataRegistry and disallow duplicate data for all cases #46139

nefeline · 2024-04-02T20:23:47Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Currently, AssetDataRegistry::add_data doesn't allow overriding existing data for a given key.

Still, the only method that calls it, AssetDataRegistry::add, has an optional param $check_key_exists with the default value set to false, which in practice doesn't have any impact in the end results, as whenever set to false, no data is overridden: in this case we have an early return combined with a fatal error triggered exclusively whenever WP_DEBUG is set to TRUE.

This logic has caused us a few problems, and after internal discussion, which happened on pdnLyh-5U0-p2, I'm proposing for us to deprecate the $check_key_exists parameter, as if we allow data override, quoting @senadir :

The structure of that data is considered public API, allowing it to be overridden opens the risk to having mismatched expectations between frontend code that’s consuming it and what was modified with. This is magnified by the fact that we evolved from passing simple primitives to objects in wcSettings.

Porting over @samueljseay 's comment here as well:

I also had a recent discussion in slack p1708450555264569/1708435510.631329-slack-C03CPM3UXDJ about this. In almost all cases we pass true to check key exists or we guard by checking if it exists before adding it. So I think we should as you say remove the param and always check or set the default to true. We could warn if the key exists, because it might be an indication your code is not working as expected?

Following Sam's suggestion, I've also updated the pre-existing logic that would throw a fatal exclusively whenever DEBUG is set to true to throw a PHP warning instead.

This is a change that should not have any impact on functionality, still, I'm tagging folks from the various teams to review this PR and test so we can confirm things are working as expected.

Closes #43404

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Ensure your WordPress install has WP_DEBUG set to true.
Access WooCommerce > Home > Customize Your Store > Start designing ( wp-admin/admin.php?page=wc-admin&path=%2Fcustomize-store%2Fassembler-hub )
Make sure you can access the pattern assembler without any problems: no fatal errors should be triggered.
Do a general smoke test including the Templates and the Cart, Mini-Cart, Checkout, Product Image, Product Collection, Reviews, and Filter blocks, and confirm that everything is working as expected: you should not see any warnings in your PHP error log.

Changelog entry

Automatically create a changelog entry from the details below.

Significance

Patch
Minor
Major

Type

Fix - Fixes an existing bug
Add - Adds functionality
Update - Update existing functionality
Dev - Development related task
Tweak - A minor adjustment to the codebase
Performance - Address performance issues
Enhancement - Improvement to existing functionality

Message

Deprecate the $check_key_exists parameter from AssetDataRegistry and disallow duplicate data for all cases.

Comment

…CAdminSharedSettings

…ing registered is not a string and whenever attempting to override existing data.

…mmerce

github-actions · 2024-04-03T14:26:50Z

Hi @albarin, @samueljseay, @nerrad, @gigitux, @senadir, @wavvves, @kmanijak, @woocommerce/rubik, @woocommerce/woo-fse

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

senadir

I'm approving initially on the premise of this PR, but there might be some unintended side-effect so let's wait and see if all tests pass.

wavvves

There seems to be a related test failure

Time: 07:40.002, Memory: 305.00 MB

There was 1 error:

1) Automattic\WooCommerce\Tests\Blocks\Assets\AssetDataRegistry::test_invalid_key_on_adding_data
array_key_exists(): The first argument should be either a string or an integer

/var/www/html/wp-content/plugins/woocommerce/src/Blocks/Assets/AssetDataRegistry.php:288
/var/www/html/wp-content/plugins/woocommerce/src/Blocks/Assets/AssetDataRegistry.php:310
/var/www/html/wp-content/plugins/woocommerce/tests/php/src/Blocks/Assets/AssetDataRegistry.php:53
phpvfscomposer:///var/www/html/wp-content/plugins/woocommerce/vendor/phpunit/phpunit/phpunit:[106](https://github.com/woocommerce/woocommerce/actions/runs/8540886005/job/23399405244?pr=46139#step:5:107)

--

There was 1 failure:

1) Automattic\WooCommerce\Tests\Blocks\Assets\AssetDataRegistry::test_already_existing_key_on_adding_data
Failed asserting that exception of type "InvalidArgumentException" is thrown.

samueljseay

Apart from the need to update the unit tests this is looking good to me. Thanks for including the warnings too 🙇🏻

…thods.

…est, considering we now return early whenever the key is duplicated, the code never reaches this stage via AssetDataRegistry:add

…mbler-is-not-loaded

nefeline · 2024-04-08T12:32:20Z

I did some additional changes here to account for the fact that we are not throwing an exception anymore 627b911

Also, I've moved the call to $this->exists to the add_data method to ensure all validations happen at that level and resolve the failing test, which was throwing a type error on array_key_exists instead of the expected warning 1fcb81c

@samueljseay would you help me out with a second review here? We want to open a CFE for this work, it would be great to have another final review here to confirm we are on track.

roykho · 2024-04-08T21:06:40Z

plugins/woocommerce/src/Blocks/Assets/AssetDataRegistry.php

-				throw $e;
-			}
-			wc_caught_exception( $e, __METHOD__, [ $key, $data ] );
+		if ( $check_key_exists ) {


For this since we're deprecating the parameter, what do you think if we check for isset instead. This way, the caller will get a deprecated feedback whether they passed 'true' or 'false' there.

For this since we're deprecating the parameter, what do you think if we check for isset instead. This way, the caller will get a deprecated feedback whether they passed 'true' or 'false' there.

Thanks for the review @roykho. This is a good point, and I agree it makes sense to check for isset, but given the urgency of this work, I'd like to propose working on this as part of a separate PR so we can unblock the release. What do you think?

Sure, if you don't mind please create a separate issue for it.

Thanks, absolutely! #46349

roykho

Left a comment which will be addressed at a later date. I've tested this PR with WP_DEBUG enabled and it fixes the fatal error. LGTM!

…disallow duplicate data for all cases (#46139) * Ensures data is added to the registry just once. * ditch unnecessary param. * Deprecate the param for AssetDataRegistry * Remove the param for additional calls to AssetDataRegistry::add() * Remove the param for additional call to AssetDataRegistry::add() on WCAdminSharedSettings * Trigger a PHP warning instead of throwing a fatal whenever the key being registered is not a string and whenever attempting to override existing data. * Add changefile(s) from automation for the following project(s): woocommerce * Replace trigger_error with error_log to clear out lint errors. * Address lint for trigger_error * Update the test_already_existing_key_on_adding_data test to expect a warning instead of an exception * Update tests * Update method from add_data to add. * Remove the throw InvalidArgumentException for the add and add_data methods. * Remove the now unnecessary test_already_existing_key_on_adding_data test, considering we now return early whenever the key is duplicated, the code never reaches this stage via AssetDataRegistry:add * Move the call to the exists method to within the add_data method. --------- Co-authored-by: github-actions <github-actions@github.com>

nefeline added 5 commits March 28, 2024 16:55

Ensures data is added to the registry just once.

7183dca

ditch unnecessary param.

7228fde

Deprecate the param for AssetDataRegistry

f6a0758

Remove the param for additional calls to AssetDataRegistry::add()

48142ca

Remove the param for additional call to AssetDataRegistry::add() on W…

3f5847b

…CAdminSharedSettings

nefeline linked an issue Apr 2, 2024 that may be closed by this pull request

CYS: When WP_DEBUG is enabled, the assembler is not loaded #43404

Closed

5 tasks

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Apr 2, 2024

nefeline and others added 3 commits April 3, 2024 10:50

Trigger a PHP warning instead of throwing a fatal whenever the key be…

534082e

…ing registered is not a string and whenever attempting to override existing data.

Add changefile(s) from automation for the following project(s): wooco…

612c40a

…mmerce

Resolve conflict & merge latest trunk

6c673f3

nefeline marked this pull request as ready for review April 3, 2024 14:20

nefeline requested review from a team, kmanijak, wavvves, samueljseay, senadir, nerrad, albarin and gigitux and removed request for a team April 3, 2024 14:21

senadir reviewed Apr 3, 2024

View reviewed changes

nefeline added 2 commits April 3, 2024 11:51

Replace trigger_error with error_log to clear out lint errors.

1500045

Address lint for trigger_error

ab6a5ab

wavvves reviewed Apr 3, 2024

View reviewed changes

samueljseay approved these changes Apr 4, 2024

View reviewed changes

nefeline added 8 commits April 5, 2024 16:12

Merge trunk and solve conflicts.

787867e

Update tests

c69c321

Update method from add_data to add.

9fd2532

Remove the throw InvalidArgumentException for the add and add_data me…

627b911

…thods.

Remove the now unnecessary test_already_existing_key_on_adding_data t…

1fa885c

…est, considering we now return early whenever the key is duplicated, the code never reaches this stage via AssetDataRegistry:add

Merge branch 'trunk' into 43404-cys-when-wp_debug-is-enabled-the-asse…

00ad85a

…mbler-is-not-loaded

Move the call to the exists method to within the add_data method.

1fcb81c

Resolve conflict and merge latest trunk

627b6ad

roykho reviewed Apr 8, 2024

View reviewed changes

roykho approved these changes Apr 8, 2024

View reviewed changes

nefeline mentioned this pull request Apr 8, 2024

Use isset to address the deprecated message related to $check_key_exists parameter from AssetDataRegistry #46349

Open

nefeline merged commit 14c06ff into trunk Apr 8, 2024
50 of 53 checks passed

nefeline deleted the 43404-cys-when-wp_debug-is-enabled-the-assembler-is-not-loaded branch April 8, 2024 21:48

github-actions bot added this to the 8.9.0 milestone Apr 8, 2024

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Apr 8, 2024

Stojdza modified the milestones: 8.9.0, 8.8.0 Apr 8, 2024

nefeline mentioned this pull request Apr 10, 2024

[CYS] Investigate issue preventing users to proceed to the pattern assembler #44916

Closed

rodelgc added a commit that referenced this pull request Apr 12, 2024

Add changelog entry of #46139 into readme.txt

b89f019

rodelgc mentioned this pull request Apr 12, 2024

Add changelog entry of #46139 into readme.txt #46546

Merged

rodelgc added a commit that referenced this pull request Apr 12, 2024

Add changelog entry of #46139 into readme.txt (#46546)

a66e866

Jinksi mentioned this pull request Apr 18, 2024

GH checks fails since WC 8.9 release – WC compatibility (latest, latest, latest, 7.4) and Code coverage Automattic/woocommerce-payments#8662

Closed

timur27 mentioned this pull request Apr 18, 2024

misc: remove deprecated param from asset data registry Automattic/woocommerce-payments#8666

Merged

6 tasks

nefeline mentioned this pull request Apr 30, 2024

I get this warning from woocommerce: Deprecate, It is on all my pages, and has disbaled the use of elementor #46744

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate the $check_key_exists parameter from AssetDataRegistry and disallow duplicate data for all cases #46139

Deprecate the $check_key_exists parameter from AssetDataRegistry and disallow duplicate data for all cases #46139

nefeline commented Apr 2, 2024 •

edited

github-actions bot commented Apr 3, 2024 •

edited

senadir left a comment

wavvves left a comment •

edited

samueljseay left a comment

nefeline commented Apr 8, 2024

roykho Apr 8, 2024

nefeline Apr 8, 2024

roykho Apr 8, 2024

nefeline Apr 8, 2024

roykho left a comment

Deprecate the $check_key_exists parameter from AssetDataRegistry and disallow duplicate data for all cases #46139

Deprecate the $check_key_exists parameter from AssetDataRegistry and disallow duplicate data for all cases #46139

Conversation

nefeline commented Apr 2, 2024 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Changelog entry

Significance

Type

Message

Comment

github-actions bot commented Apr 3, 2024 • edited

senadir left a comment

Choose a reason for hiding this comment

wavvves left a comment • edited

Choose a reason for hiding this comment

samueljseay left a comment

Choose a reason for hiding this comment

nefeline commented Apr 8, 2024

roykho Apr 8, 2024

Choose a reason for hiding this comment

nefeline Apr 8, 2024

Choose a reason for hiding this comment

roykho Apr 8, 2024

Choose a reason for hiding this comment

nefeline Apr 8, 2024

Choose a reason for hiding this comment

roykho left a comment

Choose a reason for hiding this comment

nefeline commented Apr 2, 2024 •

edited

github-actions bot commented Apr 3, 2024 •

edited

wavvves left a comment •

edited