Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Added $data param to deleteList #7255

Merged
merged 2 commits into from
Mar 3, 2015

Conversation

TomHAnderson
Copy link
Contributor

This was a [wip] which was invalidated when the branch was rewritten. Was #7140

@weierophinney
Copy link
Member

This needs a test for the new behavior, please.

@TomHAnderson
Copy link
Contributor Author

Modified existing deleteList to send array of ids.

);
$string = http_build_query($entities);
$this->request->setMethod('DELETE')
->setContent($string);
Copy link
Member

Choose a reason for hiding this comment

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

While this sets up data to pass to the controller, it does not test that the controller received that data. You need to do something with the data that you can then assert against later.

@TomHAnderson
Copy link
Contributor Author

The Travis test failures in PHP 5.5 & 5.6 do not correlate to the changes in this PR.

@weierophinney
Copy link
Member

I need to revert this, and immediately issue a 2.3.7 that removes it. The feature as merged is a BC break, as it adds a required argument to an existing signature; any code that extended it previously breaks — including the intended use case in zf-rest's RestController (tests break under 2.3.6, as reported by a number of users who updated their ZF versions after the release was made).

My plan is:

  • Revert in master, and immediately tag and release 2.3.7
  • Issue a new PR against develop that makes the argument optional.

@@ -116,7 +116,7 @@ public function delete($id)
*
* @return mixed
*/
public function deleteList()
public function deleteList($data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avert a BC break my understanding is this line should be

public function deleteList($data = null)

weierophinney added a commit that referenced this pull request Mar 12, 2015
This reverts commit 2750340, reversing
changes made to b952b35.

This reverts #7255 under master, as it introduced a BC break against
`Zend\Mvc\Controller\AbstractRestController`.
weierophinney added a commit that referenced this pull request Mar 12, 2015
weierophinney added a commit that referenced this pull request Mar 12, 2015
Forward ports a1aae5f to develop. However, it "un-reverts" (is that a word?) the
change introduced in #7255, updating it to make the new argument optional for
the upcoming 2.4 release. I'll note, however, that even addition of an optional
argument raises errors about signature mismatches.

Conflicts:
	README.md
	library/Zend/Version/Version.php
weierophinney added a commit that referenced this pull request Mar 12, 2015
Reverts #7255
Prepares for 2.3.7
@weierophinney
Copy link
Member

This is now fixed in develop. Interestingly, making the argument optional is still a signature break, and, as such, I'm retaining the "BC Break" label so we can call it out in the upgrade notes.

weierophinney added a commit that referenced this pull request Mar 26, 2015
This reverts commit ca307f4.

For some reason, merging develop to master for the RC series of 2.4.0
re-re-reverted. This commit re-instates the changes for #7255 on the master
branch for v2.4.0.
weierophinney added a commit that referenced this pull request Mar 26, 2015
Re-instate #7255 on develop branch.
weierophinney added a commit that referenced this pull request Mar 26, 2015
Re-instate #7255 on master branch for 2.4.0.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants