Create Http context handler

We now have the HttpFoundation library in WSCCI per #1290504: Merge in Symfony2 and PSR-0 autoloader, so this issue is now unblocked and critical.

Any progress here? I'd love to be able to review code, even if it's preliminary...

+++ b/includes/Drupal/Context/Handler/HandlerHTTP.php
@@ -0,0 +1,89 @@
+class HandlerHTTP extends \Drupal\Context\Handler\HandlerAbstract {

The HandlerAbstract class should be imported with a use statement, and then referenced by its short name.

Also, minor quibble. Symfony uses Http, not HTTP, so we probably should as well for consistency.

+++ b/includes/Drupal/Context/Handler/HandlerHTTP.php
@@ -0,0 +1,89 @@
+  protected $symfony_request = NULL;

Properties of an object should be in lowerCamel case. That is, $symfonyRequest. (Although just $request is probably fine here.)

+++ b/includes/Drupal/Context/Handler/HandlerHTTP.php
@@ -0,0 +1,89 @@
+  static $http_propetries = array(

See above. Also, this should be static protected, not just static, or else it becomes public. "properties" is also misspelled.

+++ b/includes/Drupal/Context/Handler/HandlerHTTP.php
@@ -0,0 +1,89 @@
+    $this->context = $context;
+    $this->params = $params;

This is already done by the parent constructor, which should be called first. parent::__construct($context, $params).

Also, the context reference is going to move to the getValue() method shortly. Just an FYI.

+++ b/includes/Drupal/Context/Handler/HandlerHTTP.php
@@ -0,0 +1,89 @@
+      // Init Symfony Request.
+      if (empty($this->symfony_request)) {
+        $this->symfony_request = \Symfony\Component\HttpFoundation\Request::createFromGlobals();
+      }

This is guaranteed to be empty when the object is constructed, so the empty() check is unnecessary.

+++ b/includes/Drupal/Context/Handler/HandlerHTTP.php
@@ -0,0 +1,89 @@
+      switch ($propetry) {

$property is misspelled.

Also, I'm not sure that the constructor is the appropriate place to pull this data out. Much of that information will likely never be requested, so there's no need to extract it from the request object at all. This switch statement (or whatever it turns into) should be in getValue().

+++ b/includes/Drupal/Context/Handler/HandlerHTTP.php
@@ -0,0 +1,89 @@
+  public function getValue(array $args = array()) {
+    return $this->params[implode(':', $args)];
+  }

This should actually just use $args[0]. If $args has extra elements at the end, because the caller did something silly, it will break as there will be no such key as "request_args:blargh", will there?

See #1262020: improve getting of butler params for further discussion of that problem.

+++ b/modules/simpletest/tests/context.test
@@ -425,3 +425,29 @@ class ContextMockTestCase extends ContextTestCases {
+    $butler->registerHandler('http', '\Drupal\Context\Handler\HandlerHTTP');
+    $butler->lock();

We probably want some way to allow the handler to be populated with a self-crafted request object. HttpFoundation\Request supports that (there's another factory method for it), and we will want that for testing.

Since we cannot pass new constructor arguments directly to a constructor object at registration, perhaps we can make use of a flag in the $params array and pass in either alternate arrays to use to populate the request object, or a pre-created request object. The latter won't work in the root context object, though, since we want that to load straight from disk.

Hm. Pounard is probably going to laugh at me for this, but perhaps testing is a place where registering a pre-instantiated object is appropriate. :-) Not sure there, but perhaps.

Thanks, Yuriy!

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,88 @@
+  public function __construct(\Drupal\Context\ContextInterface $context, $params = array()) {

This can also be simplified with a use statement.

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,88 @@
+    $this->request = \Symfony\Component\HttpFoundation\Request::createFromGlobals();

This can also be simplified with a use statement.

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,88 @@
+        case 'query_args':
+          $this->params[$property] = $this->request->query->all();
+          break;

This would return an array, would it not? I believe in most cases we would want back an individual elements based on $arg[1]. This is where I think it should be pushed to a separate internal method rather than a switch statement.

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,88 @@
+        case 'files':
+          $this->params[$property] = $this->request->files->all();
+          break;

Should this perhaps also require an arg[1], and return info on that particular file? (Specified by, uh, not sure. Name of the field?)

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,88 @@
+        case 'cookies':
+          $this->params[$property] = $this->request->cookies->all();
+          break;
+        case 'headers':
+          $this->params[$property] = $this->request->headers->all();
+          break;
+        case 'server':
+          $this->params[$property] = $this->request->server->all();
+          break;

Same here. I'd rather see $c['http:header:bah'] to get back the "bah" header, for instance.

+++ b/modules/simpletest/tests/context.test
@@ -425,3 +425,48 @@ class ContextMockTestCase extends ContextTestCases {
+  function testGetHttpProperty() {
+    $butler = new \Drupal\Context\Context();
+
+    $butler->registerHandler('http', '\Drupal\Context\Handler\HandlerHttp');
+    $butler->lock();
+
+    $method = $butler['http:method'];
+
+    $this->assertEqual($method, 'POST', t('Correct Http method property found.'));
+
+    debug($butler['http:domain']);
+  }

This is the part where I think we do need to be able to alter the request object. If this unit test is run from drush, will it pass? I don't know if drush is simulating an http request, but if it's not then this test is still dependent on the global test environment.

That's where I was thinking we could pass in a fake GET array as a param to the handler, and it would use that to call Request::create() rather than Request::createfromGlobals(). If you pass in no params, it just calls createFromGlobals() (which would be the case pretty much everywhere except in unit tests).

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,126 @@
+use \Drupal\Context\ContextInterface as ContextInterface,
+    \Symfony\Component\HttpFoundation\Request as Request,
+    \Drupal\Context\Handler;

We have no formal standard here yet, but I think my preference would be one use per line, and if you're not renaming the class the "as Foo" should be omitted.

I should probably open a coding standards ticket for this, but I'm hesitant to do so before the Symfony2 patch is in.

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,126 @@
+class HandlerHTTP extends HandlerAbstract {

For consistency I think this should be HttpHandler. I don't know that we have a formal standard yet, though.

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,126 @@
+    // Init Symfony Request.
+    if (!empty($params)) {
+      // Default values.
+      $uri = 'http://localhost';
+      $method = 'GET';
+      $parameters = array();
+      $cookies = array();
+      $files = array();
+      $server = array();
+      $content = NULL;
+
+      // Extract values.
+      extract($params);
+
+      $this->request = Request::create($uri, $method, $parameters, $cookies, $files, $server, $content);

The extract() makes me scared, as it's a dangerous function with potentially oddball side-effects. What if instead of using all $params as HTTP overrides, we do something like $params['overrides'], as an array? If there's anything in it, we then just += an array of the default values and then use that. (The default values listed here look fine.)

We could probably use some documentation here as to why we're doing this, too. I can help with that if you'd like.

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,126 @@
+    // Check whether requested prperty is known.

Property is missing an O. :-)

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,126 @@
+        case 'request_args':
+          $this->params[$property] = $this->request->request->all();
+          break;

I know Symfony uses the term "request" to refer to the POST variables, but I don't think that's particularly intuitive or self-documenting. We should come up with something else here to refer to "stuff that you used to get from $_POST".

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,126 @@
+        case 'query_args':
+          $this->params[$property] = $this->request->query->all();
+          break;

Completely personal preference: query instead of query_args? We're going to be typing this a lot, and I'd rather not have the _ in there. (Consensus can overrule me, though.)

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,126 @@
+        case 'headers':
+          $this->params[$property] = $this->request->headers->all();
+          $this->arrayCleanup($this->params[$property]);
+          break;

Is an array of all headers useful, or would we want a specific header by name? http:header:someheadername?

What about http:headers is an array, and http:header:name is just the value of that header (or empty string if it's not set)?

The same would be true for cookie, file, and server, I think.

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,126 @@
+    // If parameter is array we use $args to get proper value of the array.
+    array_shift($args);
+    return drupal_array_get_nested_value($this->params[$property], $args);

Hm. I'm not sure I like this part. It seems like it would make the return value inconsistent. I don't know that we'd want to allow indefinite nesting into the keys of an array parameter.

In fact, I'm inclined to say that we don't, since the array itself is the value of the context; that's the atomic value, so that's the level at which we should "cut" and return something.

Also, I'd like to avoid pulling random Drupal utility functions into handlers, especially low-level ones, as that's a higher-level Drupal-specific dependency. The fewer of those we have, the better. (Remember that function is only available after boostrap or common.inc are loaded, and we want to dismantle those.)

I think the main question is how "raw" the data we get back should be. I'd like to offer at least a little more than just wrapping calls to HttpFoundation. However, I could see an argument that something like http:header:foo should be its own separate handler, which just references back to http:headers. Thoughts?

Hm, interesting. Is there a compelling reason we could not follow Symfony's example here? Or if Symfony is violating the W3 / ISO standard, should we push back on them about it?

I suggest we carry forward in this thread without any language case folding in the HTTP context handler itself. Just return what Symfony gives us for now. We can open a separate discussion, probably with the Symfony project directly on GitHub, about what HttpFoundation should be returning and why.

(If we need to do any re-mapping of language strings, we can do that in a separate langauge:whatever handler.)

Any other reviews of #25?

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,124 @@
+  public function __construct($params = array()) {
+    // Init Symfony Request.
+    if (!empty($params)) {
+      // Set default values.
+      $params += array(
+        'uri' => 'http://localhost',
+        'method' => 'GET',
+        'parameters' => array(),
+        'cookies' => array(),
+        'files' => array(),
+        'server' => array(),
+        'content' => NULL,
+      );

I think this should be a sub-parameter of $params, in case we ever have need to add more things that are not overrides. So $params['values'] or something like that. Also, if we use isset() instead of !empty() I think that should allow through an empty array, which allows us to create an "empty" request object by passing in an empty array there. (I don't know when we'd want to do that, but I could see a use for testing.)

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,124 @@
+        case 'request_args':
+          $this->params[$property] = $this->request->request->all();
+          break;

This maps to what was in $_POST, right? I'm a bit concerned that people will confuse request_args with $_REQUEST, instead, which is different in important ways. Is there some other name here that would make sense? (Lukas, your input here on DX would be helpful since you've worked with HttpFoundation more.)

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,124 @@
+        case 'query_args':
+          $this->params[$property] = $this->request->query->all();
+          break;

My knee-jerk feeling here is that "query" is better than "query_args", since people will be typing it a lot.

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,124 @@
+    // If $this->params[$property] is value and
+    // we don't have second argument passed.
+    if (!is_array($this->params[$property]) || !isset($args[1])) {
+      return $this->params[$property];
+    }
+    else {
+      if (!empty($args[1]) && isset($this->params[$property][$args[1]])) {
+        return $this->params[$property][$args[1]];
+      }
+      else {
+        // We return empty string if there is no
+        // second argument key in $this->params[$property].
+        return '';
+      }
+    }

It took me a while to understand the value here. This is actually really clever, but not obvious why it's so clever.

This allows a uniform way of accessing an individual query arg, individual header, individual file, etc., all with the same code. That's quite slick, and I didn't realize until now why you were pushing for this array checking stuff. Now that I get it (from looking at the unit test), I really really like it. But that means that it needs a better documentation paragraph to explain why we're doing it, so that other people don't get confused.

As for language, let's do what Lukas suggests in #32 for now until Symfony and Drupal can standardize the string format. If/when we change it later, it won't be an API break (aside from the string changing, but it won't break the context API at all).

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,127 @@
+          // Cleanup from not necessary nesting level.

"not necessary" should be "unnecessary".

+++ b/includes/Drupal/Context/Handler/HandlerHttp.php
@@ -0,0 +1,127 @@
+    // We support only two levels of nexting.

"nexting" should be "nesting".

I'm open to suggestions on query_args/get/request_args/post/etc.

Yuriy, if it's OK with you once that's sorted out I'll just go ahead and add OCD documentation myself, then commit this. :-)

Per today's IRC meeting:

query_args should become "query".

request_args and request_body will remain as is for now.

I'll work on beefing up the docs shortly. frankcarey volunteered to review them once I'd done so. :-)

I have cleaned up the documentation on this branch and wired it up in system.module. (yeah, yeah, I know, system.module.) Also, I discovered that one of the tests would not and never would work when tests are run from the command line, so I just set that one to get skipped from the CLI.

Thanks, ygerasimov!