The AddToCartForm needs to work with any purchasable entity.
But when we fixed the "multiple add to cart forms interfere with each other" bug, we introduced this into getFormId():
$product_id = $this->entity->getPurchasedEntity()->getProductId();
$form_id = $this->entity->getEntityTypeId();
if ($this->entity->getEntityType()->hasKey('bundle')) {
$form_id .= '_' . $this->entity->bundle();
}
if ($this->operation != 'default') {
$form_id = $form_id . '_' . $this->operation;
}
$form_id .= '_' . $product_id;
That code needs to be replaced with something generic.
I thought about a variation ID, but that one changes on #ajax, which would probably break the form.
Perhaps we simply use a static counter?
Comments
Comment #3
bojanz commentedComment #4
steveoliver commentedLet's see how existing tests deal with the static counter idea -- https://travis-ci.org/drupalcommerce/commerce/builds/176834111 (https://github.com/drupalcommerce/commerce/pull/559).
Comment #5
steveoliver commented(Build passed.)
Comment #6
steveoliver commentedWhile working on something else I noticed Drupal\Component\Utility\Html::getUniqueId. Would that be helpful?
Comment #7
mglamanLooks good to me, we have coverage in MultipleCartFormsTest and tests passing. I guess the problem with getUniqueId is that it's for HTML (outputs
-when form IDs need_[which maybe they do not anymore?]). So we'd have to copy a lot of that code for seenIds and such.I think the static counter is simplistic and gets the job done.
Comment #8
steveoliver commentedThis is ready after a small change suggested by @bojanz on the GitHub PR.
See https://github.com/drupalcommerce/commerce/pull/559/files:
Comment #10
bojanz commentedThanks!
Comment #11
steveoliver commentedThis caused this issue: #2831895: AddToCartForm should support multiple instances - the static counter doesn't work when having multiple products on one page.
By committing this diff:
we can still support all purchasable entities. Should we commit that to this issue?
Comment #12
bojanz commentedI don't see how that can work since #ajax changes the selected variation, thus changing the purchasable entity ID.
Why would the static counter fail? Core reuses the form instance?
Comment #13
steveoliver commentedI'm not sure exactly why the static counter fails - the form ids do seem to be incremented as expected. But currently multiple add to cart forms are broken, and are fixed by using an entity id.
There may be issue(s) with #ajax--in fact I know of at least one, and will create a separate issue--but they are not caused by current committed code or the change I'm proposing (to use the purchased entity id), as the forms ids are already mangled when rebuilt and sent back from ajax calls. Ajax field replacement on variation change is handled by ProductVariationFieldRenderer::buildAjaxReplacementClass, and that is actually broken with the current static counter and fixed by using the purchasable entity id in the form id.
Ajax field replacement does not work as expected if you have the same product add to cart form more than once on a page -- it seems because of the replaceRenderedField targeting only a field class, when it should probably be targeting that class within the scope of the form in question.
I propose we commit the follow-up child issue, which will get multiple add to cart forms working again. Then we can work on the ajax issue(s).
Comment #14
steveoliver commentedCreated the #ajax issue I mentioned above -- #2832268: AddToCartForm should support multiple of the same purchasable entity add to cart form
Comment #15
bojanz commentedOkay, so we've committed a fix that passed tests, it introduced a regression, and we don't know why, while our tests are still green.
We now have a new fix, which works, but we also don't know why.
That means it's time to stop committing things, and start investigating / improving tests.
1) In #2831895: AddToCartForm should support multiple instances drugan mentioned that the static counter failed because of self VS static, can you check if that's true?
2) Can you check how come our tests are green?
If we can't find a quick answer to #1 and #2 I'll go ahead and revert the original fix that we did in this issue. However, I still like the simplicity of a static counter.
Comment #16
drugan commentedAs for the counter we can even to make it work like this:
But the counter is not the actual reason for not working multiple carts views. For now I've discovered that on the view we have a lot of variations but any $selected_variation ->getProductId() always returns the ID of the first product in a view list. That is interesting because when you select attributes' options or even click at the relative Add to cart button you have an expected result (if #575 is applyed).
Yesterday I had a hope that steveoliver's suggestion to replace counter with $this->entity->getPurchasedEntity()->id() will help to find the involved product but this is actually the ID of every first (chosen by default) variation on every product. If anyone know how to fetch the actually selected product in multiple carts view - please post it here, so we all know.
Comment #17
drugan commentedJust in order to not multiply issues and PRs this patch could be applied above the latest commit on #578.
The static counter does not work because:
1. The sorting order of ajax automatically added counters on a view is a total mess up. At first when you load the page they are reversed starting from 10 to 1 Add to cart forms. But when trigger the ajax by choosing an attribute's option it changes to normal 1 to 10 order. Obviously that our counter cannot not work in such environment because it always starts from 1++.
2. Automatically generated #id for each attribute select list appears not unique and that is why you thrown to the top of the page when ajax replaces elements.
3. The suggested by steveoliver $this->entity->getPurchasedEntity()->id() is also not unique enough as there are might be a lot of variations with the same id just because they are added to a product as "existing variation".
My suggestion is to concatenate variation ID, product ID, created and changed time and sha1() the resulted string just to hide this data from prying eyes. Not sure with this kind of hashing but think it should be enough.
Yes, there still could be the case when one and the same product/variation (add to cart) appears two or more times on the page but it is not the big issue because whatever instance of the same add to cart form you use - you will add to a shopping cart the same variation.
Comment #18
agoradesign commentedThe intention behind the static counter was to get a product/product variation independent solution, as the AddToCartForm should be able to handle any purchasable entity... Maybe the PurchasableEntity Interface needs to be extended to return a correct identifier, where e.g. product variations will include their parent product, other implementations may behave differently. However, that would not be an ideal solution, if this additional function would have no other usage justification than this.
Does your solution work properly with the Ajax callback on selecting different variations (does the field replacement work properly)?
Comment #19
drugan commented@agoradesign
Yes, it works. To reproduce:
1. Create a view of product type(s) having variable number of attributes.
2. Go to a view page and try choose different options on attributes.
3. Check if title, sku, price changes for a chosen variation.
4. Add to cart chosen variation.
5. Go to cart page and check if the variation is added correctly.
As for the counter it was said that incrementing nature of a that identifier will not work with the current views/ajax workflow. Because it reverts the view order after the first ajax field replacing. Look, not the order of products (add to cart forms) but the automatically generated by ajax counter identifiers. For example, when you first load the page the identifiers are in reverse order: --10 through NULL if you have 10 add to cart forms on the page. But when you trigger ajax the order becomes NULL through --10. At the same time our add to cart form counter always starts from 1 and then increments. And that does not work. To abstract from any external code handling an ajax request we need unique identifier for each add to cart form based on some persist data, not the counter. In addition to concatenated product ID, variation (purchasable entity) ID, variation created time and changed time might be added a SKU which is unique for each variation but not sure with the latest because current drupalcommerce has some possibility to add SKU-less variation to a product.
Comment #20
bojanz commentedI'll review the entire issue later, just wanted to clarify this:
Variations cannot be shared between products, they always belong to a single one.
We'll need to remove the IEF "add existing" button, otherwise the system will explode in fun ways.
Comment #21
drugan commentedThe #17 patch is outdated as it does not work in some circumstances. Instead use this one or grab the latest version of it here.
Also, as no moving were made on #578 in the last time I've made a new #582 PR.
The solution 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 add to cart forms 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.
Comment #22
drugan commentedComment #23
steveoliver commented@drugan - This patch supports multiple add to cart forms, but it still assumes "Product/variation" is the purchasable entity. We need to support any purchasable entity.
Comment #24
drugan commented@steveoliver
Now it should work with any purchasable entity.
Before the latest commit:
After:
As I understand purchased entity cannot exist without some parent entity would be that Product or instance of some other class. So let's sha1() only serialized purchased entity and if that was already assigned earlier to some twin entity then make it unique in the while loop. Initially I've added product ID prefix just to avoid as much as possible entering into the while loop because of performance considerations but the overload in this case is so tiny that using product ID prefix could be skipped. The suggested solution will work without it.
At any time you can take the latest version of this patch here.
Comment #25
drugan commentedComment #26
joake commented#17 fixed this issue for me, thanks @drugan
Comment #27
steveoliver commented(We need to get this handled sooner than later) Someone please write tests :)
Comment #28
hansfn commentedI guess the value of "me too" comments is limited. Anyway, I came here from #2786245: Wrong product added to cart and drugan's patch in comment 21 fixed the issue for me. (Using latest dev version.)
Comment #29
bojanz commentedThe main blocker to this issue is the fact that we still don't have a failing test.
Unfortunately, I won't have time for this issue in the next few days.
Comment #30
drugan commented@bojanz
ok, I'll try it.
Comment #31
drugan commentedThe test for multiple Add to cart forms on a page has been moved to a FunctionalJavascript namespace and in a lot is rewritten. It requires javascript as without the AJAX it is pretty difficult to reproduce failure case. That's why the previous test was always been green despite the fact that this and some other related issues are still remaining alive.
Also, two helper methods are introduced to facilitate fetching Add to cart and Shopping cart values displayed on a test page. If you ever need to write a test for those elements I'd recommend to look into the modules/cart/tests/src/Functional/CartBrowserTestBase.php file.
Comment #32
eric heydrich@drugan I used your patch and it works. But sometimes it's required to add the a product to the cart twice. Maybe this is an AJAX error. I disabled the cache for the view.
1. I click on add to cart
2. Page reloads, but no product is added to cart, no AJAX error is shown in console and no error is shown in watchdog
3. I once more click on add to cart
4. Product is normally added to cart
Comment #33
drugan commented@Eric Heydrich
I suppose you've applied the latest diff below (not the patches posted on this page):
https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul...
To reproduce the error I need the following info:
Comment #34
eric heydrich@drugan
1. There were no errors at all. No error in chrome dev tools or in dblog.
2.+3. I have two block views on one page. Each has approximately 30 products and each product has its own add to cart form.
4. Every form can produce this error. The position doesn't matter.
5. They are all the same product type with the the same product variation type.
6. There is only one attribute at each product. The attribute is prepopulated. It's a predefined number of copies of a product. For example 100,250,500. The 100 is the prepopulated default value.
7. When I choose an attribute the AJAX-Throbber is shown. But I think this is like it's expected.
8. There is only the patch mentioned above installed.
Comment #35
13jupiters commented#24 is working without problems for me, fixing the multi add to cart issue. (Core 8.2.7/commerce-dev 3/27.) Just a note: despite the suggestion that views cache would have to be turned off for pages with multiple add to cart forms, so far I've had no problems despite leaving caching on. Just lucky?
Comment #36
drugan commented@Eric Heydrich
Can't reproduce your issue.
What I've done:
@13jupiters
The cache should be disabled for each next view having duplicated Add to carts: page or block (panel? not tested). So if you don't have duplicated Add to carts - you don't need to disable cache.
The reason is the views uses the result from the previous DB query made with same parameters (product ID) and therefore skipping to render a form which was already rendered and instead using the views cache for this purpose. That is great for performance but not for our Add to cart forms which get their unique AJAX wrapper ID exactly in the form rendering process.
Comment #37
mglamanCan someone create a View export and paste the YAML here so we can start to work on a test? Anyone, just dump a YAML here which allows us to reproduce so we can start the test process. Or how to configure it.
Comment #38
drugan commentedWell, there is a view which I use for testing the #582 PR patch.
While uploading the .yml I've got this:
So, please rename the file to the original name: views.view.commerce_products_page_blocks.yml
Comment #39
drugan commented@mglaman
I've thought would it be more convenient for you to import DB dump with ready to use products and view and blocks enabled.
The same as on the previous upload. Just rename the file to acommerce.sql.gz
After importing run this and then log in with user admin password admin:
drush crI assume you have devel and admin_toolbar folders in the web/modules/contrib directory.
First, try choose a not default attribute and then add to cart. You should get an uncaught exception like this:
That's because all errors reporting is enabled. Then apply the patch and try again. The exception should be gone and variations added to cart as expected.
Comment #40
eric heydrichI still have the problem from #34. I have tried everything from removing modules to changing themes. I get an ajax-Error now.
1. I choose a product variation and put it in the cart. Everythings fine. The form token is y8Ut7gxb9IF2ro97FsAjbMjuqPFKkXvt6qiTJLMVn_o. Page reloads as usual.
2. I try to choose a variation and ajax-Process is terminating. Then I try to put it in the cart but nothing happens. Form Token in dev Tools is now AHa_vrhpi-__E_uyQAYR8W3O3VvGNT1WyYgFqgiJ4K0.
3. See step 1.
Is it possible that because of the changing Form Tokens AJAX can't finish every second clicks on add to cart button?
Comment #41
drugan commented@Eric Heydrich
Please, do this:
Fresh install drupalcommerce. I suppose you have a copy no older than this commit:
Install devel and admin_toolbar modules.
Dump or copy the database of the fresh installed site. You can use it later to restore the initial state of the site.
Drop the database.
Create empty database with the same name as the dropped one.
Download the DB dump on the #39 comment.
Rename the downloaded file to acommerce.sql.gz
Import the file into the empty database created above.
cd to the site's root and run this:
drush updatedb --entity-updatesGo to /my-products page and log in to the site:
user: admin
password: admin
Try to add any product variation to cart without changing attributes. It should be added as expected.
Try to add any product variation to cart after changing some of the attributes. It should result into the fatal error. Actually, changing attributes does not change product variation title, sku and price on the Add to cart form.
Apply this patch and refresh the page with Ctrl+F5.
The products variations should be added to cart as expected.
If you still observe errors while adding product variations to cart then you need to post more info on the environment you are running your drupalcommerce site.
Comment #42
Idriss.BS commentedI create a patch for this issue "Wrong product added to the cart" https://www.drupal.org/node/2880939 .The add to cart and the Ajax attributes change working fine .I hope the patch fix your problem.
Comment #43
pavelculacov commentedI dont know if i can write this bug here.
1) Content type Product with field Product reference (View mode Render Entity > View mode (Default))
2) Create single product with two variant type
3) Create two content type Product with same reference to Product
4) Create view where display all Content Type Product, where field product type is (Render Entity > View mode default)
5) Change Variant type, ajax working only for first element, not for second.
6) Cache on view is disabled.
If i understand correctly only one Product Type add to card must be on page?
make some debug, form generate only once, and for all content type is the same form. And Ajax work only for first match id in html.
Issue is found AddToCartForm should support multiple of the same purchasable entity add to cart form.
Comment #44
drugan commented@Regnoy
Please, fresh install drupalcommerce site. I suppose you have a copy no older than this commit:
Make modifications on the site to reproduce your bug: add product/variation/attribute/content types, views, whatever...
Dump sites' database to my-dump.sql.gz file.
Upload the file on this page.
Comment #45
pavelculacov commentedAfter install fresh commerce bug persis.
if try change on second content type product attribute not working, after change on first content type product attribute , start working second form.
Comment #46
pavelculacov commentedUpload my db.
Comment #47
drugan commented@Regnoy
Thanks for sharing the DB dump.
The set up that you use on your site (product referenced by a content type) does not work with the current patch.
I wonder why you don't use actual products to display Add to cart forms? Basically they are the same content types which could be extended with any fields, references, view modes, etc.. Also, you can display your products on the front page like content type entities do by default. How:
After that you can see your products displayed dynamically just visiting example.com address without view path appended.
Note: those who uses the current patch and have duplicated Add to cart forms (the same products) on a page I'd recommend to read the #21 comment.
Comment #48
pavelculacov commentedI know that path not work for me
"I wonder why you don't use actual products to display Add to cart forms?"
For me, ease use reference, that content type is single with all fields (body, features,image,tags,related link, overview, specification, compatible devise etc more that 10 fields) and have single field reference to product type (title and custom variants). In project have more than 5 product type with custom variation types. And for me not need too duplicate fields for each Product Type, all field have Content Type, ease use reference to product type.
"Also, you can display your products on ..." i always do like you describe, for exemple in dump file i made like this.
Issue that i have i this issue links.
AddToCartForm should support multiple of the same purchasable entity add to cart form.
Thanks for response. @drugan
Comment #49
drugan commented@Regnoy
Now I see what you require. If I were you I'd try this:
Create a desirable product type.
Go admin/commerce/config/product-types/MY-PRODUCT-TYPE/edit/fields and remove body field.
Create desirable content type with all fields (body, features,image,tags,related link, overview, specification, compatible devise etc more that 10 fields).
Create a content of the just created type.
Go admin/commerce/config/product-types/MY-PRODUCT-TYPE/edit/fields and add a reference field to Content -> Reference type -> just created content type.
Go to /admin/commerce/config/product-types/MY-PRODUCT-TYPE/edit/form-display and admin/commerce/config/product-types/MY-PRODUCT-TYPE/edit/display and set up order/widget type for the reference field to your content.
Create a product of the MY-PRODUCT-TYPE type and choose an instance of the content as a reference. Then use product as usual.
The only question remains how to remove a Title field of the referenced content. As a quick and dirty solution it might be done via CSS. If someone knows how to do it with a different method, please share so we all know.
Comment #50
pavelculacov commentedWhy you suggest me to revers my ideea?:) it the same like your but visa versa. O lot of module not working with Product Type like shareThis (I must change in module to be share the product type).
Back to issue, did you see that second form not working in my backup ?
The only question remains how to remove a Title field of the referenced content. >>> override node.html.twig and use Display Suite or Pseudo Field
Comment #51
drugan commented@Rednoy
Yes, I did. It does not work neither before nor after applying the current patch.
Unfortunately at the moment I have no so much time to adjust the patch for working with such cases like yours one. Sorry about that.
Comment #52
pavelculacov commentedI know, thanks for attention, but please describe error in this issue. I think issue is the same with error that i found or already existed :).. Thanks again and good luck
AddToCartForm should support multiple of the same purchasable entity add to cart form.
Comment #53
mglamanSitting down and reviewing today, along with test writing.
Comment #54
mglamanI just realized that in #2868637: AddToCartFormatter not working in Views a view was added which renders multiple add to cart forms for each product, and tests it. So maybe we can improve that test to prove what was written in #21?
Comment #55
mglamanOkay, I'm postponing this. The original issue was fixed. However I think this thread is now discussing other problems and we need to discern.
It seems we have two bugs being talked about in this thread
Comment #56
mglamanI am putting this back to fixed from #9, as the original report was about how we generated the form ID. Which has been fixed.
We have tests which pass for Views + multiple add to cart forms, which was tested and fixed in #2868637: AddToCartFormatter not working in Views. Please try the latest -dev of Commerce and make sure to test with the fix from that issue.
If you are still experiencing bugs, please review the following two issues and see if they related. Please provide detailed reproduction steps (such as big_pipe enabled, multiple products via Views, changing attribute AJAX.) We need to expand our test coverage and the best way to do so is having explicit details we can reproduce in tests with succinct issue reports.
Thanks, everyone.
Comment #57
drugan commentedI've tested against this commit on a fresh install:
... and had this exception when adding to cart from a view of products:
Seems the #9 still does not work.
Comment #58
mglaman@drugan
So as I requested for detailed steps: you added an attribute, and added how many variations? Please open an issue detailing multiple add to carts in views with attributes and how to replicate in a test.
Thanks!
Comment #59
drugan commentedTo reproduce the #57 issue:
Navigate to your webroot and run:
composer create-project drupalcommerce/project-base mysite --stability devIn the end it asks whether you want to remove .git folders. Answer n (no).
Run this:
cd mysite && composer require drupal/devel drupal/admin_toolbar(Should we require admin_toolbar module by default for the drupalcommerce?)
Install mysite site using any method.
Dump or copy the database of the fresh installed site. You can use it later to restore the initial state of the site.
Drop the database.
Create empty database with the same name as the dropped one.
Download the DB dump file on this comment.
Rename the downloaded file to mysite.sql.gz
Import the file into the empty database created above.
Run this:
cd web/modules/contrib/commerce && drush crGo to /all-my-products page and log in to the site:
user: admin
password: admin
Try to add any product variation to cart without changing attributes or otherwise. It should result into the fatal error:
Run this and refresh the page with Ctrl+F5:
wget https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pull/582.diff && git apply 582.diffThe products variations should be added to cart as expected.
Go to admin/structure/block/list/bartik and place All My Products block into the Sidebar first region.
Go to /all-my-products page. You'll see that all products in the block are basically duplications of the view page products.
Try to change attributes on the My Colors & Sizes - FIRST product in the block. You'll see that AJAX does not work there. If you try to add this product to cart you'll see that disregarding on the select list values it is always Red, Small variation added to cart.
Try to change attributes on the My Colors & Sizes - FIRST product in the page view. Now, the AJAX works not only for the page product but for the block's one too. Though select lists' values are still not changed. It is expected behaviour because AJAX wrappers for both displays of the product are different. Only formatters are the same because their wrappers based on the class not the id. That's why the title, sku and price are synced for both product displays.
So, when you changed the page product's attribute values now the same product also works on the block as expected. Obviously that this introduces a nasty UX despite no any errors are emitted. To mitigate this issue do this:
Go admin/structure/views/view/all_my_products/edit/block_1?destination=all-my-products and set Caching: None only for the block (override).
Try to change attributes on the block. You'll see that all works as expected from the first attempt though title, sku and price are still synced on both the displays of a product. Which is also a UX issue but only a minor one. Obviously that solution for this should be found but for now it would be enough to solve not working products' views which is critical issue and must be fixed before the first RC.
Comment #60
drugan commentedComment #61
bojanz commentedContinuing in #2886614: Rework the AddToCartForm form ID handling because it has better information about the actual problem.
"Unknown attribute field name "attribute_xxxxxx" might actually be unrelated to the main issue here.
Comment #63
amrausch00 commentedWhere am I supposed to be saving the patch file? I am new to Drupal. I saved it within all\modules\commerce and tried to run the patch, but it can't find the file to patch. When I go through the file directory, I do not see any folders or files labeled src. Can someone tell me what I am doing wrong?
Comment #64
drugan commented@amrausch00
You may save a patch in any place on your system. Then open the Terminal,
cdto the drupalcommerce module's root folder and run this if you have dev version of the module:git apply path/to/file.patchOr...
git apply path/to/file.diffOr, if you don't have a git:
patch -p1 path/to/file.patchOr...
patch -p1 path/to/file.diffRead more here.
As for the current issue most of the patches posted on this page are outdated. If you have problems with multiple add to carts on a page I'd recommend to read this comment:
https://www.drupal.org/node/2707721#comment-12225526
Comment #65
bojanz commented@amrausch00
This issue is fixed, which means that the fix is present in the 2.0-rc1 release. You don't need to patch anything.