RFC Refactor HTTP layer to use Symfony foundation

[maxime-rainville]

Processing HTTP requests and responses is a big pain in the backside. It's got all sort of picky edge cases and if you screw it up, you're likely to introduce vulnerabilities.

Symfony has already figured out all of this stuff and can do it way better than we can. Doing this ourselves provides zero value and is all downside.

If practical, we should leverage symfony/http-foundation in CMS5. If there's not enough time, let's look at it for CMS6.

Possible approaches

• Approach A - Nuke HTTPRequest, HTTPResponse and related classes altogether. Use the equivalent Symfony classes directly.
• Approach B - Refactor HTTPRequest, HTTPResponse and related classes to extend the equivalent Symfony classes. Preserve any Silverstripe specific logic.
  • Approach B1 - Delete all existing method that have Symfony equivalents.
  • Approach B2 - Convert all existing method that have Symfony equivalents to be aliases (try to nudge people to use the Symfony method instead).
  • Approach B3 - Update all method parameters that expect a HTTPResponse or HTTPRequest to expect the Symfony class instead.
• Approach C - Wrap around HTTPRequest and HTTPResponse around a private instance of their equivalent Symfony object.

Side Quest

Somewhat related, once upon a time we thought about implementing PSR7. A lot of the concerns there are still relevant today. The conclusion was use a "bridging module" which we never got around to creating ... Robbie had a PoC.

If we go with Symfony, people who need PSR7 can potentially piggyback of the Symfony bridging approach. We might be foreclosing direct support for for PSR7 however.

Equivalence

• Silverstripe HTTPRequest => Symfony Request
• Silverstripe HTTPResponse => Symfony Response
• Silverstripe Cookie and CookieJar => Symfony Cookie
• Silverstripe HTTPRequestBuilder => Request::createFromGlobals()
• HTTPResponse_Exception => ???
• HTTPStreamResponse => StreamedResponse
• NullHTTPRequest => ???
• Session => Maybe Session

@silverstripe/core-team

[maxime-rainville]

I think we should aim towards Approach A long term, but don't have enough lead time to CMS5 to implement it.

Approach B2 seems realistic for CMS5, presuming we reach a consensus relatively quickly.

Approach B3 could be useful to nudge people toward expecting Symfony classes ... although it might force anyone who is expecting an HTTPRequest/HTTPResponse to rewrite there parameters. We could try to do some class aliasing to make this less painful.

I put Approach C there because we briefly considered doing something like that in CMS4. But I don't think we should do this.

On the PSR7 question, I would rather switch to Symfony then be PSR7 compliant. If there's a way to use the Symfony bridging approach to get there, that would be nice and worth documenting.

[emteknetnz]

Since the guiding ideal here is 'more symfony better', A seems like the ideal outcome, while B is few different flavors of a less strict A'. Does the choice between A and B just come down to how much work is involved?

There's a huge number of references in core to HTTPRequest and HTTPResponse, a quick string search yielded over 1,000 matches. Seems impossible to estimate the amount of work that would be required here so I'm not sure how we can make a choice between A or one of the B flavors.

Given the large amount of unknown here, possibly it seems too early for an RFC at this point in time and more of a need for some exploration such as 'SPIKE - Make HTTPRequest a subclass of symfony Request and see what happens."

[maxime-rainville]

Approach A implies removing HTTPRequest and HTTPResponse altogether. That would mean a lot of refactoring.

If we only do B2, in theory you could refactor HTTPRequest and HTTPResponse and everything else still works.

[wilr]

+1 for relying on more Symfony and ditching bespoke work generally. Approach A would not be an attractive proposition for developers and might push people away from upgrades if serious refactoring is needed (sample typical project with modules I opened had 2000+ references to both across 550 files).

I haven't compared the two public apis to see if they are fundamentally similar but I'm picking it's unlikely and I doubt it could be fully automated (apart from the namespace change). That sounds like a lot of burden on the community and developers in general for what ultimately is a marginal improvement for them day to day.

