Skip to content

Kentr add server vars #16

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

Closed
wants to merge 2 commits into from
Closed

Conversation

kentr
Copy link

@kentr kentr commented Jan 26, 2016

No description provided.

@kentr kentr mentioned this pull request Jan 26, 2016
@andig
Copy link
Contributor

andig commented Jan 26, 2016

That might work. Where do you get $parameters from? Has this been tested?

@kentr
Copy link
Author

kentr commented Jan 26, 2016

I tested manually with some basic GET and POST requests for a Drupal app, and inspected via debugger. If there are other required tests, I'll need some pointers on testing further.

The resulting values for $syRequest->headers and $syRequest->server are in this comment.

$parameters is set to either $query or $post, depending on $method.

That's based on the assumption that Request::create() expects it like that and uses $parameters to populate Request::$query or Request::$request depending on $method.

The code I'm looking at is \Symfony\Component\HttpFoundation\Request::create(), ~ line 361:

        switch (strtoupper($method)) {
            case 'POST':
            case 'PUT':
            case 'DELETE':
                if (!isset($server['CONTENT_TYPE'])) {
                    $server['CONTENT_TYPE'] = 'application/x-www-form-urlencoded';
                }
                // no break
            case 'PATCH':
                $request = $parameters;
                $query = array();
                break;
            default:
                $request = array();
                $query = $parameters;
                break;
        }

@kentr
Copy link
Author

kentr commented Jan 26, 2016

Based on the above code, it would probably be better to initialize $parameters with something like this:

$parameters = in_array($method, ['POST', 'PUT', 'DELETE', 'PATCH']) ? $request : $query;

Thoughts?

@marcj
Copy link
Member

marcj commented Feb 17, 2016

Well, $parameters can now be querString params or parsed parameters from the request body? What if you have both? A POST with body and QueryString?

@kentr
Copy link
Author

kentr commented Feb 17, 2016

BTW, there's some background on this PR in #14.

Well, $parameters can now be querString params or parsed parameters from the request body? What if you have both? A POST with body and QueryString?

Good question.

SymfonyRequest::create() appears to extract the query string from the $uri parameter.

In this PR, that mechanism is failing because $uri doesn't contain the query string.

Here's a gist with a correction for the problem and some other pieces filled in.

Expected Results

Here's a baseline of an actual Request object while running under nginx / PHP-FPM. The request was created with Request::createFromGlobals() (as part of the standard Drupal request flow).

Actual Results

This is the request object under PHP-PM resulting from the suggested mapRequest() code.

@marcj
Copy link
Member

marcj commented Feb 17, 2016

Beside the ServerBag this look pretty good to me!

@kentr
Copy link
Author

kentr commented Feb 18, 2016

Deleted comment. Wrong thread.

@andig
Copy link
Contributor

andig commented Mar 16, 2016

Beside the ServerBag this look pretty good to me!

I would say thats ok since the current version always initalizes server to be empty. +1 for merging from my side (though untested- it it breaks we roll it back).

@marcj
Copy link
Member

marcj commented Mar 18, 2016

Pushed today some changes that should now completely populate the Symfony Request with all its needed values ($post, $query, $files, $server, etc)
(see also #14 (comment))

Guess this is obsolete now. Feel free to ping me back if something is missing. :)

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.

3 participants