There are several places in commerce where an Equal operator is used to compare the #value of an element trigger where an Identical operator would be better since something like if ($form_state['triggering_element']['#value'] == t('Add line item')) can evaluate to true if #value of the triggering_element is (int) 0 or (boolean) TRUE. A good example is if un-checking a checkbox triggers the AJAX callback.

This is bad for other modules that integrate AJAX callbacks into the form that have submit handlers as they may never get called if $form_state['rebuild'] is set to TRUE as a result of the improper evaluation in the if.

Comments

acrazyanimal created an issue. See original summary.

acrazyanimal’s picture

I searched the module for all such situations and came across three. Here is a patch which is git aware.

Btw, my use case to get this fixed is to provide a proper embedded registration management section on the order edit ui for the commerce_registration module. This will greatly improve the admin functionality for commerce_registration.

acrazyanimal’s picture

Status: Active » Needs review
mglaman’s picture

Issue tags: +Commerce Sprint

Interesting! I wonder if there is a way we could write a test for this, but seems like only way to test is by implementing a form alter, which tests cannot do without a testing module (that we do not have.)

TimRutherford’s picture

Status: Needs review » Reviewed & tested by the community

Can confirm patch #2 works.
As mentioned above, testing this case seems to be a bit tricky. Not really an easy was to change the triggering element in tests without somehow adding elements to the form. Might be best to just leave the tests alone.

rszrama’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the review. Committed!

  • rszrama committed 6a33048 on 7.x-1.x authored by acrazyanimal
    Issue #2652858 by acrazyanimal: Use Identical operator rather then...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.