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

Fixed SQL errors if allowed IDs are empty. #20

Merged
merged 5 commits into from
Nov 9, 2021

Conversation

bogdanarizancu
Copy link
Contributor

No description provided.

Copy link

@chrisbaltazar chrisbaltazar left a comment

Choose a reason for hiding this comment

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

very nice 👍

}
if ($allowedOrderItemIds) {
$tableWheres["{$wpdb->prefix}woocommerce_order_itemmeta"] = "order_item_id IN ({$allowedOrderItemIds})";
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we move these in the WooCommerce group/block below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean here, but we can refactor later on.

@sun
Copy link
Member

sun commented Nov 3, 2021

Not sure how there can be empty user IDs though. Every site needs at least one administrator account, so this is technically not possible.

@sun sun changed the title Fixed mysql error for empty allowed ids collection. Fixed SQL errors if allowed IDs are empty. Nov 3, 2021
Copy link
Member

@sun sun left a comment

Choose a reason for hiding this comment

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

We can move forward with this. But as mentioned before, this case most likely does not result in a functional website, because too much data is omitted in the dump. Therefore I added a warning message to inform the user about this problem.

If you are experiencing the issue of empty $allowedUserIds in one of our current sites, then you will have to dig deeper than this PR. If you only faced the WooCommerce related query errors, and you only added the same condition for the user IDs for consistency in the code, then everything is okay.

@bogdanarizancu
Copy link
Contributor Author

We can move forward with this. But as mentioned before, this case most likely does not result in a functional website, because too much data is omitted in the dump. Therefore I added a warning message to inform the user about this problem.

If you are experiencing the issue of empty $allowedUserIds in one of our current sites, then you will have to dig deeper than this PR. If you only faced the WooCommerce related query errors, and you only added the same condition for the user IDs for consistency in the code, then everything is okay.

@sun — you are right, I only experienced empty order ids, so a shop with no orders yet, and thought maybe check users while at it. Now that you mentioned it, it doesn't really makes sense to check for admins, if that one is empty, you've got bigger problems than not being able to clean export your database, I will actually remove this to keep the codebase as clean as possible, thanks for the tip.

@bogdanarizancu bogdanarizancu merged commit 65b9def into main Nov 9, 2021
@bogdanarizancu bogdanarizancu deleted the fix-check-for-empty-ids-bogdan branch November 9, 2021 06:33
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.

3 participants