Add checks and unit tests to Transformers #44634

chihsuan · 2024-02-15T07:59:55Z

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:

Partially close #44249

Related to #44448

This PR audits every PHP class that implements TransformerInterface to prevent fatal errors and ensure PHP 8+ compliance.

Changes

Apply defensive programming principles - add type checks to prevent fatal errors
Add unit tests

How to test the changes in this Pull Request:

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

Please review the changes and unit tests carefully to ensure that the changes are correct and don't introduce any regressions.

In your environment, use PHP 8.1+
If using an existing site, clear transients by running wp transient delete --all
Ensure WooCommerce > Settings > Advanced > Woo.com > Display suggestions within WooCommerce is checked
Go to Setup Wizard, /wp-admin/admin.php?page=wc-admin&path=/setup-wizard
Click continue
Select I'm just starting my business and continue
Select Australia — Australian Capital Territory and continue
Skip plugin installation
Observe no fatal when loading homescreen
Go to Tools > WCA Test Helper > Tools > Run wc_admin_daily job
Ensure the job is run successfully

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

Comment

chihsuan · 2024-02-15T08:04:46Z

plugins/woocommerce/src/Admin/RemoteInboxNotifications/Transformers/PrepareUrl.php

+		if ( ! is_string( $value ) ) {
+			return $default;
+		}
+
 		$url_parts = wp_parse_url( rtrim( $value, '/' ) );


https://developer.wordpress.org/reference/functions/wp_parse_url/

Return
mixed False on parse failure; Array of URL components on success; When a specific component has been requested: null if the component doesn’t exist in the given URL; a string or – in the case of PHP_URL_PORT – integer when it does. See parse_url()’s return values.

github-actions · 2024-02-15T08:06:30Z

Hi @ilyasfoo, @moon0326, @woocommerce/ghidorah

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

chihsuan · 2024-02-15T08:08:31Z

plugins/woocommerce/src/Admin/RemoteInboxNotifications/Transformers/ArrayColumn.php

