upkeep/1170 Bump PHP "tested up to" version 8.3 #20
Conversation
[19-Dec-2023 11:57:26 UTC] PHP Deprecated: Creation of dynamic property Automattic\WooCommerce\Admin\Overrides\Order::$payment_total is deprecated in /Users/faisalalvi/LocalSites/wpne/app/public/wp-content/plugins/woocommerce-square/includes/Framework/PaymentGateway/Payment_Gateway.php on line 1628 [19-Dec-2023 11:57:26 UTC] PHP Deprecated: Creation of dynamic property Automattic\WooCommerce\Admin\Overrides\Order::$payment is deprecated in /Users/faisalalvi/LocalSites/wpne/app/public/wp-content/plugins/woocommerce-square/includes/Framework/PaymentGateway/Payment_Gateway.php on line 1638 [19-Dec-2023 11:57:26 UTC] PHP Deprecated: Creation of dynamic property Automattic\WooCommerce\Admin\Overrides\Order::$description is deprecated in /Users/faisalalvi/LocalSites/wpne/app/public/wp-content/plugins/woocommerce-square/includes/Framework/PaymentGateway/Payment_Gateway.php on line 1644 [19-Dec-2023 11:57:26 UTC] PHP Deprecated: Creation of dynamic property Automattic\WooCommerce\Admin\Overrides\Order::$unique_transaction_ref is deprecated in /Users/faisalalvi/LocalSites/wpne/app/public/wp-content/plugins/woocommerce-square/includes/Gateway.php on line 1349 [19-Dec-2023 11:57:26 UTC] PHP Deprecated: Creation of dynamic property Automattic\WooCommerce\Admin\Overrides\Order::$square_customer_id is deprecated in /Users/faisalalvi/LocalSites/wpne/app/public/wp-content/plugins/woocommerce-square/includes/Gateway.php on line 355 [19-Dec-2023 11:57:26 UTC] PHP Deprecated: Creation of dynamic property Automattic\WooCommerce\Admin\Overrides\Order::$square_order_id is deprecated in /Users/faisalalvi/LocalSites/wpne/app/public/wp-content/plugins/woocommerce-square/includes/Gateway.php on line 356 [19-Dec-2023 11:57:26 UTC] PHP Deprecated: Creation of dynamic property Automattic\WooCommerce\Admin\Overrides\Order::$square_version is deprecated in /Users/faisalalvi/LocalSites/wpne/app/public/wp-content/plugins/woocommerce-square/includes/Gateway.php on line 357
Sidsector9
force-pushed
the
upkeep/1170
branch
from
a589e8f to
66f3963
Compare
QA Update
|
NotePlease request the Woo team for review to verify on the approach before releasing. cc: @vikrampm1 @dkotter |
@Sidsector9 I tried again several times but now the notices (Payment_Gateway.php / Gateway.php) are not coming. |
| * @return string | ||
| */ | ||
| public function load_custom_order_class( $classname ) { | ||
| if ( '\Automattic\WooCommerce\Admin\Overrides\Order' === $classname ) { |
There was a problem hiding this comment.
@woocommerce/quark requesting you to review this approach taken to resolve the dynamic properties notice.
There was a problem hiding this comment.
hmm it's not ideal. Seems like a bit of an arms race and potentially flaky if other folks need to do something similar.
For example, WC core are already doing this for some purpose I don't fully understand but it seems to add functionality that might break if it's overridden further.
Having said that, I don't know if there's an alternative other than a rethink of how the Square plugin uses these dynamic properties given the current environment. From the discussion on this issue (woocommerce/woocommerce#31316) and this PR (woocommerce/woocommerce#35763), they don't seem open to broadly rolling out the AllowDynamicProperties flag across the core WC classes.
There was a problem hiding this comment.
@diegocurbelo what's your thoughts on this then, acceptable as-is or someone else to consult on most optimal approach here?
There was a problem hiding this comment.
Note that this just resolves some deprecated notices in PHP 8.2+ so if we're unsure on this approach, we could remove this for now and come back later.
Eventually those deprecated notices will be warnings or errors I imagine so something that needs fixed but isn't the most crucial thing at the moment.
There was a problem hiding this comment.
Hmm, interesting discussion on those WooCommerce issues and I agree with @james-allan that this change isn't ideal... I ultimately don't see an alternative to this problem though and the workarounds others shared on those issues don't seem ideal either 🤔
The only suggestion I was going to make was rather than filtering every single woocommerce_order_class hook, could we get away with limiting this to just our get_order(), get_order_for_capture() and get_order_for_refund() functions to use WC_Order_Square when we need it?
i.e.
if ( is_numeric( $order ) ) {
/** @var WC_Order_Square $order */
add_filter( 'woocommerce_order_class', array( $this, 'load_custom_order_class' ), 999 );
$order = wc_get_order( $order );
remove_filter( 'woocommerce_order_class', array( $this, 'load_custom_order_class' ), 999 );
}
Or would this potentially introduce other problems?
There was a problem hiding this comment.
Or would this potentially introduce other problems?
I don't have an answer for this 😓. We can give this a try but it will be tough to imagine for what particular configuration(s) this can fail.
@jeffpaul there is still an option to refactor the plugin which will be some decent amount of work, both development and testing wise. Would you suggest we take this approach?
Sidsector9
requested review from
a team and
james-allan
and removed request for
a team
|
@james-allan noting that this PR is awaiting your review. |
|
Closing in favour of PR #71 |
All Submissions:
Changes proposed in this Pull Request:
This PR simply includes a note regarding the "PHP tested up to: 8.3". While this information may not have direct utility for WordPress/WP.org, it is an internally decided retaining it would be nice to have.
The following steps are tested in the PHP 8.3 environment.
The following warnings(s) were noticed and fixed. ✅
While performing the test, the following warnings were reported that were coming from the WooCommerce.⚠️
Closes #42.
Steps to test the changes in this Pull Request:
Make sure the plugin works fine in the PHP 8.3 environment and there are 0 errors/warnings reported in the error logs while/after performing the test steps mentioned above.
For the QA
Please test the following:
Changelog entry