Easy-digital-downloads: Introduce EDD_Cart class

[stephenharris]

Unlike my previous one-line bug fixes, this is a fairly major refactor/feature request with no code (yet...) :).

It would be great to see EDD handle carts/products using classes rather than download IDs, which would make it incredibly easy for third-party plug-ins to "piggy-back" onto EDD. In particular other plug-ins could add items to EDD's cart which are not (strictly speaking) 'downloads' (post type).

For instance, to add an item to cart (edd_add_to_cart()) requires a download ID. If instead you defined:

• cart class, EDDCart and
• a cart item interface iEDDCartItem which set out the methods that that EDDCart would use (e.g. get_checkout_label(), get_price() etc)

You could then define a class (see related #2214 )

  EDDDownloadItem implements iEDDCartItem{

  }

for downloads, while third party plug-in could create their own class

  FooBarItem implements iEDDCartItem{

  }

which would allow them to do something like

   $download = new EDDDownloadItem( $downoad_id );
   $foo = new FooBarItem();
   $eddcart->add( $download );
   $eddcart->add( $foo );
   $total = $eddcart->get_total();

This adds a layer between the cart and the database, so that the cart doesn't know/care where the data is coming from. Additionally the cart item interface sets out what methods is expected of a cart item, and so any class implementing it knows that it will behave with the cart.

I hope I've explained myself better than I think I may have :). Lastly, I'm aware that these are fairly fundamental changes, but if the cart implemented array access then to a large degree backwards compatibility could be maintained.

[chriscct7]

If they don't use the download class, they could just add items as fees in 2.0

[stephenharris]

They could, but fees (correct me if I'm wrong) don't have quantities in the cart. So when $foo is added to the 'cart', they would need to check if there are any existing $foo items and take that into account. In doing so they would be unnecessarily duplicating the role of the cart.

[chriscct7]

Product quantities are off by default in EDD, but we could add quantity support to them.

[pippinsplugins]

I absolutely agree that we need to have a good cart class (we are already doing a new EDD_Download class).

[cklosowski]

With EDD_Payment, EDD_Customer, and EDD_Download done, I think we can move to EDD_Cart as the next major development improvement.

[sunnyratilal]

Update on this issue:

• [x] EDD_Cart base class has been created
• [x] 25% of the functionality from the cart/functions.php file has been moved over into the EDD_Cart class

Left to do:

• [x] Full migration of all the cart functions

[sunnyratilal]

@pippinsplugins It would be good to discuss this issue early next week to discuss the EDDCartItem interface and how we would go about doing it to allow _non-downloads_ to be added to the cart.

[chriscct7]

We should name it EDD_Cart_Item for consistency imo

@sunnyratilal there's an EDD meeting on Tuesday. Perhaps we could either discuss it there or hold a dev meeting right before/after it?

[sunnyratilal]

@chriscct7 I'm going to be flying on Tuesday so won't be able to make that meeting.

[pippinsplugins]

@sunnyratilal Ping me during the day on Monday.

[sunnyratilal]

EDD_Cart Update

Done

• All cart functions have been migrated over to EDD_Cart
• EDD_Cart now handles all cart operations
• Some of the functions located in includes/discount-functions.php which were specific to the cart have been moved into EDD_Cart

[sunnyratilal]

@easydigitaldownloads/core-devs Would love some input here please.

I'm run into a somewhat roadblock whilst trying to finish up what's left here. We are creating EDD_Cart_Item and EDD_Download is extending EDD_Cart_Item. The base methods (get_name() and get_price()) are defined in EDD_Cart_Item. Making EDD_Download extend EDD_Cart_Item is simple and even implementing the functions is simple.

The big issue is with the add() function. We pass a $download_id to the function which is used to setup an instance of EDD_Download and we call specific functions to populate the $options array which is stored as part of the cart contents.

The issue I'm facing if we are allowing anything extend EDD_Cart_Item to be added to the cart, we will need to enforce some form of consistency to how the purchase data is created and sent to the gateway.

How best should I proceed with this? My current inclination is to move the EDD_Cart_Item abstraction to a later release after we have an exact plan how to support all types of items.

[pippinsplugins]

My current inclination is to move the EDD_Cart_Item abstraction to a later release after we have an exact plan how to support all types of items.

What would that mean for the EDD_Cart implementation? Would EDD_Cart_Item be completely separate from EDD_Download or not used at all?

[sunnyratilal]

It would mean EDD_Cart_Item would not be used at all for now until we have a solid plan in place as to be proceed with different cart items.

The way we have our cart/checkout set up right now would mean that EDD_Cart_Item would be holding a lot more data than just the name and price. We would have to change our database schema too with the way the cart_items are stored in the payment meta.

[pippinsplugins]

:+1:

[sunnyratilal]

@easydigitaldownloads/core-devs Ready for general testing.

[pippinsplugins]

In setup_cart() the saved cart is retrieved like this:

$this->saved = get_user_meta( get_current_user_id(), 'edd_saved_cart', true );

This should be updated to support logged-out customers too.

I'd like discount_output() to be left out of the class. It's a templating function.

Shouldn't get_item_discount_amountget_item_discount_amount() be part of EDD_Cart_Item?

We'll want to make sure that all filters, such as edd_get_cart_discounted_amount and edd_get_cart_subtotal only get run once, even when calling the methods multiple times. Right now it looks like they'll be called many times since we're not checking if the class properties have been loaded before loading them again.

[sunnyratilal]

We're not making using use of EDD_Cart_Item based on the comment above so it would stay in EDD_Cart for the time being.

[pippinsplugins]

In two of my tests, the checkout page loaded in 2.6865 seconds on release/2.7 compared to 0.8456 on issue/2283.

Nice improvement!

[pippinsplugins]

@sunnyratilal The add() method currently updates the cart contents via the EDD session class. Can we replace that with a call to update_cart() instead please?

[pippinsplugins]

One of the primary goals of the EDD_Cart class is to reduce the extreme repetition of calls to apply_filters() and do_action() on the checkout screen.

We haven't achieved that yet.

Filters like edd_cart_item_price should only run ones per cart item row. Right now it runs 147 times.

@sunnyratilal can you work on addressing that please?

[sunnyratilal]

@pippinsplugins Working it on now.

[sunnyratilal]

@pippinsplugins @cklosowski Ready for review.

[pippinsplugins]

@sunnyratilal a _lot_ better but not quite there. Filters are still run 55 times.

[sunnyratilal]

@pippinsplugins Got it down to a single call

[pippinsplugins]

This is looking good!

Some quick notes on what needs testing:

• [x] Cart saving
• [x] Adding / removing items
• [x] Cart item quantities
• [x] Taxes
• [x] Discount codes
• [x] Recovering a pending payment
• [x] Software Licensing (renewals, upgrades, and new purchases)
• [x] Recurring Payments (upgrades and new purchases)
• [x] Cart fees with Discounts Pro and Simple Shipping

When all working properly, _nothing_ should obviously change.

[cklosowski]

Saving carts seems to be broken.
release/2.7

issue/2283

[cklosowski]

Changing cart quantities seems to be broken on the Cart. the admin-ajax.php responds with the update data, but the cart isn't updated to show in the UX.

Verified that release/2.7 seems to work fine, but issue/2283 not.

[cklosowski]

Ok Found the issues with cart quantities, but it's exposed a bug in taxes with quantities. Fixing it all up now.

[cklosowski]

Taxes + Quantities figured out.

[pippinsplugins]

Ready for new testing?

[cklosowski]

Not yet.

[cklosowski]

Found another problem, tax is being calculated incorrectly. For instance, with discounts pro I get the following:

release/2.7
$100 Item
50% Discounts Pro discount
6% tax rate
Cart total is:
$100 Item
$50 discount
$3 tax
$53 total

issue/2283
$100 Item
50% Discounts Pro discount
6% tax rate
Cart total is:
$100 Item
$50 discount
$6 tax
$56 total

It's like the tax is being taken prior to any fees affecting the subtotal.

[sunnyratilal]

Is it to do with the filters being called too early?

[cklosowski]

Tax issue fixed,

[cklosowski]

Ok the remaining issue I believe is fixed and this needs some testing but it's raised an interesting issue. The last issue was fixing a unit test where we were creating a payment, with a fee associated with it, all programmatically, meaning, without the cart.

Since the cart didn't really exist, we previously had a filter running:
add_filter( 'edd_cart_contents', '__return_true' );

Which always ran, no matter how many times the cart had been run. This filter is run on get_cart_contents() previously, and ran every time it was called...but now, after the first time edd_get_cart_contents and subsequently EDD()->cart->get_contents() are called, the filter edd_cart_contents will never be run.

This means that the cart session is locked for the first interaction...I'm not sure that's a good outcome.

Thoughts @pippinsplugins @sunnyratilal? You can see I got around it by taking the items I was going to set as my $paymet->cart_details and set the EDD()->cart->details equal to it, but this means that adding fees to a payment are strictly tied to a cart existing before adding it, which means you cannot programmatically insert a payment with a fee.

[cklosowski]

@easydigitaldownloads/core-devs see above comment..I failed to tag ya'll ^^^

[sunnyratilal]

After discussing this at length with @cklosowski - this issue stems from only running the filters once.

I'm thinking the best option at present is to avoid the filters running just once. Alternative is to rewrite the way fees are handled in EDD.

The first option brings us back to our original problem of having filters run multiple times. The second option could cause other things to break. But then again having the filters only run once could cause bugs elsewhere when running into third-party integrations.

[pippinsplugins]

I _really_ don't want the filters to run multiple times. Not taking care of that problem means we've failed to account for one of the primary purposes of the EDD_Cart object.

@cklosowski could you provide some examples where having the cart "locked" becomes problematic?

[cklosowski]

It's in the unit tests....I explained it above.

If you try and create a payment with a fee...it will not add the fee b/c
the cart is empty, and we cannot modify that filter run.

[pippinsplugins]

My question is more "is that a realistic scenario?"

That to me indicates a weakness in our fees API (which we know is weak already).

Are there any real-world scenarios that not being able to add a fee to payments without a cart are an issue? None come to mind for me.

[cklosowski]

Examples include:
1) Programmatically creating a payment (one of the basic points of EDD_Payment) that includes a fee (since the cart does not exist and we cannot filter the cart to think we have items)
2) In the future, allowing a fee to be added to a payment from Admin
3) If an extension is trying to make a modification to the edd_cart_contents AFTER the edd_add_to_cart method has been run.

