Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Dynamic Properties Deprecations in Order Objects. #222

Closed
wants to merge 3 commits into from

Conversation

peterwilsoncc
Copy link
Contributor

All Submissions:

  • Does your code follow the WooCommerce Sniffs variant of WordPress coding standards?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Will this change require new documentation or changes to existing documentation?

Changes proposed in this Pull Request:

Fixes and issue in which deprecation notices are thrown in PHP 8.2+ due to the use of dynamic properties.

  1. Introduces WooCommerce\Square\WC_Order_Admin_Override_Square to extend Automattic\WooCommerce\Admin\Overrides\Order to include the properties.
  2. Introduces a filter to override WC_Order and Automattic\WooCommerce\Admin\Overrides\Order.

Closes #219 .

Steps to test the changes in this Pull Request:

  1. Create a simple product
  2. Enable logging of deprecation notices
    define( 'WP_DEBUG', true );
    define( 'WP_DEBUG_LOG', '/path/to/debug.log' );
    define( 'WP_DEBUG_DISPLAY', true );
    
  3. Ensure the store is configured to use blocks for the cart and checkout pages
  4. Purchase the new product
  5. Configure the store to use shortcodes for the cart and checkout pages.
  6. Purchase the product
  7. In the admin, process a refund using square for one of the orders.
  8. Activate WooCommerce Subscriptions
  9. Create a subscription product
  10. Purchase the product
  11. Go to my account
  12. View the subscription product
  13. Click renew now in the actions
  14. Go to the subscription in the Dashboard (as a store admin)
  15. Select Renew in the actions dropdown; submit the review.

Check the logs. Confirm there are no deprecation warnings for the creation of dynamic properties.

There may be some deprecation notices from Subscriptions relating to the use of Automattic\WooCommerce\Admin\Features\Navigation classes -- these are unrelated to this issue.

Changelog entry

Fix - Prevent deprecation notices creating orders in PHP 8.2 and later.

…der` class.

This override is to account for additional properties required by the Square gateway that are not defined on the original class.
@peterwilsoncc peterwilsoncc self-assigned this Sep 23, 2024
*
* Runs on the hook 'woocommerce_order_class, 20'.
*
* @since x.x.x
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to be updated with version number prior to merge.

*
* This class is used to add additional properties to the order object.
*
* @since x.x.x
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

@@ -140,6 +140,36 @@ public function __construct() {
add_action( 'action_scheduler_init', array( $this, 'schedule_token_migration_job' ) );
add_action( 'wc_square_init_payment_token_migration_v2', array( $this, 'register_payment_tokens_migration_scheduler' ) );
add_action( 'wc_square_init_payment_token_migration', '__return_false' );

add_filter( 'woocommerce_order_class', array( $this, 'order_class' ), 20 );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better location for this and the associated method?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you saw the previous PR but we originally took a similar approach to solve this in #20. I haven't compared this line by line but I think they are pretty similar. We ended up closing that out (and basically leaving the deprecation notices) due to the feedback we received. Can you can a look at that PR and the comments there to see if the approach here is different enough to try again on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter I'll take a look, I went looking yesterday but couldn't find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed James's comments on the original PR and the upstream tickets.

Given the concerns about a race to the bottom, I think we're blocked until it's addressed upstream. Probably in WC_Data with either magic methods or a setter/getter for unsaved meta data.

@peterwilsoncc
Copy link
Contributor Author

Closing per comment on ticket and review above.

@peterwilsoncc peterwilsoncc deleted the fix/219-dynamic-order-class-properties branch September 24, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 8.2 Deprecation Warnings: Dynamic Properties on Order
2 participants