Problem/Motivation

We want to actually, you know, use the full HTTP spec. That's kinda important for doing anything with web services, sending or receiving, although right now we're focused on receiving for the Web Services initiative. We also want to avoid lots and lots of globals and superglobals, since those make testing, debugging, and mocking difficult to impossible.

Doing that in a sane way takes a lot of work to build a wrapper library. We don't want to spend that kind of time.

Proposed resolution

Based on the research and discussion here, we are going to integrate the Symfony2 HttpFoundation library. That is, we're going to just bundle it with core, along with the Symfony2 autoload routines to make the code available. In practice it will almost always be accessed via the Context object, but that's for a later patch.

Remaining tasks

There's a number of follow-up issues that could benefit from this move:

#335411: Switch to Symfony2-based session handling (Use HttpFoundation's session handling, at least in part)
#1260864: Convert global $user to a context key (The user object is tightly coupled to the session handling, which is arguably itself a problem...)
#1260918: Convert language globals to contexts (HttpFoundation offers nice HTTP language negotiation capabilities)
#1279942: Create Http context handler (Offer much more robust Http information to core code via context)
#1279944: Eliminate arg() in favor of context (We can kill arg()!!!!)
#1279954: Assume Javascript delivery if XmlHttpRequest header set

User interface changes

None, this is an API/framework addition only.

API changes

For the moment, none. We still need to build the context integration atop it. Eventually, though, any use of $_GET, $_POST, etc. will be considered a bug, and all HTTP information will be accessed via context.

Original report by Crell

As previously discussed, it would be nice to have a more robust HTTP library available to us than the superglobals. Really, those suck. It would be great if it were incoming and outgoing, but at minimum we want a good, ext/filter-using library for the incoming HTTP request.

nihiliad and I had discussed using the PECL HTTP library in the past. While that would be an unpleasant dependency, if we could port it to user space that would be a great way to have our cake and eat it too. Just drop in the PECL module instead of the PHP-space version and poof, more of Drupal is in C code.

However, on further investigation that port would be considerably more work than we thought. Enough that it's probably not worth the effort. So we should investigate other possible user-space libraries we can employ so that we do not need to write it from scratch.

Zend 2 apparently has an HTTP library that doesn't suck and is worth looking into, but others may exist. If we use the Zend one it's likely we'd need to tweak it a little to remove any dependencies, and get a license exemption since it's essentially BSD-licensed.

Files: 
CommentFileSizeAuthor
#136 1178246-symfony2-changelog.patch546 bytesCrell
PASSED: [[SimpleTest]]: [MySQL] 33,651 pass(es).
[ View ]
#130 1178246-symfony2.patch208.34 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 33,631 pass(es).
[ View ]
#123 1178246-symfony2.patch208.32 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 33,301 pass(es).
[ View ]
#44 1178246-symfony2.patch206.02 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 32,901 pass(es).
[ View ]
#34 1178246-symfony2.patch204.62 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 32,922 pass(es).
[ View ]

Comments

Crell’s picture

Note: A critical requirement for this library is that it can be easily mocked. That is, a CLI app (Drush) could easily provide HTTP-esque information so that other code doesn't realize that it's not in an HTTP environment.

catch’s picture

Why is porting the PECL library to Drupal so hard?

If it supports a lot of features that Drupal doesn't need, then couldn't we just port the ones we need - but leave it compatible with the PECL library so that can be used when available?

voxpelli’s picture

So what globals is it that you want this class to wrap? All HTTP-headers + REQUEST_METHOD in $_SERVER + possibly the $_GET and $_POST?

Looking at what we've done in Services I think we don't want to expose a library like the PECL library directly in the code but rather wrap it within our own class. That's because we probably want to expose more than just raw HTTP - we probably want to do some processing and parsing of the requests to eg. unify different request alternatives. Like making these two look the same:

POST http://example.com/cool-page.xml?_method=PUT

PUT http://example.com/cool-page
Accept: application/xml

Also things like parsing the accept headers is probably something that should be built in since that parsing is really complicated. A helper for finding out what response format a request prefers rather than only exposing the raw accept-header.

I think the related code in Services can be found in this method: http://drupalcode.org/project/services.git/blob/refs/heads/7.x-3.x:/serv...

So all in all - I don't think we need to build a very advanced HTTP-library as I don't think we should expose it directly but rather just wrap it. Makes sense?

catch’s picture

I somewhat assumed we'd be wrapping either the PECL or the user-space library, but yeah I'd definitely push for it wrapping, even if we're only wrapping something we ended up writing or forking ourselves initially.

If the intention is to use this library all the way down in _drupal_bootstrap_configuration() then there is going to be no way to make it pluggable.

Crell’s picture

The original intent was actually that we wouldn't wrap it, but treat it as if it were "there". The PECL module works for outgoing requests, too, which would make it viable for replacing drupal_http_request(). As far as Butler/WSCCI itself goes, technically all we need is to make all of the incoming HTTP information available in a clean, extensible fashion.

voxpelli: That link looks like the routing code, not the HTTP wrapping code.

Parsing out headers is part of what could/should be done in such a library. Filtering with ext/filter is another thing that arguably should be done here. The main goal, though, is yes to hide all superglobals so that we can normalize them between Apache, CLI, and other PHP SAPIs. (aka, web mode and Drush mode.)

glennpratt’s picture

Notes / subscribe.

Yii (which is interesting itself) CHttpRequest.

Simple usage normalizing superglobals ChiveHttpRequest.

I'm working on porting a small Services 3.x utility from PEAR HTTP_Request to PEAR HTTP_Request2. Not sure it would map to a server side implementation, but I will investigate.

catch’s picture

http://pecl.php.net/package/pecl_http has a new 2.0-dev release.

This from the pecl list:

The new PECL package pecl_http-2.0.0dev2 (devel) has been released at http://pecl.php.net/.

Release notes
-------------
This is to become v2 of the known pecl_http extension.
It is completely incompatible to previous version.
Try it, or let it be. If you are not sure, let it be. Really.

List of changes (TBD):
* Everything lives below the http namespace
* Supported request libraries: curl, neon
* The message body is implemented as a temp stream instead of a chunk of memory
* The utterly misunderstood HttpResponse class has been reimplemented in the http\env namespace
* There's only http\Exception
* Every instance follows http\Object::$defaultErrorHandling or inherited http\Object->errorHandling, but only for errors generated by the extension itself

Package Info
-------------
Extended HTTP support. Again.

Related Links
-------------
Package home: http://pecl.php.net/package/pecl_http
  Changelog: http://pecl.php.net/package-changelog.php?package=pecl_http
   Download: http://pecl.php.net/get/pecl_http-2.0.0dev2.tgz

Authors
-------------
Michael Wallner <...> (lead)

If it's still in the running it might be worth looking at again with this new version.

Off-topic, this is the kind of discussion I think could benefit from being in the core queue rather than tucked away in here.

voxpelli’s picture

@Crell: There's no separation between any HTTP wrapping and routing in Services currently - could still be interesting to look at what parts of HTTP it utilizes and what preprocessing it does and perhaps make it possible for Services to use the same HTTP library as used here - right?

Crell’s picture

Project:Butler» Drupal core
Version:7.x-1.x-dev» 8.x-dev
Component:Code» base system

voxpelli: Definitely. If services can act as a "leading edge" here that would be very helpful, even though we are actively trying to separate HTTP wrapping from routing.

