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.

Sign up for GitHub

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

Jump to bottom

Check order type is set before returning to prevent notice. #35349

Merged
merged 5 commits into from
Oct 31, 2022
Merged

Check order type is set before returning to prevent notice. #35349

merged 5 commits into from
Oct 31, 2022

Conversation

vedanshujain
Copy link
Contributor

@vedanshujain vedanshujain commented Oct 27, 2022 •
edited by veljkho
Loading

All Submissions:

Changes proposed in this Pull Request:

When getting order type of a non-order ID, we were emitting a warning. This PR fixes the warning by adding a check to make sure that order type exists before returning.

  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

  1. Create a product, note its ID.
  2. Run the code Automattic\WooCommerce\Utilities\OrderUtil::is_order( $product_id );
  3. On the trunk, you will see a warning. On this PR, no warning will be emitted.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Oct 27, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2022 •
edited
Loading

Test Results Summary

Commit SHA: bb845bd

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests23200302351m 9s
E2E Tests186006019214m 36s
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.

@vedanshujain vedanshujain requested review from a team and barryhughes and removed request for a team October 27, 2022 11:46
barryhughes
barryhughes previously approved these changes Oct 27, 2022
View reviewed changes
Copy link
Member

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

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

Looks good! Approving, but not merging right away so you can review a note I left (I don't think we require a change, but wanted to check-in quickly to err on the safe side).

plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php
@@ -948,7 +948,7 @@ public function get_orders_type( $order_ids ) {
*/
public function get_order_type( $order_id ) {
$type = $this->get_orders_type( array( $order_id ) );
return $type[ $order_id ];
return $type[ $order_id ] ?? '';
Copy link
Member

@barryhughes barryhughes Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not a request for a change, as such, just an observation: I see a few differences between this and the established CPT implementation.

$cpt->get_order_type( $valid_product_id ); # => string(7) "product"
$cot->get_order_type( $valid_product_id ); # => strint(0) ""

$cpt->get_order_type( $invalid_id ); # => bool(false)
$cot->get_order_type( $invalid_id ); # => strint(0) ""

I think the approach here is 'more correct' (it correctly satisfies the documented interface, for one thing), and can't immediately think of a case where this difference will lead to issues, but figured I would note 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.

Yeah, I think CPT data store mimics the WP_Post behavior. On further thinking after your comment, I removed the return type (string?) from the util method declaration, as it wouldn't have worked correctly with CPT store (and would have raised an error) in acebc8d. So probably another review is needed.

Copy link
Member

Choose a reason for hiding this comment

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

Good idea.

barryhughes
barryhughes approved these changes Oct 28, 2022
View reviewed changes
Copy link
Member

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Blocked from merging because of this issue, but I pushed a small commit to address that. As soon as checks pass we can merge.

@barryhughes
Copy link
Member

...Re-running failed checks (seem like random and unrelated fails)...

@vedanshujain vedanshujain merged commit 2c5d3d2 into trunk Oct 31, 2022
@vedanshujain vedanshujain deleted the fix/order_type branch October 31, 2022 08:56
@github-actions github-actions bot added this to the 7.2.0 milestone Oct 31, 2022
@github-actions
Copy link
Contributor

Hi @vedanshujain, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

@jonathansadowski jonathansadowski modified the milestones: 7.2.0, 7.1.0 Oct 31, 2022
jonathansadowski pushed a commit that referenced this pull request Oct 31, 2022
@vedanshujain @barryhughes @jonathansadowski
Check order type is set before returning to prevent notice. (#35349)
79d6bcb 
* Check order type is set before returning to prevent notice.

* Applied code standards.

* Remove type declaration since its not consistent with CPT datastore.

* Switch to a yoda condition (satisfy required linting check).

Co-authored-by: barryhughes <3594411+barryhughes@users.noreply.github.com>
This was referenced Oct 31, 2022
Merged
Merged
jonathansadowski added a commit that referenced this pull request Oct 31, 2022
@jonathansadowski @vedanshujain @barryhughes
Prep/release 7.1.0 rc.2 (#35441)
88f58aa 
* Check order type is set before returning to prevent notice. (#35349)

* Check order type is set before returning to prevent notice.

* Applied code standards.

* Remove type declaration since its not consistent with CPT datastore.

* Switch to a yoda condition (satisfy required linting check).

Co-authored-by: barryhughes <3594411+barryhughes@users.noreply.github.com>

* Add readme entry for 35349

* Remove change file for 35349

* Update woocommerce.php to prep for 7.1.0-rc.2

Co-authored-by: Vedanshu Jain <vedanshu.jain.2012@gmail.com>
Co-authored-by: barryhughes <3594411+barryhughes@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Nov 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barryhughes barryhughes barryhughes approved these changes

Assignees
No one assigned
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@vedanshujain @barryhughes @jonathansadowski