Pieforms can't tell which "button" element submitted a form

Bug #1526624 reported by Aaron Wells
20
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Mahara
Fix Released
High
Aaron Wells
15.10
Fix Released
High
Aaron Wells

Bug Description

With Mahara 15.10, we replaced a lot of the old <input type="submit"> tags with <button type="submit"> to provide for more flexible styling.

However, no one realized that deep inside Pieform's creaky old internals, it expected your submit element to be flagged with "$element['submitelement'] = true". Without this, if there are multiple buttons in a pieform, all of their values show up in the submit method, instead of just the one that was pressed.

This is the underlying cause of Bug 1526614 ("reject" button causes you to join a group). The accept & reject buttons were previously input tags, and had been changed to buttons. The submit handler function, thinking only one or the other could be pressed, only checked for the presence of the "accept" button. Without "submitelement" on it, the accept button's value (and the decline button's value) came through every time.

In the old "submit" element, this flag is added automatically by the pieform_element_*_set_attributes() hook. I've added a similar hook to the button element that only adds it if it's a submit button (since we can also have non-submitting buttons, for Javascript).

Revision history for this message
Aaron Wells (u-aaronw) wrote :

To get a complete sense of the scope of this bug, we'd need to check and see how many forms there are that have two different submit buttons, and use the pressed button to determine which action to take.

Doing a quick grep, I see the following files that contain "usebuttontag" more than once. These are the places where we're likeliest to find forms like that, although it's possible these could be multiple form definitions in the same file. For instance, taking a quick look at view/urls.php, I see that's the case there.

./collection/views.php:2
./view/urls.php:2
./artefact/comment/lib.php:2
./artefact/annotation/lib.php:2
./artefact/blog/lib.php:3
./lib/user.php:2
./lib/group.php:7
./lib/view.php:4
./webservice/apptokens.php:2
./webservice/admin/index.php:6
./webservice/admin/oauthv1sregister.php:2
./admin/users/bulk.php:3
./admin/users/institutions.php:3
./admin/site/networking.php:2
./user/view.php:2

Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "master" branch: https://reviews.mahara.org/5854

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/5854
Committed: https://git.mahara.org/mahara/mahara/commit/03843792e20a46d9e6a2d6f493f64d03dc6d947c
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit 03843792e20a46d9e6a2d6f493f64d03dc6d947c
Author: Aaron Wells <email address hidden>
Date: Wed Dec 16 19:46:05 2015 +1300

Add "submitelement" attribute to Pieform button elements

Bug 1526624

behatnotneeded: This one actually probably could use some
behat tests. ;) But I'm curious to see if it'll pass
automated testing.

Change-Id: If1c199125d702750011777de1682acdc824dc8ca

Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "15.10_STABLE" branch: https://reviews.mahara.org/6097

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/6097
Committed: https://git.mahara.org/mahara/mahara/commit/33f0faddddda7cf17e18e0c392198d5fb853ecfc
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.10_STABLE

commit 33f0faddddda7cf17e18e0c392198d5fb853ecfc
Author: Aaron Wells <email address hidden>
Date: Wed Dec 16 19:46:05 2015 +1300

Add "submitelement" attribute to Pieform button elements

Bug 1526624

behatnotneeded: This one actually probably could use some
behat tests. ;) But I'm curious to see if it'll pass
automated testing.

Change-Id: If1c199125d702750011777de1682acdc824dc8ca
(cherry picked from commit 03843792e20a46d9e6a2d6f493f64d03dc6d947c)

no longer affects: mahara/16.04
Changed in mahara:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.