Basically if anyone wants to make a modification to the cart contents via that filter AFTER the initial setup_cart method has been run. Since setup_cart calls get_contents, that will trigger the filter, so if someone triggers on any other action anywhere else in the page load, they cannot hit that filter (edd_cart_contents)

[pippinsplugins]

What if we made just some of the filters run each time it's accessed? That would still be a significant improvement.

[sunnyratilal]

I'd be good with that.

[cklosowski]

I think that could be a vast improvement. There are just a few that need this treatment...and in fact, I think this is the only one I found that it really impacts anything just b/c of the fact that fees rely on it.

I'll start with just removing this restriction of the one needed for this unit test/integration and go from there.

[pippinsplugins]

If that ends up working, let's then go ahead and proceed with getting a new issue logged for re-building the Fees API to not have this limitation, perhaps for 2.8.

[cklosowski]

Yeah that's a good idea. Fees API should never depend on the cart contents,
in my opinion. I know why it does currently, but now that we're expanding
what a 'Payment' record looks like and how it's created, it could use an
overhaul.

[cklosowski]

Ok pushed and merged, this allowed me to remove the previous change to the unit tests where I had to set the cart contents manually while programmatically creating a payment.

[pippinsplugins]

This appears to be working really well for me!

I've tested regular purchases, SL renewals, RP subscriptions, RP Subscriptions + SL upgrades. All worked fine!