catch: Eh, valid point. Refiling over to core.

Crell’s picture

BeCircle from Twitter pointed me toward the Symfony 2 HttpFoundation Request class. This looks promising. Does someone want to look into it and give us a report?

http://api.symfony.com/2.0/Symfony/Component/HttpFoundation/Request.html

mikl’s picture

How about just using curl? Most PHP distributions I've seen has the curl module installed :)

voxpelli’s picture

@mikl: Curl is only for making requests - not for abstracting incoming HTTP data. So it can't replace any superglobals which this issue is ultimately about :)

jherencia’s picture

Sub.

omissis’s picture

I am already using it in a Symfony2 project and it looks great, moreover at first sight it doesn't seem to have any external dependencies outside the Symfony\Component\HttpFoundation namespace, therefore I'd say go for it.

EDIT: I took a quick deeper look in the class and aside the \Symfony\Component\HttpFoundation\ResponseHeaderBag, it uses \InvalidArgumentException, \RuntimeException, \DateTime and \DateTimeZone classes, all present in the root namespace.

Crell’s picture

\InvalidArgumentException, \RuntimeException, \DateTime and \DateTimeZone are all part of PHP itself as of PHP 5.2. The \ is just how you have to reference global classes in namespaced code. My concern is whether it being namespaced would cause us any issues, since Drupal doesn't know what it's doing with namespaces yet, if anything.

The license for is it BSD-ish, so we'd need to discuss a license exception, and probably talk to the Symfony folks to make sure they're all happy as well.

Are there any other libraries we should consider, such as Zend?

jbrown’s picture

There are no problems with accessing namespaced code from un-namespaced code.

You would just put

use Symfony\Component\HttpFoundation;

at the top of any file that needs to access classes, functions or constants in that namespace.

Such items would then just need to be prefixed with HttpFoundation\

lsmith77’s picture

@Crell: why would you need a license exemption? Both BSD and MIT are GPL compatible: http://en.wikipedia.org/wiki/GNU_General_Public_License#Compatibility_an...

For Zend the bigger issue is that they require signing a CLA if you want to contribute anything back.

Is Drupal 8 going to be PHP 5.3?

Crell’s picture

Drupal 8 will be PHP 5.3-based, yes.

Drupal policy is that we only allow GPL code into the repository. We occasionally make exceptions for 3rd party libraries, but not often; the usual preference is to ask the 3rd party to dual-license their code. It's not an exception from the 3rd party we need, just from Drupal's own higher-ups. :-)

lsmith77’s picture

Ah ok.

If you want to play with the HTTP Foundation stuff of Symfony2, it might be easier to evaluate their use in Silex: http://silex-project.org/

Crell’s picture