B2 gets my +1 as the best progression path since changing HTTPRequest / HTTPResponse to extend Symfony classes should be fairly trivial to implement initially without causing widespread panic. Throw deprecation warnings in all the old methods for 5, remove for future release.. (i.e getIP would be replaced with Symfony's getClientIp()

[maxime-rainville]

Had a go at implementing a POC to see how easy it would be to switch to Symfony for the HTTP Request.

At first glance, it seems to be pretty easy. Got something that looks like a functional CMS with those PRs:

• API Work with Symfony Request #10473
• POC Use Symfony to manage request silverstripe-asset-admin#1289
• POC Work with Symfony Request silverstripe-assets#510
• POC Work with Symfony Request silverstripe-cms#2772
• POC Work with Symfony Request silverstripe-errorpage#73
• POC Work with Symfony Request silverstripe-mimevalidator#58

Also you need to make a few changes to your index.php file.

<?php

use SilverStripe\Control\HTTPApplication;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\Session;
use SilverStripe\Core\CoreKernel;

// Find autoload.php
if (file_exists(__DIR__ . '/../vendor/autoload.php')) {
    require __DIR__ . '/../vendor/autoload.php';
} elseif (file_exists(__DIR__ . '/vendor/autoload.php')) {
    require __DIR__ . '/vendor/autoload.php';
} else {
    header('HTTP/1.1 500 Internal Server Error');
    echo "autoload.php not found";
    exit(1);
}

// Build request and detect flush
$request = HTTPRequest::createFromGlobals();

// Default application
$kernel = new CoreKernel(BASE_PATH);
$app = new HTTPApplication($kernel);

$response = $app->handle($request);
$response->output();

It took me about 6 hours over two days. I only did the Request part for now as I assumed this would be tho more difficult part.

My general feeling so far is that this looks pretty promising and that I managed to get pretty far given the little amount of time I spent on it.

General thoughts

• Symfony has its own factory approach for instantiating object with Factories and with some YML configuration. Didn't make any effort to getting that working.
• The trusted proxy is build directly into Symfony request. We would probably need to remove the trusted middleware proxy and tell people to interact directly with the HTTP Request.
• Most of the logic left in HTTPRequest is around route matching. We might be able to extract that to custom RequestMatcherInterface

Things I didn't try

• CLI is not working right now, but that should be trivial to fix.
• Unit test
• Behat test
• Session manager

Basically, I managed to access a few pages, logged into the CMS, edit a page, view some draft content, upload some files.

Session management

• Out of the box, Symfony doesn't use native PHP session functionality. There's a way to get that working if we really want to, hovever I'm not sure what the pros and cons are.
• The Session and Request abstractions in Symfony and in Silverstripe are intertwined. I don't think we could get away with only the Symfony Request without using the Symfony Session as well.
• Our Session object expects to have a reference to the Request. The Symfony Session doesn't which seems cleaner to me but also will make it difficult to cleanly convert some of our current custom logic.
• There's a lot of cool packages to interact with Symfony sessions. That could open some cool features. e.g.: Don't need to implement our own module to share session across DB ... can just use the Symfony one.
• Our Session has a nestedValue concept which I think is meant to allow you to sort-of-namepace your session data. I remove the concept from my implementation, which didn't seem to break anything. But would need to double check that.
• A somewhat annoying API difference is that our clear method is meant to remove a single entry while the Symfony equivalent removes every entry. That could potentially trip a lot of people upgrading.

File uploads

• The Symfony Request object has a dedicated API for retrieving Uploaded file data instead of relying on the $_POST array. It also as an UploadedFile class.
• I ended swapping the array for UploadedFile in most of our upload file logic. I can't help but think that I was double handling a lot of things that Symfony was already doing. There's probably a lot of logic that could be retired in this space.
• Alternatively, if we didn't want to take that risk we could probably just recreate the matching array data from an UploadedFile object and pass it to our regular current Upload logic.

[GuySartorelli]

@maxime-rainville Which specific approach above does your POC follow? It seems to be one of B1 or B2... or some combination of the two?

[maxime-rainville]

Mostly B2. I deleted some method that had a direct equivalent with the same name on the Symfony Object.

I also deleted some awkward methods that were left over from a time when we let you specify the HTTP verb via a custom header.

[lekoala]

i hope approach C will be reconsidered, as it looks much safer to me and will not break anything. How are the requests handled under the hood should be an implementation detail.
Just saying this because each time I have extended something instead of wrapping it, I've always ended up having issues I didn't foresee :-)