RFC - Make Cake\Network\Request implement PSR7 ServerRequestInterface

[markstory]

This is a (multiple allowed):

• [ ] bug
• [x] enhancement
• [x] feature-discussion (RFC)

In 3.3, we added a PSR7 'bridge' component. For 3.4, I'm proposing that Cake\Network\Request implement the PSR7 ServerRequestInterface. This provides a few benefits:

• Consistency across the framework. Both middleware and controllers/components will have access to the PSR7 interface methods.
• More immutable objects. Immutable objects have some nice benefits and having more of them feels like a good thing.
• Less duplication. Currently our request object, and the PSR7 transformers duplicate a significant amount of code with diactoros. By adopting the PSR7 features in Cake\Network\Request we would be able to remove a large chunk of duplicate code.

I also feel that the PSR7 interfaces help promote consistency across different PHP frameworks, which is beneficial to new users who have used other PHP frameworks. We of course will continue to offer additional 'sugar' methods that make using the PSR7 objects more ergonomic. For example setting cache headers will continue to have higher level methods, as the PSR7 interfaces are too low-level.

Dealing with Mixed Mode Operations in Requests

Currently our request objects are mutable, while PSR7 requests are immutable. Bridging this gap will seamlessly is not be feasible. Instead, I'm proposing we take the same approach we took with Cake\Http\Client\Response, and continue to honor in-place mutation for the existing methods, while offering new immutable interfaces.

Combined Getter/Setters

The methods that act as both a setter & getter would be deprecated. New getters would be added that follow the PSR7 conventions more closely. For example header() and getHeaderLine(). We would introduce new methods for the setter mode in the conventional PSR7 style:

// The following would become deprecated
$request->data('Post.title', 'Some title');
$request->param('controller', 'Articles');

New setters would be introduced:

$request->getData('Post.title');
$request = $request->withData('Post.title', 'Some title');

$request->getParam('controller');
$request = $request->withParam('controller', 'Articles');

Deprecate ArrayAccess methods

The ArrayAccess interface was added in 2.x to provide an easier upgrade path from 1.x. This interface would be deprecated in 3.4. It is not possible to support ArrayAccess and immutable objects long term. Long term, this would remove calls like:

$request['controller']; // Access $request->params['controller']
$request['url']; // Alias for $request->query;
$request['data']; // Alias for $request->data;

Deprecate magic methods

The current magic methods __get, __isset would be deprecated. These interfaces are infrequently used, and become harder to support in an immutable context. This would break access like:

$request->controller; // Access $request->params['controller']

Deprecate public properties

The existing public properties base, here, url, webroot, query, data and cookies would be deprecated. Instead data should be read through new/existing public methods:

The data, cookies and query parameters would be replaced with the existing sugar methods:

// Parameters
$request->query // Becomes $request->getQuery(), or $request->getUri()->getQuery()
$request->data // Becomes $request->getData(), or $request->getParsedBody()

// Cookies
$request->cookies // Becomes $request->cookie(), or $request->getCookie()

The current public URI properties would be available as request 'attributes', or be exposed through new methods that match the PSR7 conventions:

// Uri properties
$request->base // Becomes $request->getAttribute('base')
$request->webroot // Becomes $request->getAttribute('webroot')
$request->url // Becomes $request->getUri()->getPath()
$request->here // Becomes $request->getHere()

Because attributes and the existing public properties do not share the same storage, we would need to leverage __get, __set and __isset to emulate the properties using the PSR7 storage. Most of the emulated properties are scalar values which is great, as they are simple to emulate. However, data, cookies and query are arrays, and emulating arrays with __get and __set can have some limitations. I think this is a situation where we would need documentation in the migration guide to explain the limitations.

Impact to Existing Components

Several core components would be effected by the switch to immutable request objects, and would need to be re-factored/re-built. While these updates are not critical for 3.4.0 as we'll continue to support the in-place mutation methods, long term we need to replace the effected components with middleware based solutions, and I think it is important to have plans now on how those updates will be done.

I am not recommending these changes be done in 3.4.0. In order to keep releases smaller, component to middleware changes could be done in 3.5.0.

Convert CsrfComponent to Middleware

This component could be replaced entirely by a middleware layer. The component could be deprecated entirely, we could recommend people update to the new Middleware layer.

RequestHandlerComponent Input Parsing

The input parsing feature of RequestHandlerComponent would need to be moved into middleware. The other features of RequestHandlerComponent could stay in the component.

beforeRedirect conversion into requestAction

This feature should be deprecated and planned for removal in 4.x. This feature was missed in the 2.x deprecations and should be removed now.

Convert SecurityComponent to Middleware

SecurityComponent token validation could be converted into Middleware as well. The requireAuth and requireSecure features could be moved into a separate middleware allowing each layer to be more focused on solving specific problems. The underlying token validation and token generation should be moved into a utility class that can be re-used in both the component and middleware layer.

Unimpacted Core Components

