Page MenuHomePhabricator

HTMLMultiSelectField::loadDataFromRequest doesn't work if request method = GET
Closed, ResolvedPublic

Description

It checks wpEditToken to determine whether a form submission was made but when method="get" no edit token is attached.


Version: unspecified
Severity: normal

Details

Reference
bz27676

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 11:32 PM
bzimport added a project: MediaWiki-General.
bzimport set Reference to bz27676.
bzimport added a subscriber: Unknown Object (MLST).
Qgil added a comment.Mar 18 2013, 5:47 AM

Is this a bug or an enhancement request?

Is it still relevant today?

(In reply to comment #1)

Is this a bug or an enhancement request?
Is it still relevant today?

Both. It's not a documented behavior so you may say it's an enhancement from this aspect, but it doesn't work in the same way as other form fields so you may say it's a bug...

Qgil added a comment.Mar 18 2013, 4:16 PM

The class is documented at http://svn.wikimedia.org/doc/classHTMLMultiSelectField.html#a695a8f2d3de4a6199343daa6b4dcbea0 but I don't know whether it is supposed to work with method="get"

Also wondering whether there is a better component for this bug...

(In reply to comment #3)

The class is documented at
http://svn.wikimedia.org/doc/classHTMLMultiSelectField.
html#a695a8f2d3de4a6199343daa6b4dcbea0
but I don't know whether it is supposed to work with method="get"
Also wondering whether there is a better component for this bug...

Hmm there's already a FIXME comment there:

https://svn.wikimedia.org/doc/HTMLForm_8php_source.html#l02147

02163 # This is the impossible case: if we look at $_GET and see no data for our
02164 # field, is it because the user has not yet submitted the form, or that they
02165 # have submitted it with all the options unchecked? We will have to assume the
02166 # latter, which basically means that you can't specify 'positive' defaults
02167 # for GET forms.
02168 # @todo FIXME...

(In reply to comment #4)

Hmm there's already a FIXME comment there:

Nope. It has been changed after I submitted this bug.

155ddf6d (Andrew Garrett 2009-04-24 01:31:17 +0000 2152) function loadDataFromRequest( $request ) {
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2153) if ( $this->mParent->getMethod() == 'post' ) {
15599d3d (awjrichards 2012-05-04 15:09:33 -0700 2154) if ( $request->wasPosted() ) {
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2155) # Checkboxes are just not added to the request arrays
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2156) # so it's perfectly possible for there not to be an en
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2157) return $request->getArray( $this->mName, array() );
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2158) } else {
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2159) # That's ok, the user has not yet submitted the form,
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2160) return $this->getDefault();
e0a4a54c (Siebrand Mazeland 2010-05-30 12:44:17 +0000 2161) }
155ddf6d (Andrew Garrett 2009-04-24 01:31:17 +0000 2162) } else {
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2163) # This is the impossible case: if we look at $_GET and see no
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2164) # field, is it because the user has not yet submitted the form
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2165) # have submitted it with all the options unchecked? We will ha
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2166) # latter, which basically means that you can't specify 'positi
75c6696a (Siebrand Mazeland 2011-05-17 22:03:20 +0000 2167) # for GET forms.
75c6696a (Siebrand Mazeland 2011-05-17 22:03:20 +0000 2168) # @todo FIXME...
6bedff7a (Happy-melon 2011-03-14 15:50:26 +0000 2169) return $request->getArray( $this->mName, array() );
155ddf6d (Andrew Garrett 2009-04-24 01:31:17 +0000 2170) }
155ddf6d (Andrew Garrett 2009-04-24 01:31:17 +0000 2171) }

happy.melon.wiki wrote:

That comment is about a specific issue with checkbox fields (namely that on a GET request, we can't tell whether the checkbox is "unset" because it's been actively deselected, or because it's the first time the user has displayed the form. Sounds like the issue with wpEditToken is something different.

Can we add a hidden form field indicating whether a GET request is form submission or not?

happy.melon.wiki wrote:

(In reply to comment #8)

Can we add a hidden form field indicating whether a GET request is form
submission or not?

No, because the state of a GET form is expressed in the URL (that's the whole premise of a GET). So if we added a 'submitted=1' field and someone shares the generated URL, the form appears as if it had already been submitted. Essentially the concept of "submitted" doesn't have as strong a meaning for GET forms as it does for POST forms.

Qgil added a comment.Feb 22 2014, 9:38 PM

So... any conclusion to this discussion, helping to resolve this report in a way or another?

matmarex claimed this task.May 30 2016, 9:04 PM
matmarex added a subscriber: matmarex.
In T29676#330228, happy.melon.wiki wrote:

Can we add a hidden form field indicating whether a GET request is form submission or not?

No, because the state of a GET form is expressed in the URL (that's the whole premise of a GET). So if we added a 'submitted=1' field and someone shares the generated URL, the form appears as if it had already been submitted. Essentially the concept of "submitted" doesn't have as strong a meaning for GET forms as it does for POST forms.

That's true, but I don't think it's a problem. Such a shared URL would still work as intended, and allow us to distinguish between the user selecting no options (in which case we should preserve that), and the user not selecting anything (in which case we should load the default values).

Change 291837 had a related patch set uploaded (by Bartosz Dziewoński):
HTMLForm: Allow distinguishing between form views and submission attempts

https://gerrit.wikimedia.org/r/291837

Change 291837 merged by jenkins-bot:
HTMLForm: Allow distinguishing between form views and submission attempts

https://gerrit.wikimedia.org/r/291837

matmarex closed this task as Resolved.Jul 22 2016, 6:51 PM

It will now work as expected for forms that have an identifier set (call $form->setFormIdentifier( 'foo' ) when setting up the form).

matmarex set Security to None.