Skip to content

Added support for PUT method #849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 12, 2011
Merged

Added support for PUT method #849

merged 3 commits into from
May 12, 2011

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented May 9, 2011

This adds proper support for PUT method (see #793)

@kriswallsmith
Copy link
Contributor

Doesn't the request object check the _method parameter upstream? In the case of an emulated PUT request, this special logic is not necessary.

@fabpot
Copy link
Member Author

fabpot commented May 9, 2011

Good catch. It now uses the "raw" method.

@kriswallsmith
Copy link
Contributor

If we're going to support this, which I think we should, I think it should be in HttpFoundation, not Form.

@johnwards
Copy link
Contributor

I don't think form should be changed to support PUT like this. By the time it gets to the form the framework should have sorted this out. Be that with 'native' PUT or fake _method.

@igorw
Copy link
Contributor

igorw commented May 9, 2011

How about sticking this in an HttpKernel RequestListener?

@fabpot
Copy link
Member Author

fabpot commented May 9, 2011

I have moved the logic into Request directly. Thoughts?

@johnwards
Copy link
Contributor

I've got concerns about PUT, especially in apache. You need to tell apache a script is okay to support put explicitly, or when you ask it what methods it returns put won't be listed. Jquery for example asks apache first before sending a put and kills the request if it doesn't support it. I wonder how safe it would be to provide an htaccess file that says controllers are put ok.

Also this form type flag you are checking for, would that be used in a non HTML form api? Seems an odd thing to set to send from JavaScript or curl, I might be missing something though.

@Seldaek
Copy link
Member

Seldaek commented May 9, 2011

The application/x-www-form-urlencoded content type should be used whenever you use form data stuff, which look just like query params in the url, but placed in the request body. When you do a POST with jQuery, it will encode the object you pass it in that form, and I sure hope it uses the proper header alongside it.

@fabpot: One use case that might not be handled is a PUT with multipart/form-data? Not sure if that's valid, but I don't see why it wouldn't be.

@kriswallsmith
Copy link
Contributor

I wonder if this is too magic? There's no way to create a request object during a PUT request without consuming the stream anymore. I liked the idea of reading the stream in createFromGlobals() because the stream is similar-ish to a global, and if you want to save the stream for later you can just pass the super globals to the constructor.

@Seldaek
Copy link
Member

Seldaek commented May 9, 2011

I think it's alright in the sense that for those requests, you probably don't want to do anything else than parse_str() on that data, but yes it maybe should not be in core (but rather handled in RestBundle and such, and therefore remain optional).

@kriswallsmith
Copy link
Contributor

The core should provide support for this, and probably do it behind the scenes, but there should be a way to disable it. Imagine someone adding a Silex app in a larger PHP application which already makes use of the input stream, for instance.

@marijn
Copy link

marijn commented May 9, 2011

What if a client wishes to PUT data with let's say Content-Type: application/json?

@igorw
Copy link
Contributor

igorw commented May 9, 2011

@marijn
Copy link

marijn commented May 9, 2011

I'm well aware of that bundle but I just don't understand why we can't support this natively it shouldn't be in the core/framework..?

@stof
Copy link
Member

stof commented May 9, 2011

@marijn then you need to get them via getContent which is supported natively. But it does not make sense to parse them to put the content in the query data.

@marijn
Copy link

marijn commented May 10, 2011

@stof why not? The client is requesting to PUT that data into the requested resource... Right..?

@stof
Copy link
Member

stof commented May 10, 2011

@marijn When the are not application/x-www-form-urlencoded data, you cannot use them the same way you use POST data.

@Chekote
Copy link

Chekote commented May 10, 2011

I agree with @kriswallsmith, this makes most sense to be to be in Request::createFromGlobals(). I understand the arguments about different content-types, but this should really just be a "best effort" to capture the name value pairs for a standard PUT request. Anything beyond that would require custom process of the data from Request::getContent().

If this is standard behavior, it would not be difficult for developers to understand that php://input is not valid to use in Symfony, much like $_POST or $_GET is not.

  • Request body is always accessed via Request::getContent(), never php://input
  • If content type is "application/x-www-form-urlencoded", then request parameters are also accessible through Request::request
  • If not, then access the raw request through Request::getContent()

@marijn
Copy link

marijn commented May 10, 2011

Sorry but I still fail to see what the semantic difference is between the following two requests:

Content-Type: application/x-www-form-urlencoded

someKey=someValue&someOtherKey=someOtherValue&someArrayKey[0]=test&someObjectKey[key]=value

Versus:

Content-Type: application/json

{ someKey:       "someValue"
, someOtherKey:  "someOtherValue"
, someArrayKey:  [ "test" ]
, someObjectKey: { key: "value" }
} 

Why should we separate between the two?

@stof
Copy link
Member

stof commented May 10, 2011

marijn "something" is also a valid JSON content but cannot be used this way. So it would not work in all cases.

@marijn
Copy link

marijn commented May 10, 2011

@stof, not it's not. JSON should always be wrapped in object tags.

@stof
Copy link
Member

stof commented May 10, 2011

@marijn I don't see anything in the JSON spec saying that only objects are valid.

@Chekote
Copy link

Chekote commented May 11, 2011

A JSON request can be any valid JSON data type. It does not need to be an object.

@Chekote
Copy link

Chekote commented May 11, 2011

Trying to get the framework to magically translate all these other data types is just going to bloat the framework. If you're saying that JSON data should be automatically decoded in the PUT request, then wouldn't that also apply to POST?

Key / Value pair should be the only thing that's supported by the core framework, since it's just an extension of the very same support that's already implemented in a standard POST request. Anything beyond that should be implemented in Bundles.

@fabpot
Copy link
Member Author

fabpot commented May 11, 2011

I have moved the logic into the createFromGlobals() method. Is it alright for everybody now?

@Chekote
Copy link

Chekote commented May 11, 2011

"Is it alright for everybody now?"

I think Fabien needs a hug! XD


if ('application/x-www-form-urlencoded' == $request->server->get('CONTENT_TYPE')
&& in_array(strtoupper($request->server->get('REQUEST_METHOD', 'GET'), array('PUT', 'DELETE'))
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You have an typo here (parse error), forgot to close bracket.

 if ('application/x-www-form-urlencoded' == $request->server->get('CONTENT_TYPE')
     && in_array(strtoupper($request->server->get('REQUEST_METHOD', 'GET')), array('PUT', 'DELETE'))
 ) {

@fabpot fabpot merged commit 0848604 into symfony:master May 12, 2011
@simensen
Copy link
Contributor

I think this is great and I think that createFromGlobals is where this belongs.

garak pushed a commit to garak/symfony-docs that referenced this pull request Nov 2, 2013
Chekote pushed a commit to Chekote/symfony that referenced this pull request Mar 4, 2014
…sts.

PHP, and by extension, Symfony does not support multipart/form-data requests when using any request method other than POST. This limits the ability to implement RESTful architectures using Symfony. This is a follow up to Pull Request symfony#849 I made a few, and  addresses concerns brought up by Saldaek at the time.

I have implemented this functionality by manually decoding the php://input stream when the request type is PUT, DELETE or PATCH and the Content-Type header is mutlipart/form-data. The implementation is based on an example by [netcoder at stackoverflow](http://stackoverflow.com/a/9469615).

This is necessary due to an underlying limitation of PHP, as discussed here: https://bugs.php.net/bug.php?id=55815.

This fixes symfony#9226.

__Security Concerns__

The main concern I had while implementing this feature, was working with PHP's assumptions as to what constituted an uploaded file. Since the files that this commit creates are not done so through PHP's usual uploaded file mechanisms, none of the uploaded file functions (such as is_uploaded_file, move_uploaded_file) will work with them. As such, I had to implement a mechanism for recording the uploaded files as they were created, so that Symfony's UploadedFile class could be sure that it was not moving non-uploaded files.

To achieve this, I added a static array to the UploadedFile class called $files. Any uploaded files that are created outside of PHP's normal mechanism can be recorded here. UploadedFile can then check this list to ensure that if is_uploaded_file returns false, that the file being moved is in fact an uploaded file. This results in two changes to UploadedFile: Firstly, the is_uploaded_file check had to be expanded to also check if the file was recorded in UploadedFile::$files. Secondly, the move_uploaded_file needed to be changed to rename.

The change of move_uploaded_file to rename might raise a few red flags, but I do not believe this is a problem. Before rename is called, UploadedFile calls isValid, which checks that the file either is_uploaded_file, or is a member of UploadedFile::$files. Therefore the use of move_uploaded_file is an unnecessary double check (even without these changes).

Fix symfony#9226.
Chekote pushed a commit to Chekote/symfony that referenced this pull request Mar 4, 2014
…sts.

PHP, and by extension, Symfony does not support multipart/form-data requests when using any request method other than POST. This limits the ability to implement RESTful architectures using Symfony. This is a follow up to Pull Request symfony#849 I made a few, and  addresses concerns brought up by Saldaek at the time.

I have implemented this functionality by manually decoding the php://input stream when the request type is PUT, DELETE or PATCH and the Content-Type header is mutlipart/form-data. The implementation is based on an example by [netcoder at stackoverflow](http://stackoverflow.com/a/9469615).

This is necessary due to an underlying limitation of PHP, as discussed here: https://bugs.php.net/bug.php?id=55815.

This fixes symfony#9226.

__Security Concerns__

The main concern I had while implementing this feature, was working with PHP's assumptions as to what constituted an uploaded file. Since the files that this commit creates are not done so through PHP's usual uploaded file mechanisms, none of the uploaded file functions (such as is_uploaded_file, move_uploaded_file) will work with them. As such, I had to implement a mechanism for recording the uploaded files as they were created, so that Symfony's UploadedFile class could be sure that it was not moving non-uploaded files.

To achieve this, I added a static array to the UploadedFile class called $files. Any uploaded files that are created outside of PHP's normal mechanism can be recorded here. UploadedFile can then check this list to ensure that if is_uploaded_file returns false, that the file being moved is in fact an uploaded file. This results in two changes to UploadedFile: Firstly, the is_uploaded_file check had to be expanded to also check if the file was recorded in UploadedFile::$files. Secondly, the move_uploaded_file needed to be changed to rename.

The change of move_uploaded_file to rename might raise a few red flags, but I do not believe this is a problem. Before rename is called, UploadedFile calls isValid, which checks that the file either is_uploaded_file, or is a member of UploadedFile::$files. Therefore the use of move_uploaded_file is an unnecessary double check (even without these changes).

Fix symfony#9226.
Chekote pushed a commit to Chekote/symfony that referenced this pull request Mar 4, 2014
…sts.

PHP, and by extension, Symfony does not support multipart/form-data requests when using any request method other than POST. This limits the ability to implement RESTful architectures using Symfony. This is a follow up to Pull Request symfony#849 I made a few years ago, and  addresses concerns brought up by Saldaek at the time.

I have implemented this functionality by manually decoding the php://input stream when the request type is PUT, DELETE or PATCH and the Content-Type header is mutlipart/form-data. The implementation is based on an example by [netcoder at stackoverflow](http://stackoverflow.com/a/9469615).

This is necessary due to an underlying limitation of PHP, as discussed here: https://bugs.php.net/bug.php?id=55815.

This fixes symfony#9226.

__Security Concerns__

The main concern I had while implementing this feature, was working with PHP's assumptions as to what constituted an uploaded file. Since the files that this commit creates are not done so through PHP's usual uploaded file mechanisms, none of the uploaded file functions (such as is_uploaded_file, move_uploaded_file) will work with them. As such, I had to implement a mechanism for recording the uploaded files as they were created, so that Symfony's UploadedFile class could be sure that it was not moving non-uploaded files.

To achieve this, I added a static array to the UploadedFile class called $files. Any uploaded files that are created outside of PHP's normal mechanism can be recorded here. UploadedFile can then check this list to ensure that if is_uploaded_file returns false, that the file being moved is in fact an uploaded file. This results in two changes to UploadedFile: Firstly, the is_uploaded_file check had to be expanded to also check if the file was recorded in UploadedFile::$files. Secondly, the move_uploaded_file needed to be changed to rename.

The change of move_uploaded_file to rename might raise a few red flags, but I do not believe this is a problem. Before rename is called, UploadedFile calls isValid, which checks that the file either is_uploaded_file, or is a member of UploadedFile::$files. Therefore the use of move_uploaded_file is an unnecessary double check (even without these changes).

Fix symfony#9226.
Chekote pushed a commit to Chekote/symfony that referenced this pull request Mar 4, 2014
PHP, and by extension, Symfony does not support multipart/form-data requests when using any request method other than POST. This limits the ability to implement RESTful architectures using Symfony. This is a follow up to Pull Request symfony#849 I made a few years ago, and  addresses concerns brought up by Saldaek at the time.

I have implemented this functionality by manually decoding the php://input stream when the request type is PUT, DELETE or PATCH and the Content-Type header is mutlipart/form-data. The implementation is based on an example by [netcoder at stackoverflow](http://stackoverflow.com/a/9469615).

This is necessary due to an underlying limitation of PHP, as discussed here: https://bugs.php.net/bug.php?id=55815.

This fixes symfony#9226.

__Security Concerns__

The main concern I had while implementing this feature, was working with PHP's assumptions as to what constituted an uploaded file. Since the files that this commit creates are not done so through PHP's usual uploaded file mechanisms, none of the uploaded file functions (such as is_uploaded_file, move_uploaded_file) will work with them. As such, I had to implement a mechanism for recording the uploaded files as they were created, so that Symfony's UploadedFile class could be sure that it was not moving non-uploaded files.

To achieve this, I added a static array to the UploadedFile class called $files. Any uploaded files that are created outside of PHP's normal mechanism can be recorded here. UploadedFile can then check this list to ensure that if is_uploaded_file returns false, that the file being moved is in fact an uploaded file. This results in two changes to UploadedFile: Firstly, the is_uploaded_file check had to be expanded to also check if the file was recorded in UploadedFile::$files. Secondly, the move_uploaded_file needed to be changed to rename.

The change of move_uploaded_file to rename might raise a few red flags, but I do not believe this is a problem. Before rename is called, UploadedFile calls isValid, which checks that the file either is_uploaded_file, or is a member of UploadedFile::$files. Therefore the use of move_uploaded_file is an unnecessary double check (even without these changes).

Fix symfony#9226.
Chekote pushed a commit to Chekote/symfony that referenced this pull request Mar 4, 2014
PHP, and by extension, Symfony does not support multipart/form-data requests when using any request method other than POST. This limits the ability to implement RESTful architectures using Symfony. This is a follow up to Pull Request symfony#849 I made a few years ago, and  addresses concerns brought up by Saldaek at the time.

I have implemented this functionality by manually decoding the php://input stream when the request type is PUT, DELETE or PATCH and the Content-Type header is mutlipart/form-data. The implementation is based on an example by [netcoder at stackoverflow](http://stackoverflow.com/a/9469615).

This is necessary due to an underlying limitation of PHP, as discussed here: https://bugs.php.net/bug.php?id=55815.

This fixes symfony#9226.

__Security Concerns__

The main concern I had while implementing this feature, was working with PHP's assumptions as to what constituted an uploaded file. Since the files that this commit creates are not done so through PHP's usual uploaded file mechanisms, none of the uploaded file functions (such as is_uploaded_file, move_uploaded_file) will work with them. As such, I had to implement a mechanism for recording the uploaded files as they were created, so that Symfony's UploadedFile class could be sure that it was not moving non-uploaded files.

To achieve this, I added a static array to the UploadedFile class called $files. Any uploaded files that are created outside of PHP's normal mechanism can be recorded here. UploadedFile can then check this list to ensure that if is_uploaded_file returns false, that the file being moved is in fact an uploaded file. This results in two changes to UploadedFile: Firstly, the is_uploaded_file check had to be expanded to also check if the file was recorded in UploadedFile::$files. Secondly, the move_uploaded_file needed to be changed to rename.

The change of move_uploaded_file to rename might raise a few red flags, but I do not believe this is a problem. Before rename is called, UploadedFile calls isValid, which checks that the file either is_uploaded_file, or is a member of UploadedFile::$files. Therefore the use of move_uploaded_file is an unnecessary double check (even without these changes).

Fix symfony#9226.
@Chekote
Copy link

Chekote commented Mar 4, 2014

Sorry for the reference spam!

Chekote pushed a commit to Chekote/symfony that referenced this pull request Mar 5, 2014
PHP, and by extension, Symfony does not support multipart/form-data requests when using any request method other than POST. This limits the ability to implement RESTful architectures using Symfony. This is a follow up to Pull Request symfony#849 I made a few years ago, and  addresses concerns brought up by Saldaek at the time.

I have implemented this functionality by manually decoding the php://input stream when the request type is PUT, DELETE or PATCH and the Content-Type header is mutlipart/form-data. The implementation is based on an example by [netcoder at stackoverflow](http://stackoverflow.com/a/9469615).

This is necessary due to an underlying limitation of PHP, as discussed here: https://bugs.php.net/bug.php?id=55815.

This fixes symfony#9226.

__Security Concerns__

The main concern I had while implementing this feature, was working with PHP's assumptions as to what constituted an uploaded file. Since the files that this commit creates are not done so through PHP's usual uploaded file mechanisms, none of the uploaded file functions (such as is_uploaded_file, move_uploaded_file) will work with them. As such, I had to implement a mechanism for recording the uploaded files as they were created, so that Symfony's UploadedFile class could be sure that it was not moving non-uploaded files.

To achieve this, I added a static array to the UploadedFile class called $files. Any uploaded files that are created outside of PHP's normal mechanism can be recorded here. UploadedFile can then check this list to ensure that if is_uploaded_file returns false, that the file being moved is in fact an uploaded file. This results in two changes to UploadedFile: Firstly, the is_uploaded_file check had to be expanded to also check if the file was recorded in UploadedFile::$files. Secondly, the move_uploaded_file needed to be changed to rename.

The change of move_uploaded_file to rename might raise a few red flags, but I do not believe this is a problem. Before rename is called, UploadedFile calls isValid, which checks that the file either is_uploaded_file, or is a member of UploadedFile::$files. Therefore the use of move_uploaded_file is an unnecessary double check (even without these changes).

Fix symfony#9226.

Conflicts:
	src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
Chekote pushed a commit to Chekote/symfony that referenced this pull request Mar 10, 2014
PHP, and by extension, Symfony does not support multipart/form-data requests when using any request method other than POST. This limits the ability to implement RESTful architectures using Symfony. This is a follow up to Pull Request symfony#849 I made a few years ago, and  addresses concerns brought up by Saldaek at the time.

I have implemented this functionality by manually decoding the php://input stream when the request type is PUT, DELETE or PATCH and the Content-Type header is mutlipart/form-data. The implementation is based on an example by [netcoder at stackoverflow](http://stackoverflow.com/a/9469615).

This is necessary due to an underlying limitation of PHP, as discussed here: https://bugs.php.net/bug.php?id=55815.

This fixes symfony#9226.

__Security Concerns__

The main concern I had while implementing this feature, was working with PHP's assumptions as to what constituted an uploaded file. Since the files that this commit creates are not done so through PHP's usual uploaded file mechanisms, none of the uploaded file functions (such as is_uploaded_file, move_uploaded_file) will work with them. As such, I had to implement a mechanism for recording the uploaded files as they were created, so that Symfony's UploadedFile class could be sure that it was not moving non-uploaded files.

To achieve this, I added a static array to the UploadedFile class called $files. Any uploaded files that are created outside of PHP's normal mechanism can be recorded here. UploadedFile can then check this list to ensure that if is_uploaded_file returns false, that the file being moved is in fact an uploaded file. This results in two changes to UploadedFile: Firstly, the is_uploaded_file check had to be expanded to also check if the file was recorded in UploadedFile::$files. Secondly, the move_uploaded_file needed to be changed to rename.

The change of move_uploaded_file to rename might raise a few red flags, but I do not believe this is a problem. Before rename is called, UploadedFile calls isValid, which checks that the file either is_uploaded_file, or is a member of UploadedFile::$files. Therefore the use of move_uploaded_file is an unnecessary double check (even without these changes).

Fix symfony#9226.

Conflicts:
	src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
Chekote pushed a commit to Chekote/symfony that referenced this pull request Mar 19, 2014
PHP, and by extension, Symfony does not support multipart/form-data requests when using any request method other than POST. This limits the ability to implement RESTful architectures using Symfony. This is a follow up to Pull Request symfony#849 I made a few years ago, and  addresses concerns brought up by Saldaek at the time.

I have implemented this functionality by manually decoding the php://input stream when the request type is PUT, DELETE or PATCH and the Content-Type header is mutlipart/form-data. The implementation is based on an example by [netcoder at stackoverflow](http://stackoverflow.com/a/9469615).

This is necessary due to an underlying limitation of PHP, as discussed here: https://bugs.php.net/bug.php?id=55815.

This fixes symfony#9226.

__Security Concerns__

The main concern I had while implementing this feature, was working with PHP's assumptions as to what constituted an uploaded file. Since the files that this commit creates are not done so through PHP's usual uploaded file mechanisms, none of the uploaded file functions (such as is_uploaded_file, move_uploaded_file) will work with them. As such, I had to implement a mechanism for recording the uploaded files as they were created, so that Symfony's UploadedFile class could be sure that it was not moving non-uploaded files.

To achieve this, I added a static array to the UploadedFile class called $files. Any uploaded files that are created outside of PHP's normal mechanism can be recorded here. UploadedFile can then check this list to ensure that if is_uploaded_file returns false, that the file being moved is in fact an uploaded file. This results in two changes to UploadedFile: Firstly, the is_uploaded_file check had to be expanded to also check if the file was recorded in UploadedFile::$files. Secondly, the move_uploaded_file needed to be changed to rename.

The change of move_uploaded_file to rename might raise a few red flags, but I do not believe this is a problem. Before rename is called, UploadedFile calls isValid, which checks that the file either is_uploaded_file, or is a member of UploadedFile::$files. Therefore the use of move_uploaded_file is an unnecessary double check (even without these changes).

Fix symfony#9226.

Conflicts:
	src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants