Easy-digital-downloads: Stop using -1 for User ID references on Payments
Brought up in #5721, we really need to stop using -1 for the "no user assigned" value. We should be using 0.
cklosowski
All 20 comments
For a check, these are all the < 0 checks in EDD that would be affected by looking for a user_id reference that is -1. This is in PHP files, none of them reference a User ID.
/Users/cklosowski/Development/edd.dev/wp-content/plugins/easy-digital-downloads/includes (11 usages found)
class-edd-customer.php (2 usages found)
623if( $new_total < 0 ) {
673if( $new_value < 0 ) {
class-edd-discount.php (2 usages found)
1880if ( $amount < 0 ) {
1948if ( $this->uses < 0 ) {
class-edd-fees.php (1 usage found)
114if( $args['amount'] < 0 ) {
class-edd-register-meta.php (1 usage found)
401if ( ! $allow_negative_prices && $price < 0 ) {
deprecated-functions.php (1 usage found)
284if( $amount < 0 )
download-functions.php (1 usage found)
748return $earnings < 0 ? 0 : $earnings;
formatting.php (2 usages found)
42if( $amount < 0 ) {
131$negative = $price < 0;
shortcodes.php (1 usage found)
352if ( $query['posts_per_page'] < 0 ) {
/Users/cklosowski/Development/edd.dev/wp-content/plugins/easy-digital-downloads/includes/admin/payments (2 usages found)
actions.php (2 usages found)
46} elseif ( $hour < 0 ) {
55} elseif ( $minute < 0 ) {
/Users/cklosowski/Development/edd.dev/wp-content/plugins/easy-digital-downloads/includes/admin/tools (1 usage found)
class-edd-tools-recount-store-earnings.php (1 usage found)
85if ( $total < 0 ) {
/Users/cklosowski/Development/edd.dev/wp-content/plugins/easy-digital-downloads/includes/api (1 usage found)
class-edd-api.php (1 usage found)
723if( $per_page < 0 && $this->get_query_mode() == 'customers' ) {
/Users/cklosowski/Development/edd.dev/wp-content/plugins/easy-digital-downloads/includes/cart (5 usages found)
class-edd-cart.php (5 usages found)
259if ( $total < 0 ) {
714if ( $discounted_price < 0 ) {
1043if ( $subtotal < 0 ) {
1149if ( $total < 0 || ! $total_wo_tax > 0 ) {
1326if ( ! empty( $fee['no_tax'] ) || $fee['amount'] < 0 ) {
/Users/cklosowski/Development/edd.dev/wp-content/plugins/easy-digital-downloads/includes/payments (9 usages found)
class-edd-payment.php (7 usages found)
811} elseif ( $log_count_change < 0 ) {
821} elseif ( $price_change < 0 ) {
970if ( $total_change < 0 ) {
1152if( $total < 0 ) {
1610if ( $this->subtotal < 0 ) {
1642if ( $this->fees_total < 0 ) {
1684if ( $this->tax < 0 ) {
functions.php (2 usages found)
839if( $total < 0 ) {
870if( $total < 0 ) {
I've merged the PR in but let's leave this open for now. There's a good chance we'll run into an extension or two that is looking for -1 that we'll need to update.
pippinsplugins
on 11 May 2017
Marking for feedback so it's easier to know where these stand.
cklosowski
on 26 Jun 2017
@easydigitaldownloads/core-devs After searching the entire Add-Ons folder in Dropbox, I found these lines with -1 in them and then looking through for strings of user
Event Ticketing/events/resources/tickets-attendees.js:201:
if ( $user > -1 )
Misc/Cross Sell & Upsell/edd-cross-sell-upsell/includes/class-export-cross-sells.php:118:
$user_id = isset( $user_info['id'] ) && $user_info['id'] != -1 ? $user_info['id'] : $user_info['email'];
Misc/Cross Sell & Upsell/edd-cross-sell-upsell/includes/class-export-upsells.php:118:
$user_id = isset( $user_info['id'] ) && $user_info['id'] != -1 ? $user_info['id'] : $user_info['email'];
We can fix Cross Sell and Upsell easily and send Modern Tribe an email about event ticketing.
pippinsplugins
on 6 Jul 2017
It's actually more than just -1.
The ways you could have done it:
- To detect logged out user: < 1, < 0< == -1, <= -1
- To detect existent user: > -1, >= 0, > 0, >= 1, != -1.
Also just a note, the variable in certain cases is called $user_id, $id, $customer_id (if predating the customer object), etc.
What might be more prudent to do, given there's a lot of custom code out there that likely uses this, is for EDD core to be able to handle both -1 and 0 denoting logged out/non attached user, the same way we do for $key being either 0 or 1 for the first variation. This will take a tiny bit more time from a backcompatibility standpoint, but have the advantage of ensuring no code is broken.
If absolutely set on removing this, this really needs to be in a dev note for like a month+ because the type of use case that people will be using this type of thing for is like My Account and checkout stuff and messing with that could really hurt people. Maybe even go so far as to when a Payment is retrieved, and the user_id === -1, throw a notice.
We also need to be careful in that there might be functions that people use that say if the user_id is empty coming in, try to get the current user ID or something like that.
chriscct7
on 6 Jul 2017
Just a side note, numbers going into update/add/delete/get meta functions absint, so payment->user_id of -1 uses a value of 1 in these functions: https://core.trac.wordpress.org/browser/tags/4.8/src/wp-includes/meta.php#L37
chriscct7
on 6 Jul 2017
What might be more prudent to do, given there's a lot of custom code out there that likely uses this, is for EDD core to be able to handle both -1 and 0 denoting logged out/non attached user, the same way we do for $key being either 0 or 1 for the first variation. This will take a tiny bit more time from a backcompatibility standpoint, but have the advantage of ensuring no code is broken.
I agree whole heartedly with this.
pippinsplugins
on 10 Jul 2017
If you really want to discourage this kind of thing going forward, adding a _doing_it_wrong() wouldn't be completely out of line either. It's one of the least utilized methods of re-education in WordPress, imo.
DrewAPicture
on 10 Jul 2017
It wouldn't be applicable here. -1 is coming from EDD's database, and the
plugins are just doing checks based on the value (which is the thing
they're supposed to be doing). EDD is just backwards compatibility
breaking, and having to update plugins that were designed around what for a
long time has been a core value of EDD
On Mon, Jul 10, 2017 at 1:33 PM, Drew Jaynes notifications@github.com
wrote:
If you really want to discourage this kind of thing going forward, adding
a _doing_it_wrong() wouldn't be completely out of line either. It's one
of the least utilized methods of re-education in WordPress, imo.—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/easydigitaldownloads/easy-digital-downloads/issues/5727#issuecomment-314178251,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQ2XfHSrZMjX5rkjYM9Ebe9SwbgYluwks5sMmBpgaJpZM4NXSVD
.
chriscct7
on 10 Jul 2017
Going to revert this from 2.8 for now after some discussion. We'll get a formal plan in front of devs in the coming weeks. We'll have a blog post explaining the change coming on the 31st of July. After which we'll add support for both -1 and 0 in core and extensions, and then we'll work on correcting the old data.
cklosowski
on 17 Jul 2017
The new orders table stores 0 as the user_id by default if one isn't passed or the user is not logged in. We could take care of this during the migration routine for existing payments.
I'll add in the backwards compatibility too when doing the meta back compat for payments.
sunnyratilal
on 30 May 2018
Just a note, something to also test here in core + plugins is -1 is not empty wheras 0 is in PHP. There might be plugins doing empty() on this type of thing
chriscct7
on 5 Jun 2018
The real crime here is not having a function to encapsulate these comparisons. Without it, plugins needed to do the -1 check directly, instead of calling edd_payment_has_customer() or whatever.
JJJ
on 7 Jun 2018
That's incorrect, since this is used to look up a userID not the customerID,and -1 does not reflect not having an EDD customer, it reflects a checkout where the user is logged out at the time of purchase and the payment is not linked to a customer, or where the customer was deleted later but the payment retained.
What should be done is an edd_is_guest_payment() function that checks to see if the user_id meta is saved, and if not checks to see if a customer is saved, and if so, if the customer has a user_id (bare in mind, not all EDD customers have user IDs).
We should make sure we carefully word notices to devs because user != customer and userID != customerID in EDD. These are 2 separate data objects and points respectively.
chriscct7
on 7 Jun 2018
That's incorrect, because the lack of a user (or customer) is different than a user or customer having been deleted.
If a user or customer was deleted, that user certainly wasn't a guest. And it depends on whether we are trying to identify where the user went, or whether there ever was one in the first place, or if that even matters.
JJJ
on 7 Jun 2018
The meta key though historically has been used to tie a payment to a WordPress user. If the WordPress user is deleted (or the payment detached from the customer) the user_id meta key field has historically reverted to -1 in specific cases
chriscct7
on 7 Jun 2018
Post has been published on the development blog notifying developers: https://easydigitaldownloads.com/development/2018/06/21/breaking-changes-to-orders-in-easy-digital-downloads-3-0
Implementation wise, the rest of this is complete. User IDs are now stored as 0 in lieu of -1 in the edd_orders table. Unfortunately, this is not something we cannot provide backwards compatibility for.
sunnyratilal
on 21 Jun 2018
Opened a PR for Cross-Sell & Upsell: https://github.com/easydigitaldownloads/edd-cross-sell-upsell/issues/24
sunnyratilal
on 22 Jun 2018
Closing this out as the issue over on the Cross-Sell and Upsell repository has been added to the 3.0 Compatibility project.
sunnyratilal
on 21 Jul 2018
Related issues
amdrew
·
5Comments
boluda
·
4Comments
mikeyhoward1977
·
5Comments
davidsherlock
·
4Comments
nabeghe
·
5Comments
Most helpful comment
The new orders table stores
0as theuser_idby default if one isn't passed or the user is not logged in. We could take care of this during the migration routine for existing payments.I'll add in the backwards compatibility too when doing the meta back compat for payments.