Skip to content

Commit

Permalink
Remove un-necessary Set::merge().
Browse files Browse the repository at this point in the history
Using Set::merge() on an empty array causes issues with out of order
numeric keys. Only merge if necessary.

Fixes #2595
  • Loading branch information
markstory committed Feb 19, 2012
1 parent 2ddc387 commit 89df484
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
15 changes: 14 additions & 1 deletion lib/Cake/Network/CakeRequest.php
Expand Up @@ -145,7 +145,14 @@ public function __construct($url = null, $parseEnvironment = true) {

/**
* process the post data and set what is there into the object.
* processed data is available at $this->data
* processed data is available at `$this->data`
*
* Will merge POST vars prefixed with `data`, and ones without
* into a single array. Variables prefixed with `data` will overwrite those without.
*
* If you have mixed POST values be careful not to make any top level keys numeric
* containing arrays. Set::merge() is used to merge data, and it has possibly
* unexpected behavior in this situation.
*
* @return void
*/
Expand All @@ -165,8 +172,14 @@ protected function _processPost() {
}
unset($this->data['_method']);
}
$data = $this->data;
if (isset($this->data['data'])) {
$data = $this->data['data'];
}
if (count($this->data) <= 1) {
$this->data = $data;
}
if (count($this->data) > 1) {
unset($this->data['data']);
$this->data = Set::merge($this->data, $data);
}
Expand Down
13 changes: 13 additions & 0 deletions lib/Cake/Test/Case/Network/CakeRequestTest.php
Expand Up @@ -177,6 +177,19 @@ public function testPostParsing() {
$_POST = array('one' => 1, 'two' => 'three');
$request = new CakeRequest('some/path');
$this->assertEquals($_POST, $request->data);

$_POST = array(
'data' => array(
'Article' => array('title' => 'Testing'),
),
'action' => 'update'
);
$request = new CakeRequest('some/path');
$expected = array(
'Article' => array('title' => 'Testing'),
'action' => 'update'
);
$this->assertEquals($expected, $request->data);
}

/**
Expand Down

8 comments on commit 89df484

@ceeram
Copy link
Contributor

@ceeram ceeram commented on 89df484 Feb 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git bisect shows this commit as introduding duplicate values when posting habtm data with input('Tag') on a Post form.

request->data['Tag']['Tag'] should be array(1, 2), but now shows array(1, 2, 1, 2) when selecting first 2 tags

@ceeram
Copy link
Contributor

@ceeram ceeram commented on 89df484 Feb 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ceeram/cakephp@2.1...2.1-requestdata

results: http://bin.cakephp.org/view/1818149545

created a testcase for this, not sure how to solve it yet, need to look into that

@ceeram
Copy link
Contributor

@ceeram ceeram commented on 89df484 Feb 22, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

included a fix now as well

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm an idiot, I don't know how I missed that the first time around. I merged in your changes :D

@ADmad
Copy link
Member

@ADmad ADmad commented on 89df484 Feb 23, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markstory Your original commit was on 2.0 so perhaps the fix done by Ceeram for 2.1 should be backported to 2.0 too.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It certainly should. I cherry-picked the changes in [836f913] and [6d3c659]

@ceeram
Copy link
Contributor

@ceeram ceeram commented on 89df484 Feb 23, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented in wrong place already i think

@markstory The branch had 4 commits in total for the tests and fix, i messed up with making tests
you only cherry-picked two of the commits in 2.0, causing a fail

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I guess I'll find the other commits and merge those too.

Please sign in to comment.