Page MenuHomePhabricator

Merging $_GET and $_POST with array_merge is a bad idea
Closed, ResolvedPublic

Description

Author: lrbabe

Description:
Recently, the way we deal with REQUEST datas has changed (see http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/includes/WebRequest.php?r1=29192&r2=30882).
We doesn't use any more $_REQUEST, instead we merge $_GET and $_POST with array_merge($_GET, $_POST) and $_POST overrides $_GET datas wich share same keys (see http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/includes/WebRequest.php?annotate=30882#l53).
But the array_merge function has a side effect : pure numeric (not beginning with 0) keys of $_GET and $_POST are changed (see http://fr2.php.net/manual/en/function.array-merge.php)!
Despite the fact that numeric names are forbidden in HTML (see http://www.w3.org/TR/html4/types.html#type-cdata), people using them in extension (for example) will get confused. Moreover, it breaks some backward compatibility, for example with the quiz extension (see https://bugzilla.wikimedia.org/show_bug.cgi?id=13074 and test that : http://en.wikiversity.org/wiki/User:McCormack/quiz_test).

Solutions are :

  • changing $this->data = array_merge($_GET, $_POST); simply with $this->data = $_GET + $_POST; wich should works as expected and does'nt seems to have any contraindication (once again, see http://fr2.php.net/manual/en/function.array-merge.php).
  • checking both $_POST and $_GET for pure numeric keys and throwing an exception.

PS : I apologize for my poor english level.


Version: 1.12.x
Severity: normal
OS: Linux
Platform: PC

Details

Reference
bz13139

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:07 PM
bzimport set Reference to bz13139.
bzimport added a subscriber: Unknown Object (MLST).

ayg wrote:

CC'ing Brion, who committed r30882.

Fixed in r31327. Replaced array_merge() with wfArrayMerge() which behaves in a sane fashion.

Gee, I wish we didn't have to reimplement half of PHP's standard library.