[sunnyratilal]

Great! I'm just running a few final checks!

[sunnyratilal]

Merged into release/2.7!

[cklosowski]

There may be an issue with Simple Shipping. I'm working through it now. Re-opening to that we don't miss this.

[cklosowski]

[cklosowski]

On EDD master

[cklosowski]

Ok, so the problem we have here is that if the fee amount is less than 0, it's affecting the subtotal...but it's not affecting it if the fee amount is greater than 0. I'll submit a PR, but we need some testing on this before we merge.

[cklosowski]

Ok, it's a bug in the EDD_Cart class. @sunnyratilal you mind testing this commit and PR?

[cklosowski]

@sunnyratilal look at issue/2283-2 that's the one that I ended up committing to.

[sunnyratilal]

5430 fixed the bug.

[pippinsplugins]

@sunnyratilal @cklosowski That change concerns me. Can you please confirm it does not reintroduce https://github.com/easydigitaldownloads/easy-digital-downloads/issues/4658?

[sunnyratilal]

Confirmed it does not reintroduce #4658

[pippinsplugins]

@sunnyratilal Your screenshot shows a discount code. This needs to be tested with a negative fee.

[sunnyratilal]

My bad - bug is reintroduced with fees.

[cklosowski]

Well we need a happy medium. As it was previously, fees aren't affecting
cart total.

On Jan 31, 2017 7:23 PM, "Sunny Ratilal" [e-mail] wrote:

My bad - bug is reintroduced with fees.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/easydigitaldownloads/easy-digital-downloads/issues/2283#issuecomment-276555854,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABU2LFUY4LQ1mn8TX5lOFiYtyiVWQCMeks5rX-y1gaJpZM4B6PHU
.

[sunnyratilal]

@cklosowski @pippinsplugins PR opened at #5431.

[sunnyratilal]

Seems to be breaking unit tests - I need to do some further work.

[cklosowski]

Taking a look now.

[cklosowski]

Ok I think I fixed it, it was a bug in the EDD_Fees function. When you specify a Download ID for the get_fees we were iterating over all the fees, and if the download_id was empty on a fee, doing a continue, but this will fail for null and 0, leaving them in the list of fees for a specific download ID, even though it doesn't have a download ID.

[pippinsplugins]

@cklosowski shouldn't this check that the download ID was specified before unsetting it?

if ( (int) $download_id !== (int) $fee['download_id'] || empty( $fee['download_id'] ) ) {

[cklosowski]

@pippinsplugins We can check it first but EDD_Fees::add_fee defaults a download_id value no matter what.

[cklosowski]

@pippinsplugins updated to that request, ready for a review.

[sunnyratilal]

Seems to be working well on my end 👍

[sunnyratilal]

Merged into release/2.7