+			null !== $arguments->key &&
+			! is_string( $arguments->key ) &&
+			! is_int( $arguments->key )
+		) {


https://www.php.net/manual/en/function.array-column.php

array_column(array $array, int|string|null $column_key, int|string|null $index_key = null): array

column_key
The column of values to return. This value may be an integer key of the column you wish to retrieve, or it may be a string key name for an associative array or property name. It may also be null to return complete arrays or objects (this is useful together with index_key to reindex the array).

github-actions · 2024-02-15T08:20:02Z

Test Results Summary

Commit SHA: c819b75

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 36s
E2E Tests	322	0	0	3	0	325	8m 59s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

moon0326 · 2024-02-15T21:51:02Z

plugins/woocommerce/src/Admin/RemoteInboxNotifications/Transformers/ArrayKeys.php

@@ -21,6 +21,10 @@ class ArrayKeys implements TransformerInterface {
 	 * @return mixed
 	 */
 	public function transform( $value, stdClass $arguments = null, $default = null ) {
+		if ( ! is_array( $value ) ) {
+			return $default;


What do you think of setting the default to an empty array?

moon0326 · 2024-02-15T21:51:59Z

plugins/woocommerce/src/Admin/RemoteInboxNotifications/Transformers/ArrayValues.php

@@ -21,6 +21,10 @@ class ArrayValues implements TransformerInterface {
 	 * @return mixed
 	 */
 	public function transform( $value, stdClass $arguments = null, $default = null ) {
+		if ( ! is_array( $value ) ) {
+			return $default;


Same as above. What do you think of setting the default value to an empty array? I think it makes more sense as the caller expects the return value to be an array. It also aligns with the built-in array_values function.

Good point! @moon0326 That makes sense to me. I'll update the default value to an empty array. 🙂

Changed in 1eb5627. I also updated default value for ArrayFlatten.

moon0326

I've left a few minor comments, but LGTM! 👍

moon0326

Changes LGTM! 👍 🚀

ilyasfoo

Changes look great! I have a minor comment but pre-approving

ilyasfoo · 2024-02-16T03:48:05Z

plugins/woocommerce/src/Admin/RemoteInboxNotifications/Transformers/ArrayFlatten.php

@@ -20,7 +20,11 @@ class ArrayFlatten implements TransformerInterface {
 	 *
 	 * @return mixed|null
 	 */
-	public function transform( $value, stdClass $arguments = null, $default = null ) {
+	public function transform( $value, stdClass $arguments = null, $default = array() ) {


It seems ArrayFlatten, ArrayKeys, and ArrayValues default is set to array(), but in ArrayColumn and ArraySearch it's null. Do you have any reason on why we treat these transformers differently? Otherwise, I think it's better to be consistent.

@ilyasfoo Thanks for pointing that out. I missed that for ArrayColumn and just updated it in c819b75 . I think we should keep the default value of ArraySearch null because we are returning null when value is not found. Does that sound good to you?

Sounds good, thanks!

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

chihsuan self-assigned this Feb 15, 2024

chihsuan marked this pull request as ready for review February 15, 2024 08:00

chihsuan commented Feb 15, 2024

View reviewed changes

chihsuan requested review from a team, ilyasfoo and moon0326 February 15, 2024 08:06

chihsuan commented Feb 15, 2024

View reviewed changes

moon0326 reviewed Feb 15, 2024

View reviewed changes

chihsuan added 4 commits February 16, 2024 10:21

Add checks and unit tests to Transformers

15c1aa5

Add changelog

27a2d71

Update docs

c1553f2

Fix test

c72005f

chihsuan force-pushed the dev/audit-transformaters branch from f01824a to c72005f Compare February 16, 2024 02:21

Set the default value to an empty array

1eb5627

moon0326 approved these changes Feb 16, 2024

View reviewed changes

ilyasfoo approved these changes Feb 16, 2024

View reviewed changes

Set default value to array() for ArrayColumn

c819b75

chihsuan merged commit 75118fb into trunk Feb 16, 2024
36 checks passed

chihsuan deleted the dev/audit-transformaters branch February 16, 2024 08:51

github-actions bot added this to the 8.7.0 milestone Feb 16, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checks and unit tests to Transformers #44634

Add checks and unit tests to Transformers #44634

chihsuan commented Feb 15, 2024 •

edited

chihsuan Feb 15, 2024

github-actions bot commented Feb 15, 2024

chihsuan Feb 15, 2024

github-actions bot commented Feb 15, 2024 •

edited

moon0326 Feb 15, 2024

moon0326 Feb 15, 2024 •

edited

chihsuan Feb 16, 2024

chihsuan Feb 16, 2024

moon0326 left a comment

moon0326 left a comment

ilyasfoo left a comment

ilyasfoo Feb 16, 2024

chihsuan Feb 16, 2024 •

edited

ilyasfoo Feb 16, 2024

Add checks and unit tests to Transformers #44634

Add checks and unit tests to Transformers #44634

Conversation

chihsuan commented Feb 15, 2024 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

Changes

How to test the changes in this Pull Request:

Changelog entry

Significance

Type

Message

Comment

chihsuan Feb 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 15, 2024

chihsuan Feb 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 15, 2024 • edited

Test Results Summary

moon0326 Feb 15, 2024

Choose a reason for hiding this comment

moon0326 Feb 15, 2024 • edited

Choose a reason for hiding this comment

chihsuan Feb 16, 2024

Choose a reason for hiding this comment

chihsuan Feb 16, 2024

Choose a reason for hiding this comment

moon0326 left a comment

Choose a reason for hiding this comment

moon0326 left a comment

Choose a reason for hiding this comment

ilyasfoo left a comment

Choose a reason for hiding this comment

ilyasfoo Feb 16, 2024

Choose a reason for hiding this comment

chihsuan Feb 16, 2024 • edited

Choose a reason for hiding this comment

ilyasfoo Feb 16, 2024

Choose a reason for hiding this comment

chihsuan commented Feb 15, 2024 •

edited

github-actions bot commented Feb 15, 2024 •

edited

moon0326 Feb 15, 2024 •

edited

chihsuan Feb 16, 2024 •

edited