PHP 8.2 Deprecation Warnings: Dynamic Properties on Order

KratosGemini commented 1 month ago

Describe the bug

When running on PHP 8.2, there are some deprecation warnings that revolve around dynamic properties added to WC_Order. According to the comments on WC_Order_Square, that class was added to address the deprecation warnings. However, this seems to not be working. Maybe it's not fully implemented yet?

To reproduce

Make sure deprecation warnings are logged and you are running PHP 8.2+.
Submit an order using Square.
View the deprecation warnings in the log.

Expected behavior

The code avoids using dynamic properties so it does not break in a future version of PHP.

Environment (please complete the following information):

WordPress Version: 6.6.2
WooCommerce Version: 9.3.1
WooCommerce Square Plugin Version: 4.8.0
Browser [e.g. chrome, safari] and Version: N/A

Additional details

As I'm not familiar with extending WooCommerce, I'm not sure what the best solution is (there are certainly different ways to go about it).

Here's an old discussion from 2018, so I'm not sure if it still applies: https://github.com/woocommerce/woocommerce/issues/19058

Someone asked a question about tackling a similar problem recently here, but the discussion hasn't gone anywhere yet: https://github.com/woocommerce/woocommerce/discussions/43367

System Status Report

``` ### WordPress Environment ### WordPress address (URL): [Redacted] Site address (URL): [Redacted] WC Version: 9.3.1 Legacy REST API Package Version: The Legacy REST API plugin is not installed on this site. Action Scheduler Version: ✔ 3.8.1 Log Directory Writable: ✔ WP Version: 6.6.2 WP Multisite: – WP Memory Limit: 512 MB WP Debug Mode: ✔ WP Cron: – Language: en_US External object cache: – ### Server Environment ### Server Info: Apache/2.4.52 (Ubuntu) PHP Version: 8.2.23 PHP Post Max Size: 25 MB PHP Time Limit: 30 PHP Max Input Vars: 1000 cURL Version: 7.81.0 OpenSSL/3.0.2 SUHOSIN Installed: – MySQL Version: 10.6.18-MariaDB-0ubuntu0.22.04.1 Max Upload Size: 25 MB Default Timezone is UTC: ✔ fsockopen/cURL: ✔ SoapClient: ❌ Your server does not have the SoapClient class enabled - some gateway plugins which use SOAP may not work as expected. DOMDocument: ✔ GZip: ✔ Multibyte String: ✔ Remote Post: ✔ Remote Get: ✔ ```

peterwilsoncc commented 1 month ago

@dkotter Referred me to an earlier PR which used a very similar approach to the one I created yesterday. It was ultimately closed without merging after agreeing this was best solved by allowing custom data in the WC Core classes.

I think this is blocked until a core solution can be found as @james-allan made some excellent points about the risks involved with solving the issue by replacing the WC_Order class with a custom one. I think the upstream order objects need a way of setting meta-like data that is not saved to the database.

KratosGemini commented 1 month ago

@peterwilsoncc Thanks for taking a look at it.

I read through the myriad of linked PRs and Issues on the subject. Among them is someone who ran into a similar issue on one of his projects. The issue discusses a handful of potential solutions (all but one requires a WC Core change). In the end, he went with the solution that does not require a WC Core change since that seemed unlikely at the time (not sure if anything has changed since then). Here are links to a couple of his comments describing the solution he went with:

Since I don't see any open issues in WC Core to address this use case, is it worth just moving ahead with the above solution?

peterwilsoncc commented 1 month ago

@KratosGemini I'm happy to do so if y'all have decided that's the preferred approach since #20 was closed without the change.

I'm guessing James's concerns at the time was that if both the Square and another gateway required the changes and applied similar fixes then one of the filters would win and the deprecation errors return to the other gateway. This would result in deprecation errors showing in the logs against Square when they were triggered by woocommerce-some-other-gateway.

@james-allan Have your views on how to fix this changed since this discussion https://github.com/woocommerce/woocommerce-square/pull/20#pullrequestreview-1849741249

peterwilsoncc commented 1 month ago

Sorry, premature send...

To allow for multiple gateways needing dynamic properties, I'm happy to work on an upstream PR to manage it in WC_Order. Something like this but with additional protections to handle unset properties etc.

class WC_Order ... {
   /**
    * Dynamic Order Properties used by gateways.
    *
    * @var array[]
    */
    $gateway_properties = [];

    public function set_gateway_property( $gateway, $property, $value ) {
       $this->gateway_properties[ $gateway ][ $property ] = $value;
    }

    public function get_gateway_property( ... ) {
       return $this->gateway_properties[ $gateway ][ $property ];
    }
}

james-allan commented 1 month ago

@james-allan Have your views on how to fix this changed since this discussion https://github.com/woocommerce/woocommerce-square/pull/20#pullrequestreview-1849741249

No, I think my thoughts haven't really changed on the approach taken in that PR. Overriding the global order object with a custom order class to get around the dynamic properties rule change in PHP 8.2 feels hacky and I have no doubt would lead to issues down the track.

WC Core already do that in an admin context and that order class has unique functions used in an admin context so Square would need to do something like this (see code below) to make sure we don't ever remove any functionality or worse, cause fatals.

class WC_Square_Order extends WC_Order {
    // Custom functionality for Square orders
}

class WC_Square_Admin_Order extends \Automattic\WooCommerce\Admin\Overrides\Order {
    // Custom functionality for Square admin orders
}

add_filter( 'woocommerce_order_class', 'load_custom_order_class', 999 );

public function load_custom_order_class( $classname ) {
    switch ( $classname ) {
        case 'WC_Order':
            return 'WC_Square_Order'; // Custom class for normal orders

        case '\Automattic\WooCommerce\Admin\Overrides\Order':
            return 'WC_Square_Admin_Order'; // Custom class for admin orders

        default:
            /**
             * We don't recognise the order type and so it's custom and we have no way of knowing if we can
             * override it or not. In this scenario the Dynamic Properties errors may just continue to happen. 
             */
            return $classname; 
    }
}

As I mentioned above, there's a risk that this fix would just fall over if anyone else ever registered a custom order object class.

Reading the very lenthy debate on https://github.com/woocommerce/woocommerce/issues/31316, it looks like the generally accepted approach is to continue using dynamic properties on PHP versions pre 8.2 and use a WeakMap on php 8.2+.

To allow for multiple gateways needing dynamic properties, I'm happy to work on an upstream PR to manage it in WC_Order.

That would resolve the need to override the base classes, but I'm not convinced that WC core would accept that kind of change. They would have to accept that there's an inherent need for gateways to set dynamic properties and other plugins don't -- and I don't believe that's true. They might as well add a general array of key values that folks could use to set this sort of thing and they aren't likely to do that.

KratosGemini commented 1 month ago

@james-allan

...They might as well add a general array of key values that folks could use to set this sort of thing and they aren't likely to do that.

Would the fact that the below discussion about this kind of issue doesn't have a proposed solution help that case get accepted?

https://github.com/woocommerce/woocommerce/discussions/43367

It seems to be something people run into at times as PHP 8.2 becomes more widely used. Including WooCommerce Square here, there are 3 documented cases of needing this functionality in the issues/discussions linked in this thread alone. Granted, one wrote their own solution using WeakMap because WC Core didn't have a solution for it at the time.

woocommerce / woocommerce-square