Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.I noticed a module we are using (uc_option_image) stopped working after uc_aac upgrade (it took me a while to realize it was this module). Just now I also noticed that the uc_out_of_stock also fails to work.
I came then to the issue queue to check for the same issues and I am almost positive #666612: uc_acc conflicts with ahah is because of this same thing.
Now I know this is part of your current project's spec:
Rebuilds the entire product view form on every change. This allows users to leverage existing Drupal hooks such as hook_nodeapi and hook_form_alter to alter the appearance of the product view form.
I have not 100% understood that statement enough to propose an alternative solution that would fit within your specs, but what you are currently doing is a bit aggressive for other javascript modules that plug in somehow into the ubercart add to cart form because, while removing and recreating the DOM form, all of the other events that might have been attached to the original form are lost..
I am leaving this for discussion in how you think this might be fixed or addressed but I think it's an important enough thing if you plan on supporting only the 2.x from now on.
I'll have to revert back to 1.x as well.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 697976-uc_aac-drupal-behaviors-2x.patch | 1.54 KB | hanoii |
| #5 | 697976-uc_aac-drupal_behaviors.patch | 780 bytes | hanoii |
Comments
Comment #1
jantoine CreditAttribution: jantoine commentedHi Hanoii,
The idea of the 2.x branch of uc_aac is to provide a single module that all other modules can leverage for updating product pages via AJAX. It provides some very basic AJAX updates on it's own such as the price, sku and attribute options. It also allows other modules to interact with it via some Drupal hooks and it's own hooks. This is why I have proposed the integration of the uc_out_of_stock module with uc_acc and provided a patch in this issue: #687748: Integration with Ubercart Ajax Attribute Calculations. The uc_option_image module is the next module I plan on integrating after uc_out_of_stock. If all AJAX related modules shared a central hub, users will be able to pick and choose different AJAX features they want without having to worry whether or not the modules work together. If you have ideas on how to improve this module I would love to hear your thoughts.
Cheers,
Antoine
Comment #2
hanoiiI'd really like to be more constructive at this time but I really haven't gone deep enough in either this module or your proposed integration with uc_out_of_stock, but I can give you some general thoughts.
* The main thing I think this module has to rework is the way you are erasing the full form. It's not that you are giving something that other modules can leverage, but you are forcing them to do so. Having one place to centralize things is not always the best thing to do, specially when the behavior of each module is independent from each other. Not only that, but any other module that may, or may not have to do with ubercart but with general forms, just as the incompatibility reported here with the ahah, or any other module that it tries to plug into form elements will not be able to work with this module enabled.
* There's something with the semantic of the project as well. I expect uc_aac to do one specific thing, maybe if you really feel that the idea of this API or module framework you are creating is worth enough to be used by any other module, it may be hosted as a separate project?
But I am most worried with the first thing. A module should be plug and play and should not, as much as they can, do things that can conflict with other modules. So my only concrete thought so far is that what you are doing re-generating the add to cart form should be done differently, in a way that other modules, whether they decide or not to use some code from other modules, can co-exist.
Comment #3
longwaveuc_aac and other modules should use Drupal.behaviors instead of $(document).ready() to attach events to forms, and uc_aac should call Drupal.attachBehaviors after replacing the form in the DOM. This is the recommended way of dealing with these issues in Drupal 6 and means that modules that alter pages via JavaScript can work independently or together, without specific knowledge of each other.
See http://api.drupal.org/api/drupal/developer--topics--javascript_startup_g... for more.
Comment #4
hanoiiThanks for pointing us to that, I wasn't aware of that at all and seems more than reasonable. I'll try to have a look at it soon to update my JS modules and If I can I'll suggest a patch for this one if Antoine doesn't sort this out first.
Comment #5
hanoiiWell, here it goes the first patch. This uses the proper method explained in the link provided by longwave on #3.
This solves the problem of my module stopping to work before of the form being removed as now I switched to use Drupal.behaviors and I did the same for yours and add Drupal.attachBehaviors() after you remove the form.
There are still usabilities issues though, which I'll try to sort them out some time but I still thinks that removing the form is a bit too much. Right now, because of the removal+attachingBehaviors, the product's stock is queried twice. Some display things happen also because the form is replaced by a new one and my module is queried once more, you may get some flickering between the add to cart button and the out of stock message if that was being displayed before.
This mentined problem, I am not sure how to fix without either reworking the removal of the form from this module or tying the two modules together (which I rather not) is that as both modules are modifying the same form and somehow the order is important and all this behaviors happen asynchronously.
Will report back, however this patch is an important one, for this and any other module wrongly relaying on document.ready() rather than Drupal.bahaviors.
Comment #6
hanoiiI reviewed the patch and because the module now uses behaviors, the need to run the attach function on the Ajax post is unnecessary.
Also changed function names to lowerCamelCase following http://drupal.org/node/172169
Attached is updated patch.
Comment #7
jantoine CreditAttribution: jantoine commentedHanoii,
Thanks for putting the time into this module. I have been busy with other projects this past month. I think this patch is a step in the right direction and gives us a temporary fix to the incompatibility issue. I added one change to the .module file to correctly call the renamed ucAacCalculate JavaScript function on page load and committed the patch.
Next I would like to address the flicker issue. Perhaps there is a way to specify the order in which the JavaScript is called?
Lastly I would like to focus on the issue of the module being to aggressive in replacing the form and brainstorm some new ideas in how we can achieve the same functionality without being so aggressive.
Cheers,
Antoine
Comment #8
Nick_vhI mark this patch as reviewed since it solved my problem of the aggressive form replacement of the uc_aac module.
This makes this module much more usable and compatible. Please add to the head :-)
Comment #9
cYu CreditAttribution: cYu commentedAntoineSolutions committed this code a while ago. Someone can re-open this or open a new ticket if there is still work to be done in this area.