I do still mean to get back to this if no one beats me to it, but someone please beat me to it. :-(

As an update, I have checked with Dries and he is OK with using a 3rd party library in this fashion, so the licensing question shouldn't be an issue.

EugenMayer’s picture

To be honest Crell i very suprised by this approach, but rather in the positive ways. Reusing that library gets a big plus by me.

mikeytown2’s picture

In terms of a drupal_http_request replacement, I've come up with some code that uses stream_select to "ping" a url in the background and to also issue multiple http requests at once, waiting till all of them come back.

The fast "ping" code (1,000 http requests sent in 100ms) is currently being used in Advanced CSS/JS Aggregation as a way to build the aggregates in the background while the page is still getting rendered. This could be used to build various caches after a cache flush, priming ESI, etc...

The "multiple http requests at once" code still hasn't found a home, but there is some test code that does work, demonstrating the advantage async has when pulling from multiple http streams at once. Feeds are the only thing that comes to mind for this code, nonetheless I find it interesting.

Not exactly sure how this all fits in. Just letting everyone know there is some interesting code out there for making async http requests in php.

Crell’s picture

In the WSCCI meeting today, we put together a list of what we want in a library. Notes from the meeting are as follows:

Things we want in the HTTP library:
* Secure-by-default (like Drupal generally). If it uses ext/filter, so much the better.
* Clean access to raw HTTP headers, not just the usual suspects (GET and POST). We want to be able to act on all kinds of headers.
* Good API for parsed HTTP data (eg, access headers are more complex than they sound)
* Cleanly extensible, because we know we'll want to do stuff with it and we don't want to touch existing code.
* Lean, mean, fast.
* Optional bonus: Proxy-capable HTTP sending ability (i.e., to replace drupal_http_request()).
* Open license. Zend requires a Contributor License Agreement (you have to sign something before you can send in a patch—only an issue if we want to submit fixes back upstream), Symfony does not. Unless there's a major difference in capability that may bias us toward Symfony.

Candidate libraries:
* Zend Framework 2: Http: https://github.com/zendframework/zf2/tree/master/library/Zend/Http
* Symfony: https://github.com/symfony/HttpFoundation

skwashd and dixon_ volunteered to investigate those libraries and report back this weekend. Thanks guys!

Additional notes that we want to make sure to consider:
* ext/filter has known issues with URL parsing. The library either needs to work around that or we will need to do so ourselves.
* Researchers, please talk to Lukas Smith. He's a Symfony dev and very interested in what Drupal's up to these days. He's even posted in this issue already. :-)
* Remember cookies! (They're tasty.)

adub’s picture

Is it worth having a look at what's in lithium? I'm not exactly familiar but it is recent and the namespacing doesn't clash with Drupal.

dixon_’s picture

Our research on the Zend and Symfony HTTP request handling can be found in the WSCCI group, here: http://groups.drupal.org/node/167299

dcrocks’s picture

Just curiosity, not judging. Were zend and symfony looked at because of meeting some elimination process or because they are what the people who have devoted their time to this issue are familiar with? If the former I would find the the details educational.

catch’s picture

@drocks I believe it's the latter, although they have permissive licenses and Symfony in particular has been making an effort to create re-usable individual components. If you have another framework in mind then there is absolutely no harm to reviewing that and posting the results at this point.

skwashd’s picture

@dcrocks, the IRC meeting had nominated Symfony and ZF2 to be reviewed. When I caught up with @dixon_ on skype on Friday night (AEST), we quickly considered if there were any other frameworks available that we should evaluate. Neither of us have recent experience working with the nominated frameworks (I've worked with ZF1 in the past). We chose to stick with the original 2 frameworks.

The main reasons why we chose to stick with the 2 frameworks were primarily time and resources - we were doing this on our weekend. Secondly in our quick evaluation nothing else really stood out. I looked at Kohona quickly and their tree seemed very chaotic. Code Igniter was disregarded as the source of the Kohona fork. ZF1 was rejected as it was felt that there was no point in choosing something which won't be supported until Drupal 10 is released. CakePHP's components appeared to be heavily dependent on the rest of the framework. I can't recall which ones Dick looked at - I might even be taking credit for his work here.

We posted the report on g.d.o so the discussion could be held there and the decision making could end up here. That way someone doesn't need to read a full 200+ comment thread to understand the final decision.

Note: My comments about the other frameworks are based on sub 5 minute VCS repo reviews. They are quite possibly wrong. All of the projects listed above have good communities and work for certain use cases, but it was felt they wouldn't work for Drupal.

dcrocks’s picture

I did take a quick look also and most frameworks don't package their code as component libraries. Solar is the only other one I found but I didn't look close enough to see how 'de-coupled' their packages are. I would think that using the zend or symfony http libraries here doesn't commit drupal from using any of the rest of their framework, given they are considered heavyweights. Drupal is in the position to pick and choose. I did ask at the kohana site if it would be easy to package kohana code as 'libraries' and the answer was 'sure, easy'. I'm sure I would get the same answer from all the other frameworks as well. Clearly, it would be more work to evaluate the http implementations that don't provide clearly separated packages.

Crell’s picture

The other reason to consider Zend and Symfony in particular is because they are larger libraries; we don't have to use all of either one, certainly, but if we're going to need, say, an HTTP library, a YAML parser, and a Thingamabob class, it's just easier for all involved if they all come from the same 3rd party library. Zend and Symfony seemed to be the most likely to be able to "scale out" as we needed to pull in more 3rd party stuff, in part because they're built for exactly that usage. Drupal is a full stack framework. It doesn't make sense to try to pull in pieces of another full stack framework when there are component frameworks available. :-)

Gábor Hojtsy’s picture

My understanding from the g.d.o discussion at http://groups.drupal.org/node/167299 that Symphony seems to have won. Do we have an issue for adding that in? (I've been discussing the implications of that for replacing our browser language detection code in the language system with Symphony HTTP's solution with Yorirou that I wanted to record).

skwashd’s picture

@Gabor there isn't an issue for it yet. During Crell's London core conversation presentation he announced that Dries had agreed for Symfony to be imported into D8 core (at 2am). I think it is between 40 and 50mins into this video http://blip.tv/drupalcon/drupal8_full-5494012

Crell’s picture

Title:Locate a good HTTP library» Add Symfony2 HttpFoundation library to core
Assigned:Unassigned» Crell

Yep, Dries verbally approved Symfony at about 2 am so I'm holding him to it. :-) Because there's some legal ramifications here due to licensing to be sorted out, I'm assigning this to me. It's high on my priority list.

Crell’s picture

Status:Active» Needs review
StatusFileSize
new204.62 KB
PASSED: [[SimpleTest]]: [MySQL] 32,922 pass(es).
[ View ]

And here's a patch. It includes both the HttpFoundation libraries and the ClassLoader libraries, and wires up the ClassLoader for Symfony code. So far it doesn't actually do anything with them, but that's for later patches.

Crell’s picture

Issue summary:View changes

Added summary showing that we're going Symfony2.

heyrocker’s picture

sub

chx’s picture

yay! smallcore! (sorry. couldnt resist. I hope those lunatics will finally realise that quality and quantity are not related.)

Anyways. You sure that o_O code in bootstrap.inc doesnt need a test?

Crell’s picture

I'm not entirely sure how to write a test for that. I did verify it works manually, but that's hard to encapsulate. Suggestions welcome.

neclimdul’s picture

sub

Wim Leers’s picture

Subscribing.

After a quick first glance: how do you envision e.g. Symfony's UploadedFile going to be integrated with Drupal's File interface? Or is this too early to ask? :) (I.e. is it a question to be answered by follow-up issues?)

catch’s picture

We've been discussing the session handling which is also part of this in another issue (would link if not typing from phone). Over there the session storage interface looks fine and one or two of the implementations, and we already had an issue open to refactor that to use classes, but without much recent activity. However the actual session class and pdo implementation not so much ( session class has application-y 'flash' handling that conflicts with dsm(), raw pdo in core is a bit ouch). If we're using some parts of the session handling and not others, and the stuff we don't use directly conflicts or might be otherwise confusing there's a couple of choices (assuming it's not something small we could file an upstream patch for) - document somewhere the bits we don't use, or actually just delete those files from git at some point during the 8.x cycle.

Since these are bundled as one package, it makes sense to drop it in since we already have a use case to replace direct calls to request superglobals, but then open issues for different parts to see if it fills gaps, updates, or duplicates something we're not planning to change for each part.

chx’s picture

Cant you simply verify the class can be loaded? A totally empty test (can be unit test even, i think) which just instanties without a single assert is a test because if PHP fatals , the test catches that (try to instantiate a nonexisting class and you will see).

skwashd’s picture

I might have missed the issue with the test, but if you just want to test that the autoloader is loading classes, something like this should do the trick:

<?php
$class_name
= 'Symfony\\Component\\HttpFoundation\\Request';
$this->assertTrue(class_exists($class_name), t('Class !class_name exists', array('!class_name' => $class_name)));
?>
Crell’s picture

StatusFileSize
new206.02 KB
PASSED: [[SimpleTest]]: [MySQL] 32,901 pass(es).
[ View ]

Huh. So not only does #43 work, it works even in a UnitTest rather than WebTest. Sweet!

Revised patch attached.

There are other threads discussing other places we can leverage these tools, as catch noted. The focus here is just getting it in place as a wrapper for the HTTP super-globals, which we need for WSCCI. We can find other places to replace Drupal code with Symfony code in other threads. One step at a time. :-)

beejeebus’s picture

first, awesome, awesome patch - I love where this is taking us.

#43 is nearly right, but it can lead to false positives.

the unit tests currently run inside the parent drupal, which may already have loaded the class we are seeking to test autoload.

could we add a test class that a real site is unlikely to use, and check that we can successfully load that?

Crell’s picture

Hm. Not easily. That would require having a separate root for that test class that we register, at which point we're just duplicating the tests that Symfony already has (that we're not including as they're PHPUnit based). The whole point is to test that we can load code specifically in the /includes/* tree. So I guess for right now this test does work, but once core starts using that code it won't. :-) I don't know how to resolve that, but since the test works for now I'm satisfied for the time being.

chx’s picture

Status:Needs review» Reviewed & tested by the community

I am going to RTBC this while I voice severe reservations. Some things to consider:

We are moving from simple ($_GET) to something complicated -- right now it's just Symphony but we will layer WSCCI on top. It will be fairly impossible to debug where a certain value comes form esp if there is a bug because there will be so many layers. Again. Reminds me of form API and menu loaders. We wrap certain parts of the request up with these and in both cases we get power and flexibility back at the price of complexity.

Will this raise the bar for new developers even more?

We always said in various form API docs "peek at $_POST but dont store it" then blurred it further by providing form_state['input']. Is this what awaits us? A system for wrapping requests that although very flexible by nature of wrapping it's limiting and so we will still need to peek behind the curtains?

In the past we have piled everything, form API, menu API, field API (even multilingual, oh horror) without considering these. For Drupal 8 this must stop. We can't just add stuff forever without considering what are we doing. Yet, I do understand the necessity for this patch and so i say it's ready to be committed but -- we must be aware of what we are doing.

attiks’s picture

Sub

eigentor’s picture

Without understanding the technical implications a severe +1 to chx considerate post. D8 must be very careful in how much complexity it adds in order to not make Drupal Fort Knox just by increased complexity. I had already quite some of these experiences in D7.

podarok’s picture

subscribe

EugenMayer’s picture

@49: to be honest, D6 is already one of the most complicated frameworks i have ever used or touched. One of the main reasons is, that most of the stuff is implemented by the drupal community itself. Then later on, it did not get properly maintained as of lack of time ( thats O.k. - thats not a paid job ). There are several of those issues in the core ( not to mention some important contribs ).

Using external libs can be very helpful to refocus on the core part of drupal, making it a good CMS. Dont forget, drupal is not only a "php framework", it should be a good CMS. And due to all of this architectural work, there is not much time left for actually taking the tools and building a competitive CMS.

From:
- simpletest (PHPunit), to
- language server, the translation UI ( poodle )
- d.o git integration (gitorious or gitolite, github )

That list can be continued. Some of the alternative, ready to use solution seem to have issues themselfs, but the one mentioned do have a lot on their own now - which do get fixed in slow-motion. But still we contribute a lot of time creating those things, instead of taking part in other projects (contributing there) and actually using their drive, while concentrating on our own goals - drupal the CMS.

Sometimes it seems to me, those "we reinvent the wheel" projects happen, because the devs in Drupal need something "real" to do. So they chose to rewrite something from scratch. After some time, when the "core work is done" they dissapear, moving on to the "next real work", letting the written part more or less unmaintained.

Maybe some of those things are off-topic, but they are relaed to the question to use Symfony2 HttpFoundation or not. I would even suggest using more parts of Symfony2! What makes drupal DRUPAL is not the PHP framework. Someone who needs a PHP-framework will not chose Drupal, if he already know sit.

We should concentrate on what we are good in and what we are chosen for - thats the CMS, thats this application.

Wim Leers’s picture

I agree with catch: commit entire Symfony component(s) now, handle duplicate functionality later, so that we can continue working on this (this is also the reason for #39, which I just posted to ask what the plan was).

I also agree with chx's concerns. We can't continue adding layers.

#46:

at which point we're just duplicating the tests that Symfony already has (that we're not including as they're PHPUnit based)

So are we simply not going to include Symfony's tests, by the reasoning "if it works for Symfony, we don't need to include or run those unit tests, since we're not modifying Symfony"?

catch’s picture

Moshe has been working on shifting DrupalUnitTestCase and DrupalWebTestCase to extend PHPUnit instead of the current fork/rewrite of simpletest. That mostly works afaik. If we went ahead and completed that work + committing it, we could add the PHPUnit tests from Symfony too.

Jeff Veit’s picture

+1

beejeebus’s picture

re #46, wanted to make sure its clear i'm ok with this going in despite the test issues. the fact that our unit tests are fail right now is not for this issue to solve, i just wanted to make sure we were conscious of the issue.

lsmith77’s picture

if you want to support stuff like ESI for subrequests via a config option (like its possible in Symfony2) then you dont have a choice but to wrap the superglobals. Also if you care about testability, you again have no choice but to wrap the superglobals. Now of course this takes you a bit away from "standard PHP", which means that it will increase the learning curve a bit. Now anyone who isnt using subrequests can still introspect the superglobals to get the answers they would get from looking at the relevant properties in the Request instance.

Furthermore in Symfony2 we have a very sophisticated, yet easy to use solution with the profiler and the web toolbar. It gives you quick access to all this information.

Gábor Hojtsy’s picture

ygerasimov’s picture

subscribe

neclimdul’s picture

We seem to have gotten a bit off track discussing core developer dynamics, converting to PHPUnit and what happens after this.

Patch is good, we all want it, we have integration testing for the code we're technically "using" from it. Done, RTBC! :)

Next thread?

jackbravo’s picture

sub

CitizenKane’s picture

Looks solid to me, and I can think of at least half a dozen functions that it greatly simplifies, removes dirty hacks from, or removes outright. On top of that, getting something that is likely to be well maintained. Big +1 from me. Time to get to follow-up issues.

elvis2’s picture

subscribing

Crell’s picture

I think we're just waiting for Dries to get back from his cabin in the woods that has no modern amenities except for Twitter... ;-)

jackbravo’s picture

@Crell I thought that comment could be a little mean, until I saw that's actually the case =P haha.

http://twitter.com/#!/Dries/status/110059060275064833

Shellingfox’s picture

Subscribe

webchick’s picture

Issue tags:+WSCCI

Tagging.

catch’s picture

Status:Reviewed & tested by the community» Needs review

This needs better documentation in the issue summary before it can go in.

1. Which classes are included, what do they do.

2. Which Drupal functions/APIs/existing classes might they replace.

3. Individual issues to discuss how and whether to convert that.

chx’s picture

Status:Needs review» Postponed

There is a lot more needed than that. You know, I was coding relation the other day and needed to know whether a field hook replaces or augments the default hook. I had what I might call a "field_invoke" moment. Following the field_invoke call chain is rather tedious. Am I going to have a moment like that for every. single. piece. of. the. request? How is this is going to be used, what's the call chain from our API back $_REQUEST or similar?

There are a lot of completely invalid arguments in the issue already BTW. Let me a list a few: " a CLI app (Drush) could easily provide HTTP-esque information so that other code doesn't realize that it's not in an HTTP environment." -- blimey and I thought Drush, the command line test runner and other scripts are running mighty fine already, Thanks for telling me we need another 200kb in core to make them work.

ESI?? We were testing ESI at the end of 2009 (pre-D7 beta) for Examiner and if memory serves, it worked. (We decided against it because the only CDN we were able to find supporting it was Akamai.)

Also, #764558-84: Remove Trigger module from core says "Until we have a standard to determine what should and should not be in core, no more modules should be removed. " yes! and neither should new ones to be added. (Note the author of that comment)

chx’s picture

Status:Postponed» Needs work

OK i was to post facts not tirades. Let's stick to the facts! http://drupal.org/node/1178246#comment-4815646 is a rather sane list of what we want. The second bullet point on that list is all headers. Unless there is some deep voodoo I missed this is not happening because getallheaders / apache_request_headers() is not called at all anywhere.

To augment catch's post and maybe the biggest question here -- do we need to add this thing wholesale? is this the smallest unit we can add? Complete with response, uploaded file mime detection etc.?

Crell’s picture

Status:Needs work» Needs review

To the size of the package question, it's the package size that Symfony built. I don't think it's wise at this point to selectively pick out individual classes we do/don't want when this has already been sliced off of a larger project as a self-contained unit. We may or may not have made the same decision as to where to slice were we doing it, but that's the size that the Symfony devs decided was appropriate.

When installing Views, you don't say "well I'm not going to need row styles on this particular side, so I'll strip out those classes from Views first". That would be silly, and more often than not you find you need that functionality later anyway. We may yet end up using more of this library than we expect. We just don't know yet.

That said, I'm open to looking into whether there is code we want to prune from this library, or elsewhere... in a year or two when Drupal 8 is in alpha and we're pretty solid on which parts we will and will not be using. It's premature to do so now.

catch’s picture

Status:Needs review» Needs work

OK one more thing I'd like to see.

I opened up:

https://github.com/symfony/HttpFoundation/blob/master/Request.php

And saw right at the top

use Symfony\Component\HttpFoundation\SessionStorage\NativeSessionStorage;

Originally I thought this class was being used in there somewhere (which would be a concern since we're actively discussing the viability of the session handling in another issue), but instead it looks completely redundant.

So we can file an upstream issue (on github? If not where?), and test the response time. I imagine at least a couple of Symfony developers are keeping an eye on this issue so it might be faster than usual, but still a good test.

Another question - is this code pulled from a tag or a branch? Are we going to run bleeding edge during Drupal 8 or only ever commit tags or published releases?

catch’s picture

Status:Needs work» Needs review
Owen Barton’s picture

Status:Needs review» Postponed

One use case that comes to mind (not necessary to solve here, but I think would be worth considering) is alternate page generation approaches, such as Pressflow xRender, Drupal running on Appserver in PHP (which Symfony2 can already do), the drush runserver command (Drush 5.x has a built in dev. webserver) and other parallel or preloading page build/serve frameworks (this goes beyond ESIs). Obviously none of those need to be supported directly by core, but right now they need to do a quite a bit of global environment mangling, and some things (e.g. fetching all http headers) get hard to do reliably without patching core. It would be nice if Drupal could more easily run in these kind of non-standard environments by having the main environment better encapsulated and extensible (if needed).

An even more basic one is simply running Drupal with webservers other than Apache - right now core has quite a few "some webservers do X with $_SERVER['foo'], so we use mangle($_SERVER['bar']) instead". It would be interesting if some of this could be separated out more clearly, hopefully making it easier to see where information comes from and what "failover conditions" may trip (both good for security) and making it easier to adapt and run Drupal on yet-to-be-tested webservers.

Owen Barton’s picture

Status:Postponed» Needs review
chx’s picture

Apparently the all headers thingie is solved by PHP and all headers are in SERVER prefixed by HTTP_ . This is not documented anywhere cleanly on php.net but it seems to be the case and reading a few of the cleaner SAPI source files this seems to be the case (nsapi.c is the easiest to understand in this regard, essentially it does for (i=0; i < rc->rq->headers->hsize; i++)  spprintf(&value, 0, "HTTP_%s", entry->param->name); php_register_variable(value, entry->param->value, track_vars_array TSRMLS_CC);)

Edit: pending PHP team approval I will fix the docs. http://news.php.net/php.internals/55301

Crell’s picture

Owen: Those are all good examples of the use cases that we want to serve by properly encapsulating context, including the request information, and we can't really do without such encapsulation.

Crell’s picture

The $_SERVER variable (which per #75 includes header information) is one of the things that the request object wraps internally. (See the createFromGlobals() method of the Request class, which is the "default" initializer.) That data ends up parsed out into the HeaderBag class, an instance of which is held by the request object. A quick "Find" through the request class finds $this->headers used in all sorts of places, mostly inside utility methods that derive things like the port (could be either of 2 headers), the HTTP Method (could be in an override header, apparently), languages (requires some processing), etc.

I agree it's weird that PHP would be doing that and not well-document it, as something that old tends to be better documented. But, such is PHP. :-/

Crell’s picture

Also, to catch's other question: This patch uses the 2.0.1 tagged release of the library, as made available using GitHub's "download a tarball of this tag" feature. I figure we'll track the stable bugfix releases for the time being. I considered using a git submodule but that's a much larger process discussion I wanted to avoid having for right now.

lsmith77’s picture

Yes, if you guys find an issue, just open a ticket .. or better yet a PR on https://github.com/symfony/symfony to get the issue fixed upstream. Do note that upstream patches need to be submitted under the MIT (which we implicitly assume if code is put on any symfony repo on github.

Also expect the code even for 2.1 to be fairly stable for HttpFoundation (and actually most components with the exception of Config, Form, Security and Serialzier). The API is marked as stable and so there will be no BC breaks allowed at all in 2.1. In 2.0.x, which is the branch I think you guys are using, its really just bug fixes.

lsmith77’s picture

Is there a ticket for evaluating the HttpKernel component? It would make it even easier to integrate with Symfony2. More importantly it would provide Drupal8 with ESI support including a PHP user land ESI capable reverse proxy: https://github.com/symfony/HttpKernel

chx’s picture

https://github.com/symfony/symfony/pull/2141 removal of use is done. I recommend a reroll.

chx’s picture

Re #80 I filed #1275316: Replace Drupal kernel with Symfony please discuss (if there is anything to discuss which I doubt) over there.

Crell’s picture

That was quick. :-) Thanks, chx.

I don't know if we want to reroll just yet. Since that fix doesn't actually change anything in practice, I'm inclined to wait for the 2.0.2 release to include it. At least for now I don't think we should be "chasing head" in Symfony.

lsmith: I don't think we're ready to even think about the whole Symfony kernel at this point. One step at a time. :-)

catch’s picture

I'd be fine with waiting for 2.02. Once this is in we could use a docs page for contributing fixes back to HttpFoundation, where to pull releases from etc.

It'd still be good to have more issues like #335411: Switch to Symfony2-based session handling, I would need to read through the HttpFoundation code more to know which ones to open though.

dropcube’s picture

Subscribing

sdboyer’s picture

sub

chx’s picture

Out of respect for @sdboyer I moved his off topic comment to the linked HttpKernel issue. Any comments regarding that topic in here ongoing will simply be deleted. This is about HttpFoundation.

chx’s picture

Issue summary:View changes

Correct reference to Symfony2's license.

Crell’s picture

Per catch's request, I have created/linked to/both a number of issues that I think are good follow-ups that want this patch. That's just those I came up with going through the Request object. It looks like the Response object has some other places we could clean things up, especially with regard to HTTP headers and caching.

I didn't create an issue for it yet, since it's more of a meta issue, but if we want to reduce our usage of DrupalWebTestCase (slow as molasses) in favor of DrupalUnitTestCase (lickety-split), we need to be able to separate out the request information into a mockable form. Having the Http information encapsulated into an object that we can fake-out easily (see Request::create()) is a critical step toward having fast unit tests rather than slow integration tests.

webchick’s picture

Status:Needs review» Reviewed & tested by the community

Great. Looks like catch's concerns are resolved. Back to RTBC.

chx’s picture

Status:Reviewed & tested by the community» Needs review

Not quite, we are waiting for 2.0.2

Crell’s picture

Status:Needs review» Reviewed & tested by the community

chx: Er. No we're not. 2.0.2 has no functional difference; the "use" fix you filed doesn't fix any bugs aside from code cleanliness.

A follow up patch to move to 2.0.2, 2.0.3, etc. when those are released should be quite easy for anyone to write.

q0rban’s picture

Subscribe

catch’s picture

Status:Reviewed & tested by the community» Needs review

I discussed this issue with Crell in irc and became even less keen on committing it without an implementation. The follow up issues aren't concrete enough for me, they're more "things we can do when all this is in place" not "how we're going to convert current code in core to use the HttpFoundation library", which is what I was after but appears to be a difficult answer to actually get.

It's my understanding this code will not actually be executed until the context object is in, and the context object itself is still being actively worked on in a sandbox (and there is no http implementation of that yet anywhere, apparently we're waiting for this to get into core before even trying that which seems backwards).

I would be very happy to see the autoloader go in asap, then we can start work on using that for some low level core classes.

I don't see any reason to commit the library until there is at least some reviewable Drupal code that uses it somewhere, and that's not the case at the moment.

Since it looks like webchick RTBC'd this somewhat on my behalf, and Crell can't RTBC his own patches (although I believe he's going to discuss this issue with Dries), I'm marking this back to CNR - it needs someone to actuall RTBC it on its own merits IMO, but without anyone actually trying to convert functions like request_path() (which is not pretty) or current_path() and the rest to it, it is starting to just feel speculative to me at this point.

That's not to underestimate the work that dixon_ and skwashd did on researching this, or that Symfony isn't the best framework to be pulling code from in terms of how they've organised their code for that purpose, but that in itself does not feel enough to justify committing this now rather than when there's a more solid plan for implementing its use - it's not exactly a hard patch to re-roll, precisely because there's no refactoring of existing core code in it.

chx’s picture

To quote myself above, "How is this is going to be used, what's the call chain from our API back $_REQUEST or similar?" so yeah, having implementation would be lovely.

neclimdul’s picture

Status:Needs review» Needs work

Crap, I looked at this last night and realized the require_once calls added to bootstrap.inc don't meet our coding standards. require_once('foo.inc'); vs require_once 'foo.inc';

Interestingly, Symfony's do. Not that that matters since we shouldn't hold external projects to our coding standards.

neclimdul’s picture

@chx if we don't commit this without implementation then we need to pull a set of commits otherwise we're overloading the commits IMHO.

catch’s picture

@neclimdul it doesn't necessarily need to be committed the exact moment as the implementation, but afaik there is not yet Drupal code in existence anywhere that uses this library (or none related to core at least), and it's this that's makes me not willing to RTBC it. Having a core patch in progress stacked on it would be plenty - doesn' t need to be finished, just to see what we'e able to remove and how it's looking. With DrupalCacheArray (which was not much more custom code than added here) I had one implementation with the patch then several lined up and partially or mostly mapped out to ensure the base class was generic enough to actually be re-used for example.

#1262014: Move request path handling into context relates to this - we don't use any pure HTTP headers directly, so where's the interaction between context, HttpFoundation and the code that sits in between those?

Or to put it another way. This is within the first 100 or so lines of code executed in a Drupal bootstrap:

<?php
/**
 * Initialize PHP environment.
 */
function drupal_environment_initialize() {
  if (!isset(
$_SERVER['HTTP_REFERER'])) {
   
$_SERVER['HTTP_REFERER'] = '';
  }
  if (!isset(
$_SERVER['SERVER_PROTOCOL']) || ($_SERVER['SERVER_PROTOCOL'] != 'HTTP/1.0' && $_SERVER['SERVER_PROTOCOL'] != 'HTTP/1.1')) {
   
$_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.0';
  }

  if (isset(
$_SERVER['HTTP_HOST'])) {
   
// As HTTP_HOST is user input, ensure it only contains characters allowed
    // in hostnames. See RFC 952 (and RFC 2181).
    // $_SERVER['HTTP_HOST'] is lowercased here per specifications.
   
$_SERVER['HTTP_HOST'] = strtolower($_SERVER['HTTP_HOST']);
    if (!
drupal_valid_http_host($_SERVER['HTTP_HOST'])) {
     
// HTTP_HOST is invalid, e.g. if containing slashes it may be an attack.
     
header($_SERVER['SERVER_PROTOCOL'] . ' 400 Bad Request');
      exit;
    }
  }
  else {
   
// Some pre-HTTP/1.1 clients will not send a Host header. Ensure the key is
    // defined for E_ALL compliance.
   
$_SERVER['HTTP_HOST'] = '';
  }

 
// When clean URLs are enabled, emulate ?q=foo/bar using REQUEST_URI. It is
  // not possible to append the query string using mod_rewrite without the B
  // flag (this was added in Apache 2.2.8), because mod_rewrite unescapes the
  // path before passing it on to PHP. This is a problem when the path contains
  // e.g. "&" or "%" that have special meanings in URLs and must be encoded.
 
$_GET['q'] = request_path();

 
// [snip]
}

function
request_path() {
  static
$path;

  if (isset(
$path)) {
    return
$path;
  }

  if (isset(
$_GET['q'])) {
   
// This is a request with a ?q=foo/bar query string. $_GET['q'] is
    // overwritten in drupal_path_initialize(), but request_path() is called
    // very early in the bootstrap process, so the original value is saved in
    // $path and returned in later calls.
   
$path = $_GET['q'];
  }
  elseif (isset(
$_SERVER['REQUEST_URI'])) {
   
// This request is either a clean URL, or 'index.php', or nonsense.
    // Extract the path from REQUEST_URI.
   
$request_path = strtok($_SERVER['REQUEST_URI'], '?');
   
$base_path_len = strlen(rtrim(dirname($_SERVER['SCRIPT_NAME']), '\/'));
   
// Unescape and strip $base_path prefix, leaving q without a leading slash.
   
$path = substr(urldecode($request_path), $base_path_len + 1);
   
// If the path equals the script filename, either because 'index.php' was
    // explicitly provided in the URL, or because the server added it to
    // $_SERVER['REQUEST_URI'] even when it wasn't provided in the URL (some
    // versions of Microsoft IIS do this), the front page should be served.
   
if ($path == basename($_SERVER['PHP_SELF'])) {
     
$path = '';
    }
  }
  else {
   
// This is the front page.
   
$path = '';
  }

 
// Under certain conditions Apache's RewriteRule directive prepends the value
  // assigned to $_GET['q'] with a slash. Moreover we can always have a trailing
  // slash in place, hence we need to normalize $_GET['q'].
 
$path = trim($path, '/');

  return
$path;
}
?>

What can we drop from here? What has to stay? If it stays where does it live? These feel like very open questions at the moment.

sdboyer’s picture

@catch and howzabout the session handling stuff, which we've already established that we want to use over in #335411: Switch to Symfony2-based session handling? It's another use case for the libraries, and I've marked it postponed pending the commit of this issue.

Having the libraries present, even if refactoring the code takes a little while, seems fine to me, as we're still early in the process. Now, if we had a branch somewhere that pulled in HttpFoundation that we could all merge from while working on related changes, then it would be easier...ah wait, that's my responsibility :) But really, even if there are open questions, it'd be easier to resolve those if the autoloader is in and fixed, and a base autoload point is established for the sf2 libraries.

catch’s picture

@sdboyer: the session issue is very early too, and we only want to use about 1/2 of the session code that comes with this, as well as needing to document the bits we won't use and why to avoid confusing people. That issue is part of why I'd like to see things a bit further along with that kind of discussion for the main stuff this is lined up for ($_GET etc.).

I agree the autoloader should go in asap.

Owen Barton’s picture

I agree the session code is a complex area to start in - in my view it would be preferable to start with a more foundational area, such as the bootstrap code above.

lsmith77’s picture

@chx: hmm I don't see a HttpKernel ticket linked here .. am I overlooking something?

chx’s picture

chx’s picture

Yes and even if we do a conversion, I would very very much like to see the answer to #96 above: what is the code / debug path to the wrapper request element? That question is now ignored several times but I will simply not stop until that one is answered. So, say I want to the Drupal-8-equivavlent-of-$_GET['foo'] and so that function calls X which in turns call Y which finds GET loaded by the constructor... etc.

catch’s picture

As well as request_path() and current_path() etc. there's also all of these which we may or may not be able to get rid of:

http://api.drupal.org/api/drupal/includes--common.inc/group/http_handling/7

neclimdul’s picture

@chx & @catch Those are context questions which is the tightly connected next step. From Crells Drupalcon presentation, the rough idea is:

<?php
class Foo {
  protected
$context;
  public function
__construct($context) {
   
$this->context = $context;
  }
  public function
behave() {
   
$node = $this->context['node'];
   
$var = $this->context['http:get:var'];
   
// ...
 
}
}

function
behave() {
 
$context = drupal_get_context();
 
$language = $context['language'];
 
$foo = $context['http:header:foo'];
}
?>

Implementation is still being hammered out. Details from last weeks meeting: http://groups.drupal.org/node/175544

catch’s picture

That's not what I'm asking for though, those examples have been posted in plenty of places, but that's no different from saying:

<?php
$this
->context['organic_group'];
?>

It doesn't tell me how the context actually gets provided to there.

There is going to be code between:

<?php
$this
->context['http:get:var'];
?>

and the HTTPFoundation library.

It's this that is the interesting bit, and is still entirely in discussion stage in issues like #1262014: Move request path handling into context.

neclimdul’s picture

I'll repeat the link to http://groups.drupal.org/node/175544 then which is the latest information on them figuring out how that implementation works. That said, I don't actually know, much other then they seem to be using ArrayAccess to map it and that implementation is something Crell is actively working on using this code. Hence the request to add it based on all the design discussions on gdo and the ongoing work.

RobLoach’s picture

I'm really liking the direction this is going. Adding the NIH tag.

catch’s picture

@neclimdul: right, I attended that meeting (although it started very late to me so was only half following), and that's partly what makes me hesitant about this patch going in - there is a lot to be sorted out still for everything that will interface with this.

chx’s picture

@neclimdul how Drupal will work on top of this is one half but the other half is SHF itself. 'Cos if we are talking to debugging there will be a point where the Drupal API calls Symfony and then ... how does debugging proceed?

webchick’s picture

Assigned:Crell» Dries

Adding to Dries's queue temporarily to chime in on the "how to get this into core" question.

chx’s picture

Assigned:Dries» Crell

I have studied http://api.symfony.com/2.0/Symfony/Component/HttpFoundation/Request.html and the code and got advice from lsmith as well. So, the answer is, there are two factory methods, called createFromGlobals and create. These use the new PHP 5.3 late static binding feature to call the constructor of whatever class they ended up with ie. if you have class foo extends Symfony\Component\HttpFoundation\Request then they return an instance of class foo. Usually you use createFromGlobals, the other is when you want to mock stuff.

The constructor, in turn, does nothing else but calls initialize (which is not called from anywhere else in HttpFoundation).

This will, in turn store a number or properties in the request object using various bags. We have classes called ParameterBag, FileBag, ServerBag and HeaderBag. They are all descendants of ParameterBag, it contains a few mehods, the most important is get. It is rather similar to variable_get by taking a key and a default but it also supports drupal_array_get_nested_value type recursion. There is also a set which does not support such recurseion.

While the Request class itself contains a get method, it's considered bad practice. So here's the code we will use:

<?php
// Somewhere early
$request = Symfony\Component\HttpFoundation\Request\createFromGlobals();
// createFromGlobals passed $_GET into the $request constructor as the value of
// $query which is stored into the query property wrapped in a ParameterBag.
print $request->query->get('q', 'the_default_for_get_q');
?>

There's a method which demonstrates how the default can be very useful:

<?php
public function getScriptName() {
        return
$this->server->get('SCRIPT_NAME', $this->server->get('ORIG_SCRIPT_NAME', ''));
    }
?>

The code is kind of simple but the mapping is kind of confusing: POST ends up in $request->request, GET ends up in $request->query, $_COOKIE ends up $request->cookies, $_FILES go into $request->files and $_SERVER ends up in $request->server. Also the headers are extracted into $request->headers. There is a $request->attributes bag which is Symfony specific, it's not relevant to Drupal and so it stays empty.

I think this is easy to follow, yes.

chx’s picture

Assigned:Crell» Dries

Sorry, crosspost.

Owen Barton’s picture

Essentially, I think we need to aim for replacing index.php with something like the first snippet chx posted (perhaps via bootstrap 0 or a wrapper function).The point is that something could create a request from something other than globals, and get all the output (including cookies etc) without messing with $_ANYTHING.

One thing to bear in mind though, is that Drush (and probably other consumers) do need a way to bootstrap Drupal without ever requesting a page. Would we bootstrap during the $request initialize? Either way, it may not be feasible to make $request optional all the way through the bootstrap (not that I would complain!). Worst case we can mock a request and suppress the page callback somehow (basically what we do now), but finding a more elegant way of doing this would be nice.

neclimdul’s picture

@owen barton totally. a lot of wscci is about being able to get drupal to run in interesting ways. Mostly as a rest server for different content types but I don't see why that shouldn't help drush. The way I see it, worse case something built off this would make the current drush code more straightforward.

sdboyer’s picture

@Owen Barton yeah, i've been trying to chime in on the "don't forget alternate entry points" thing for a while. I'm not sure of the best answer here, but I do agree that one where drush has to pretend it's an http request is...really wrong.

I will say that one of the ideas I've liked from sf2 since I did the very initial research was their multiple "front controller" (aka, various versions of an index.php) approach. I don't know what the specifics ought to look like, but I do think we would be VERY well-served if our goal was to organize the bootstrap process deliberately such that purpose-built front controllers become pretty easy. (Note that the front controller and Request class are decoupled).

Crell’s picture

chx: Hm. I thought you were talking about a request trace for the full context-request line, which is what I cannot provide at the moment. (I was also dealing with 8 clients last week and am at an all company retreat this week, so I've barely had time to breathe, much less trace code.) Yes, that is essentially the code I see happening inside the http context handlers.

Owen: The current code in WSCCI has a dummy request object that is passed into the context constructor. However, the Services team in London had a very good point; that's a stupid way of doing it. :-) Instead, request information should be provided via yet-another-ordinary-context-handler, that happens to use the HttpFoundation library internally. I think they're dead-right on this one, and it eliminates the "fake HTTP" problem.

So for a normal HTTP request, Drupal first boots up a context object and associates the default handler configuration (stored in the CMI configuration system), one of which is the http handler. If that is never used, though, it's never used. The basic outline becomes:

<?php
$configuration_system
= start_config_system(); // Just gives us access to config objects.
$context = new DrupalContext(new Configuration('context_handlers'));
figure_out_response($context); // This replaces menu_execute_active_handler(), and could return any number of things.
?>

Drush would never use the http context information, so it can safely ignore that. However, it could very easily provide a cli-wrapping context handler instead (wrapping argv and argc and whatever else), and do all sorts of fun and exciting things with that. The same "central access point with easy overriding and lazy-loading" benefits apply. Consider having Drush commands that can sub-call other Drush commands with completely different command line arguments without having to muck about with globals, even indirectly. It's exactly the same process as a block that has a sub-block (mini-panels style) where it overrides information. And HTTP information never needs to enter the picture.

(And, should a given object in Drupal that's called from Drush expect a node that would normally come from the HTTP GET path, you simply set an overridden node on the context object before it gets there. That code is none-the-wiser, and you can now call that code from Drush, cleanly, even though it wasn't written for it in the first place.)

Owen Barton’s picture

Crell - that makes a lot of sense to me - this arrangement should work great for Drush.

I guess the only question I have is how the http request could be altered by the original caller in this workflow (before additional context is deduced from it)? This doesn't really apply to Drush (although runserver could use it), but could apply to callers operating in unusual environments (see #73). Would they extend/wrap the http context handler?

Crell’s picture

Owen: That's the stacking/mocking/layering functionality already in the sandbox, and also well off-topic for this thread. Lets' get back on track here. I contacted Dries a while ago about the process here (as project lead it's his call), so I'm waiting on his response.

lsmith77’s picture

FYI 2.0.2 (and 2.0.3) have been released.

ygerasimov’s picture

I have pushed branch "symfony" to Services module (http://drupalcode.org/project/services.git/shortlog/refs/heads/symfony). Now it uses Symfony HTTPFoundation component to get all the http globals to context. I think this is first example of practical use of the Symfony component in drupal.

This code has been developed during codesprint during DrupalCon London.

Cyberwolf’s picture

Subscribing.

Crell’s picture

Status:Needs work» Needs review
StatusFileSize
new208.32 KB
PASSED: [[SimpleTest]]: [MySQL] 33,301 pass(es).
[ View ]

And here's a revised patch that includes the just-released 2.0.4 versions of ClassLoader and HttpFoundation. There's also a few tweaks to the autoload configuration so that it doesn't use APC automatically, since the caching may cause issues during development. It's now a variable you can switch on for production. Heyrocker said that was probably the best approach for now until CMI figures out how to handle pre-database-bootstrap configuration.

podarok’s picture

#123 Looks like resonable before CMI starts

Crell’s picture

Can someone either mark this RTBC or give some critique of the code? The autoload wiring in bootstrap? Or putting code in /lib instead of /includes? Anything? Beuler?

RobLoach’s picture

Status:Needs review» Needs work
+
+  // Register classes with namespaces.
+  $loader->registerNamespaces(array(
+    // All Symfony-borrowed code lives in /includes/Symfony.
+    'Symfony' => DRUPAL_ROOT . '/includes',
+  ));
+

Although I haven't looked into Symphony as much as I would've liked to, is there any way to register additional classes here? Any other libraries we could use with the auto-loader?

+++ b/includes/bootstrap.incundefined
@@ -2225,6 +2225,38 @@ function _drupal_bootstrap_configuration() {
+  // @todo Switch to a cleaner way to switch autoloaders than variable_get().
+  switch (variable_get('autoloader_mode', 'default')) {

Should APC support be moved to http://drupal.org/project/apc ? Since this is so early in the bootstrap, it might not be an option. What did you have in mind for a cleaner method of switching autoloaders?

-14 days to next Drupal core point release.

RobLoach’s picture

Whoops, didn't mean to change status.

Crell’s picture

Status:Needs work» Needs review

The classloader doesn't need an entry per-class. You just need to register the root of a given namespace, and it will work for anything in that namespace. So as long as anything in the /Symfony namespace goes in the right PSR-0-defined location inside includes/Symfony, the class loader will find it.

In the WSCCI sandbox we also have an includes/Drupal namespace registered that works for any core-class in the Drupal namespace. That's the main benefit of using the PSR-0 standard. It makes the class loader configuration really simple.

For APC, it's low-level enough that I don't see how a module could do it. The class loader is not using a full-on Drupal cache system (nor could it), so routing it through the cache system (which APC module ties into) wouldn't be useful. The better way of doing it would likely be "whatever the CMI folks come up with when they figure this out". :-) Heyrocker said to go ahead with this approach for the time being and when CMI figures out how to handle super-early configuration we can move it over to that, along with a half-dozen other things.

Crell’s picture

For those playing our home game, we're working on the integration of this library into the Context API here: #1279942: Create Http context handler

I expect that to be committed to core as part of the initial Context API commit.

Crell’s picture

StatusFileSize
new208.34 KB
PASSED: [[SimpleTest]]: [MySQL] 33,631 pass(es).
[ View ]

Minor comment improvement from deviantintegral.

RobLoach’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+symfony

Even having the class loader here will benefit us greatly. I'm setting this to RTBC. Is WSCCI the place for follow up Symfony2-related issues?

catch’s picture

It's great to see the activity in #1279942: Create Http context handler that removes all my previous objections to committing this as is. I've not reviewed the whole patch but I did look at the new APC hunk and disabling by default looks sensible to me. We have #1241190: Possible approaches to bootstrap/front loading of classes and the main PSR-0 issue open to refine that.

This has been in Dries' queue for a while so I'm leaving it there though given he hasn't chimed in here yet.

Crell’s picture

Rob Loach: Using the HttpFoundation library for WSCCI/Context purposes, yes, that should be done in the WSCCI sandbox at present. There's lots of other things we can do with the autoloader, Symfony-related and not, but that's for other issues, including #1241190: Possible approaches to bootstrap/front loading of classes as catch mentioned.

catch: Yay! I spoke to Dries at BADCamp this weekend and he said he was inclined to commit this as is, pending talking to you to see if you had any objections. It looks like no, so we're just waiting on Dries to get back around to it to push the button.

There's also not much else to review than that small block in bootstrap. :-) The rest of the patch is a new line in the README to mention Symfony, and then the actual Symfony code itself.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.x! Tadada. Sorry for the delay.

catch’s picture

Title:Add Symfony2 HttpFoundation library to core» Changelog/change notification for Add Symfony2 HttpFoundation library to core
Category:feature» task
Priority:Major» Critical
Status:Fixed» Active

That means we need a CHANGELOG.txt entry and a change notification :)

Crell’s picture

Status:Active» Needs review
StatusFileSize
new546 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,651 pass(es).
[ View ]

Changelog attached. I structured it so that if we add more Symfony2 components later we can just add to the list.

Change notices created. I will update docs and post some follow-up issues shortly.

xjm’s picture

Status:Needs review» Reviewed & tested by the community

CHANGELOG addition looks fine and summarizes the key additions.

Status:Reviewed & tested by the community» Needs work
Issue tags:-WSCCI, -Proudly Found Elsewhere, -symfony

The last submitted patch, 1178246-symfony2-changelog.patch, failed testing.

webchick’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI, +Proudly Found Elsewhere, +symfony

#136: 1178246-symfony2-changelog.patch queued for re-testing.

A changelog.txt entry causing 1 fail? That's one epic changelog.txt entry. ;)

RobLoach’s picture

Status:Needs review» Reviewed & tested by the community

We definitely need tests for CHANGLELOG.txt ;-).

catch’s picture

Title:Changelog/change notification for Add Symfony2 HttpFoundation library to core» Add Symfony2 HttpFoundation library to core
Assigned:Dries» Unassigned
Category:task» feature
Priority:Critical» Major
Status:Reviewed & tested by the community» Fixed

Thanks! Committed and pushed.

aem34’s picture

i hope those components will get regular updates
following the releases of drupal 8, 8.2, 8.4, ... :)
because on the symfony side, they will get security/debugging releases too !

wilmoore’s picture

is there any way to register additional classes here? Any other libraries we could use with the auto-loader?

You just need to add another prefix to the array. That's it. You can add regular Company_Package_Subpackage prefixes, or you can add Company\Package\... namespaces.

Here is an example pulling in multiple paths/libraries:
https://gist.github.com/937433#file_autoload.php.dist

It is also a pretty good example of how to bring in libraries that have their own autoloader that you want to defer to.

yched’s picture

Status:Fixed» Needs work

In the patch that got in :

+++ b/includes/bootstrap.inc
@@ -2228,6 +2228,38 @@ function _drupal_bootstrap_configuration() {
+    case 'dev':
+    case 'default':
+    default:
+      $loader = new \Symfony\Component\ClassLoader\UniversalClassLoader();
+      break;

What is that default: label ? I don't see where this would be needed ?

-26 days to next Drupal core point release.

neclimdul’s picture

ysched its a switch so default is the fall through. I think crell probably wrote it that way as "here are the expected values and if we get something weird default to this as well"

Crell’s picture

Status:Needs work» Fixed

Correct. Better to fall back to the auotloader we know works than to explode weirdly. (And that code is actually from pounard in a separate thread, to be fair.)

We probably want to clean up that section later once we have a better "dev mode" concept, but that can be done in another thread. This one is already long enough. :-)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

Add list of follow-up issues.