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

Shopping cart performance enhancements #6011

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

lat9
Copy link
Contributor

@lat9 lat9 commented Oct 12, 2023

Reformatting

  • Use null coalescing (??) operator where indicated
  • Use short array syntax
  • No one-line conditionals
  • No one-line multiple assignments

Refactoring

  • Since the contents property is keyed on a 'uprid', change the $products_id key to be $uprid on foreach traversals of that array. That makes it more obvious when viewing the code as to what the contents array is keyed on.
  • Limit MySQL updates to a single row for unique operations.
  • Use === where warranted.
  • Use zen_define_default where warranted.
  • Use strpos instead of preg_match when checking for gift-certificates in the cart.
  • Remove redundant code.
  • Use zen_get_product_details where warranted to get maximum cached queries. Results in a roughly 50% reduction in time taken to add/remove/update a product as well as the cart's overall calculations!

Corrections

  • calculate didn't process a 'base' product pricing if a product wasn't found, but did process the attributes! Should the product be removed from the cart if this condition is found?

Reformatting
    - Use null coalescing (??) operator where indicated
    - Use short array syntax
    - No one-line conditionals
    - No one-line multiple assignments
Refactoring
    - Since the `contents` property is keyed on a 'uprid', change the `$products_id` key to be `$uprid` on foreach traversals of that array.
    - Limit MySQL updates to a single row for unique operations.
    - Use `===` where warranted.
    - Use `zen_define_default` where warranted.
    - Use `strpos` instead of `preg_match` when checking for gift-certificates in the cart.
    - Remove redundant code.
    - Use `zen_get_product_details` where warranted to get maximum cached queries.  Results in a roughly 50% reduction in time taken to add/remove/update a product as well as the cart's overall calculations!
Corrections
    - `calculate` didn't process a 'base' product pricing if a product wasn't found, but *did* process the attributes!  Should the product be removed from the cart if this condition is found?
@lat9
Copy link
Contributor Author

lat9 commented Oct 13, 2023

FWIW, I'm also considering removing the use of $display_debug_messages; is anyone actually using this? There are many better ways to debug the shopping-cart actions.

@drbyte
Copy link
Member

drbyte commented Oct 13, 2023

FWIW, I'm also considering removing the use of $display_debug_messages; is anyone actually using this? There are many better ways to debug the shopping-cart actions.

I've only used it a few times, but it was certainly helpful at the time.

If the class had more smaller methods in it it would be even easier to debug when a problem occurred, instead of having to trace through to figure out which block is doing what ... which that $display_debug_messages var was trying to help with, IIRC.

@lat9
Copy link
Contributor Author

lat9 commented Oct 13, 2023

Agreed on the smaller methods. If the feature has utility, it stays!

@lat9 lat9 mentioned this pull request Oct 17, 2023
@drbyte
Copy link
Member

drbyte commented Oct 18, 2023

Oy, this is a massive bunch of changes in one commit. Super complex to review. 😨

Copy link
Member

@drbyte drbyte left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks. That's a huge overhaul, optimizing a ton of things.

A couple small bugs highlighted.

Brings back memories of when a bunch of this code was written ... ... shaking my head :D

@lat9
Copy link
Contributor Author

lat9 commented Oct 18, 2023

I believe that I've got all the corrections/comments addressed in the just-pushed update.

lat9 added a commit to lat9/zencart that referenced this pull request Oct 18, 2023
lat9 added a commit to lat9/zencart that referenced this pull request Oct 18, 2023
@drbyte drbyte merged commit 636d4a3 into zencart:master Oct 18, 2023
5 checks passed
@torvista
Copy link
Member

torvista commented Oct 20, 2023

If you set
declare(strict_types=1);
to the start of this file, and add a product to the cart, it dies/spits out a series of errors due to mismatched casting.

@lat9
Copy link
Contributor Author

lat9 commented Oct 20, 2023

Thanks, @torvista. I'll address that in the changes associated with #6017, since the shopping_cart class requires updates there as well.

@torvista
Copy link
Member

There's only 12 errors, but some may generate a "big-picture" discussion, so it may be prudent to hoist it above the parapet as a separate thing to throw mud at, in the interests of mixing metaphors.

@lat9 lat9 deleted the cart-performance-update branch December 10, 2023 15:20
drbyte pushed a commit to drbyte/zencart that referenced this pull request Jan 30, 2024
* Shopping cart performance enhancements

Reformatting
    - Use null coalescing (??) operator where indicated
    - Use short array syntax
    - No one-line conditionals
    - No one-line multiple assignments
Refactoring
    - Since the `contents` property is keyed on a 'uprid', change the `$products_id` key to be `$uprid` on foreach traversals of that array.
    - Limit MySQL updates to a single row for unique operations.
    - Use `===` where warranted.
    - Use `zen_define_default` where warranted.
    - Use `strpos` instead of `preg_match` when checking for gift-certificates in the cart.
    - Remove redundant code.
    - Use `zen_get_product_details` where warranted to get maximum cached queries.  Results in a roughly 50% reduction in time taken to add/remove/update a product as well as the cart's overall calculations!
Corrections
    - `calculate` didn't process a 'base' product pricing if a product wasn't found, but *did* process the attributes!  Should the product be removed from the cart if this condition is found?

* Updating based on review comments
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.

None yet

3 participants