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

Issue #2827721: AddToCartForm should support all purchasable entities #582

Closed
wants to merge 7 commits into from

Conversation

drugan
Copy link

@drugan drugan commented Dec 13, 2016

…AddToCartForm should support all purchasable entities

Works with multiple add to cart forms having different number of attributes. Also works with add to cart duplications on a page but the only deficiency is that all SKUs, titles and prices are replaced properly on all the twin carts but the attribute value is replaced just on the element which is actually triggered an ajax event. Obviously it requires a solution for this complex add to carts use case but for now it might be easily resolved by excluding the same add to cart forms on a page or just ignoring ,,the feature,, as it does not break checkout workflow in any way except UX issue.

Important note: if you still require multiple duplicated carts on a page you need to disable views caches on the each next view (page or block) which has the same add to cart forms as the previous one.

noviewscaching

@drugan drugan changed the title Issue #2827721 by steveoliver, drugan, bojanz, agoradesign, mglaman: … Issue #2827721: AddToCartForm should support all purchasable entities Dec 14, 2016
@bojanz bojanz force-pushed the 8.x-2.x branch 2 times, most recently from 68e6e5f to ac287b7 Compare January 12, 2017 04:31
@@ -136,9 +132,21 @@ public function getFormId() {
if ($this->operation != 'default') {
$form_id = $form_id . '_' . $this->operation;
}
$form_id .= '_' . self::$formInstanceId;
$id = sha1(serialize($this->entity->getPurchasedEntity()->toArray()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use spl_object_hash for the ID

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the point because I didn't know about this function before. Though for the first look it might seem more concise and 2.15 times faster than sha1(serialize($this->entity->getPurchasedEntity()->toArray())) but, .... it does not work. Despite all the generated IDs may appear unique they are not actually. It's the same issue as with the counter which we have now. The reason why these both methods to generate unique ID for the form don't work is that they operate on the values which exist only in the current PHP memory and when happen AJAX replacement (after choosing another attribute field option) the previous values are not more relevant to objects where they were generated before. PHP description of the spl_object_hash():

This id can be used as a hash key for storing objects, or for identifying an object, as long as the object is not destroyed.

Additionally to the above spl_object_hash() ignores the properties of the object which also prevents to uniquely identify it. I think that there is no any other way to do it except relying on the values (properties) saved in the consistent memory.

Note, all the tests are done not only with multiple Add to cart forms but also with duplicated carts on a page.

@mglaman
Copy link
Contributor

mglaman commented Jul 7, 2017

@drugan thanks for your SQL dump before. There's several issues being fixed in this PR. I'm going to review this.

@mglaman
Copy link
Contributor

mglaman commented Jul 8, 2017

I am picking this up in #758.

@mglaman mglaman closed this Jul 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants