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

Use WC_Order_Factory to get order id #1656

Conversation

marcinkrzeminski
Copy link
Collaborator

@marcinkrzeminski marcinkrzeminski commented Dec 19, 2024

Fixes #1655

Describe your approach and how it fixes the issue.

Checklist

  • Project documentation has been updated to reflect the changes in this pull request, if applicable.
  • I have tested the changes in the local development environment (see contributing.md).
  • I have added phpunit tests.

Release Changelog

  • Fix: Use preferred WC_Order_Factory to get the order ID

Copy link
Contributor

@tharsheblows tharsheblows left a comment

Choose a reason for hiding this comment

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

There's another occurrence of this in the post status transition hook here -- should that one be updated also?

@@ -437,8 +437,8 @@ public function callback_deleted_post( $post_id ) {
return;
}

$order = new \WC_Order( $post->ID );
$order_title = esc_html__( 'Order number', 'stream' ) . ' ' . esc_html( $order->get_order_number() );
$order_id = \WC_Order_Factory::get_order_id( $post->ID );
Copy link
Contributor

Choose a reason for hiding this comment

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

❓question
I'm not sure on this -- WC_Order_Factory::get_order_id() will always return the post ID:

        /**
	 * Get the order ID depending on what was passed.
	 *
	 * @since 3.0.0
	 * @param  mixed $order Order data to convert to an ID.
	 * @return int|bool false on failure
	 */
	public static function get_order_id( $order ) {
		if ( false === $order ) {
			return self::get_global_order_id();
		} elseif ( is_numeric( $order ) ) {
			return $order;
		} elseif ( $order instanceof WC_Abstract_Order ) {
			return $order->get_id();
		} elseif ( ! empty( $order->ID ) ) {
			return $order->ID;
		} else {
			return false;
		}
	}

Is that the order ID now?

Although -- this is on the deleted_post hook so it should be ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tharsheblows good question, however I agree that we should be OK since this action is triggered on post deletion.

While I was working on a solution, I came across this comment on the abstract-wc-order.php, and that's why I used it.

* Get the order if ID is passed, otherwise the order is new and empty.
* This class should NOT be instantiated, but the wc_get_order function or new WC_Order_Factory
* should be used. It is possible, but the aforementioned are preferred and are the only
* methods that will be maintained going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcinkrzeminski Ah ha, thanks!

@marcinkrzeminski
Copy link
Collaborator Author

There's another occurrence of this in the post status transition hook here -- should that one be updated also?

Good catch. I've found another place and updated it as well to use the \WC_Order_Factory::get_order_id()

@@ -437,8 +437,8 @@ public function callback_deleted_post( $post_id ) {
return;
}

$order = new \WC_Order( $post->ID );
$order_title = esc_html__( 'Order number', 'stream' ) . ' ' . esc_html( $order->get_order_number() );
$order_id = \WC_Order_Factory::get_order_id( $post->ID );
Copy link
Contributor

Choose a reason for hiding this comment

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

@marcinkrzeminski Ah ha, thanks!

$order = new \WC_Order( $order_id );
$order_title = esc_html__( 'Order number', 'stream' ) . ' ' . esc_html( $order->get_order_number() );
$order_id = \WC_Order_Factory::get_order( $order_id );
$order_title = esc_html__( 'Order number', 'stream' ) . ' ' . esc_html( $order_id );
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 praise
Nice catch!

@marcinkrzeminski marcinkrzeminski merged commit 7ffd0b2 into develop Dec 23, 2024
1 check passed
@marcinkrzeminski marcinkrzeminski deleted the fix/1655-fatal-error-on-attempt-to-empty-the-trash-for-the-woocommerce-orders branch December 23, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatal error on attempt to empty the trash for the WooCommerce orders
2 participants