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

RESTful: Handle single field request with empty value #251

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

RESTful: Handle single field request with empty value #251

wants to merge 3 commits into from

Conversation

autowp
Copy link
Contributor

@autowp autowp commented Jul 25, 2017 •
edited
Loading

Controller/AbstractRestfulController

There is no way to PUT/PATCH object with single field to empty value.

PUT /item/1
Content-Type: application/x-www-form-urlencoded

name=

Request payload is interpreted as string ($data = 'name=';) regardless Content-Type.
Proper value is $data = ['name' => ''];

I think ideally is split behaviour by Content-Type but that follows to huge BC. (see testCanReceiveStringAsRequestContent)

My pull request handle just one situation (example above) by header.

@autowp autowp changed the title Handle single field request with empty value RESTful: Handle single field request with empty value Jul 25, 2017
@michalbundyra
Fixed parsing request content string - one field with empty value
11ca56d 
To keep BC we need to process as follows:
- `foo=`    -> `['foo' => '']`
- `foo`     -> `foo`
- `foo&bar` -> `['foo' => '', 'bar' => '']`

This is no longer dependent on content-type provided, as it was before.
michalbundyra
michalbundyra reviewed Dec 8, 2019
View reviewed changes
test/Controller/RestfulControllerTest.php
@@ -100,22 +100,6 @@ public function testDispatchInvokesCreateMethodWhenNoActionPresentAndPostInvoked
$this->assertEquals('create', $this->routeMatch->getParam('action'));
}

public function testCanReceiveStringAsRequestContent()
Copy link
Member

Choose a reason for hiding this comment

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

This test has been merged into the new test added below.

@michalbundyra
Copy link
Member

@autowp I've made the change a bit more generic. It does not depend now on the content-type, as it was before. In case we have one element with empty value we are checking if the last character of the content is =. If so, then it is "correct" urlencoded content and we return array, as expected.
In case we have any string without = it will return this string, not an array.

In general I think it should be completely changed to work properly with the given content type correctly, as now this is quite hacky, imo.

@michalbundyra michalbundyra added this to the 3.1.2 milestone Dec 8, 2019
@weierophinney weierophinney mentioned this pull request Dec 31, 2019
Open
@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-mvc; a new issue has been opened at laminas/laminas-mvc#18.

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-mvc. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-mvc to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-mvc.
  • In your clone of laminas/laminas-mvc, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@michalbundyra michalbundyra michalbundyra left review comments

@froschdesign froschdesign Awaiting requested review from froschdesign

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
3.1.2
Development

Successfully merging this pull request may close these issues.
3 participants
@autowp @michalbundyra @weierophinney