• AuthComponent - No impact as auth manipulates the session which is not-immutable.
• FlashComponent - Uses Session data exclusively.
• CookieComponent - Uses Cookies which go into the Response (which is a separate harder problem).

  TODO

• [x] Implement PSR7 interface on request.
  • [x] Attribute methods
  • [x] Query string methods
  • [x] Server parameter methods
  • [x] Cookie methods
  • [x] HTTP method methods
  • [x] Parsed Body methods
  • [x] Uploaded Files methods
  • [x] Body stream methods
  • [x] Request target methods
  • [x] Protocol Version methods
  • [x] Header methods
  • [x] Implement interface
  • [x] Uri methods
• [x] Make ServerRequestFactory return a Network\Request instance.
• [x] Deprecate and remove usage of RequestTransformer. Instead pass Cake\Http\ServerRequest through the middleware and controller.
• [x] Create Cake\Http\ServerRequest to be an alias for Cake\Network\Request.
• [x] Update core code to use PSR7 methods where possible.
• [x] Update Validation methods that deal with files to work with UploadedFileInterface.
• [x] Update documentation to include pre + post 3.4 examples.
• [x] Add new getter methods that more closely follow PSR7 conventions.

  Changelog

• Removed added withQuery method. The current query() method does not allow modifying the query parameters.
• Added deprecations of getter mode in mixed mode query/param/data methods.

[davidyell]

Adopting a standard is always welcome to me, the more the framework can do to bring its parts inline with standards the better.

[johannesnagl]

Although there's a lot to be changed (Framework as well as existing Code bases in the wild) I would vote for the change. Using these standards make a lot of sense.

[ignaciocarre]

Hi guys, always learning new things with you.. I think this change is for good. Fist of all, having less duplicated code means better code to me, and the request object should be immutable (in my personal experience of 5 years with cakephp i never change the request object). So I upvote for this change :+1:

[neojoda]

Agreed.

[hmic]

Can we stop forcing PSR7 into 3.x please! The 3.3 release was a disaster already as it had (unintentional) BC breaks and quite some bugs slipped through because of the big changes involving PSR7...

This is a 4.0 thing! - That said, a 4.0 release could be scheduled way sonner than 3.0 from the initial 2.0 release :P I'd rather EOL 2.x, support 3.x another year and have 4.0 pretty soon...

[dereuromark]

How would that (and the bugs) be any different when you upgrade to 4.x then? Version numbers aside. I think we need in general a better community effort here to test pre-releases before we can mark them as stable to avoid those many small bugfixes afterwards. That would actually help getting the quality we want to ship. Note that the current app templates now use 3.3.* by default, which you should, as well. And maybe just don't be the early adopter if you feel that strongly about tripping over those bugs.

[thinkingmedia]

I can help with PSR7 issues or bugs. Please point me to any open issues that need to be fixed asap.

[ADmad]

@thinkingmedia Thank you, all the one's reported have been taken care of. We need to wait if people find more :)

[adayth]

-1 , I prefer to have this change in 4.0 version.

[markstory]

@hmic What I'm trying to avoid by incrementally adding PSR7 support is another 'blow up the world' kind of upgrade we had with 3.0. I completely agree that the 3.3.0 release was not smooth, and contained more regressions that I would have liked. With that said I had deployed the various beta releases to all the sites I could to do additional testing. I also agree with the comment that @dereuromark made. All of the issues that were found post 3.3.0 could have been found if people had tried the beta/rc releases.

If we did push all the PSR7 work into a 4.0 release, how would it be less bumpy than the current process?

[thinkingmedia]

@markstory it would be a good idea to add the beta releases to the CakePHP home pages as an alternate download option for visitors. That would get more people using it.

[dereuromark]

So let's do the following once 3.4 is beta/RC: We tweet and communicate it very loudly, we basically trump it into the world.

Then everyone and every project does the following:

git checkout -b testing-new-minor
composer require ~3.4.0

• Check php phpunit
• Check frontend by clicking around
• Note any potential BC break and report back to core

This should in total not take more than a few minutes and potential issues can be fixed prior to the stable release saving that person and everyone else collectively so much time and trouble in the long run.

[markstory]

Because attributes and the existing public properties do not share the same storage, we would need to leverage get, set and __isset to emulate the properties using the PSR7 storage.

I've been thinking about this more, and I like it less each day. Instead I'm thinking of pushing all the shim code into the withAttribute() and getAttribute() methods. This is less likely to cause problems with existing applications.

[thinkingmedia]

@markstory what specs are you following to make these changes? Is there a PSR-7 developers guide or something?

[markstory]

@thinkingmedia I'm using the psr7 docs and looking at zend-framework/zend-diactoros.

[ADmad]

@markstory This is done right?

[markstory]

Yeah we can close this. I'll handle " Add new getter methods that more closely follow PSR7 conventions." as a separate piece of work, as I think those method signatures aren't fully fleshed out yet.