Permit HttpFoundation\Request class override #7461

jfsimon · Mar 23, 2013

This PR solves #7453. Requests are now created through a RequestFactoryInterface.
The default factory just call ::create() method on the request class which can be configured with request_class parameter under framework section.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	I don't know
Deprecations?	no
Tests pass?	yes
Fixed tickets	#7453

stloyd · Mar 23, 2013

src/Symfony/Bundle/FrameworkBundle/HttpCache/HttpCache.php

     */
-    public function __construct(HttpKernelInterface $kernel, $cacheDir = null)
+    public function __construct(HttpKernelInterface $kernel, $cacheDir = null, $requestClass = null)


Maybe typehint class name instead of just comment ?

You mean __construct(..., $requestClass = 'Symfony\Component\HttpFoundation\Request')?

More like:

use Symfony\Component\HttpFoundation\Request; // (...) public function __construct(..., Request $requestClass = null) {}

But this must be class name (string), not instance :)

schmittjoh · Mar 23, 2013

By injecting the request class, you're basically creating many many factories. If someone wants to replace some parameters of the create call, then that is not so nice. Why not create a factory and inject that?

docteurklein · Mar 23, 2013

yes, a factory would be the best option.

jfsimon · Mar 23, 2013

@schmittjoh I was thinking about this option while using call_user_func but found it overkill.
That said, your parameters argument is pretty convincing! Let's do that.

igorw · Mar 23, 2013

Why do you need to extend the request class? That really should not be necessary, the request should be treated like a value (i.e. data).

fabpot · Mar 23, 2013

@igorw I totally agree with you... but sometimes, mostly to ease the migration from an old code base to Symfony, this is needed. In fact, I was about to reject the changes we made recently about this, but then I realized that it would help a lot with some of our customers. Moreover, the possibility to override the Request class is documented. So, you should not extend the Request class, but we (unfortunately) should cover this use case.

lyrixx · Mar 23, 2013

src/Symfony/Bundle/FrameworkBundle/HttpCache/HttpCache.php

    }

    protected function createStore()
    {
-        return new Store($this->cacheDir ?: $this->kernel->getCacheDir().'/http_cache');
+        return new Store($this->getKernel()->getContainer()->get('request_factory'));


You forgot the cache dir ?

@lyrixx wow, good one! thanks :)

jfsimon · Mar 24, 2013

@fabpot should I add something to a changelog? If yes, which one?

stof · Mar 24, 2013

src/Symfony/Component/HttpKernel/Request/RequestFactory.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\Request;


Why putting it in HttpKernel ? It only depends on HttpFoundation

I put it there because it's only used in HttpKernel, but now that you mention it... I think I should move it.

beberlei · Apr 29, 2013

Why use call_user_func_array here and not implement a Reuqest factory for the default Request and then hvae people extend the factory if they need something else? Doesn't make so much eense to make it so generic here or?

jfsimon · Apr 29, 2013

@beberlei the idea is to permit to extend the Request class and simply set its name to the factory (I added a config entry under framework section). It's still possible to implement your own factory and replace the existing service.

beberlei · Apr 30, 2013

@jfsimon i think the config variable is not needed, because overriding the request is only necessary in <1% of all use-cases probably much less and in that case registering your own factory is just a one liner again using the .class% parameter, plus a 10 line factory. That is what the DIC is about and what you try to solve again by building a super generic factory class.

beberlei · Apr 30, 2013

@fabpot while I agree this PR and GH-7707 are related, they don't touch the same code at all. I would see them as separate.

jfsimon · Apr 30, 2013

@beberlei okay, I understand your point. You're right, this is too much. I will remove the $class property and the request_class parameter today (to have a chance to see this PR merged in 2.3).

beberlei · Apr 30, 2013

@jfsimon its just my opinion, it doesn't count for mergeability :-)

jfsimon · Apr 30, 2013

@beberlei I know, but I use to over engineer everything. Simpler is better, isn't it?

Squashed commits: [Httpernel] added request factory [FrameworkBundle] added request_factory service moved request factory in HttpFoundation component [HttpFoundation] added request factory class check

jfsimon · Jul 10, 2013

Rebased & simplified (with a huge delay).
@fabpot is something missing to be merged?

fabpot · Sep 7, 2013

As overriding the Request class is quite an edge case, I propose something that is less intrusive and still flexible enough: see #8957

igorw · Sep 8, 2013

IMO this patch is the lesser evil than introducing yet another global var on the request, which could lead to strange side-effects. But I believe it can be vastly simplified by removing RequestFactoryInterface and replacing it with a callable that defaults to Request::create.

Just my $0.02.

jfsimon · Sep 8, 2013

@igorw thanks for mentioning theses awful static properties... the solution you propose is the one I used in the first place, then came some convincing comments in favor of a factory. Now I really think this factory is a good point as it may permit to remove the static propoerties from the request.

@fabpot what's your mind on this factory + static properties removal thing?

fabpot · Sep 8, 2013

My idea with the other PR was to reduce the impact of supporting this feature as it is really just an edge case and I'm not comfortable with changing so many things just for that.

fabpot · Sep 13, 2013

Also, with my hacks, all current bundles creating Requests will be "fixed" automatically. That's not the case with this PR

fabpot · Sep 13, 2013

As this is really just to fix some edge cases for people moving to Symfony2, I really don't like this PR. So, we can either merge mine or close it as a won't fix.

…ss (fabpot) This PR was merged into the master branch. Discussion ---------- [HttpFoundation] added a way to override the Request class | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7461, #7453 | License | MIT | Doc PR | symfony/symfony-docs#3021 This is an alternative implementation for #7461. I've also reverted #7381 and #7390 as these changes are not needed anymore. Todo: - [ ] add some tests Commits ------- 464439d [HttpFoundation] added a way to override the Request class

stloyd reviewed Mar 23, 2013
View reviewed changes

lyrixx reviewed Mar 23, 2013
View reviewed changes

stof reviewed Mar 24, 2013
View reviewed changes

jfsimon mentioned this pull request Apr 2, 2013

Tag render with customized request class #7192

Closed

fabpot mentioned this pull request Apr 22, 2013

GH-7007: Synchronized Service alternative, backwards compatible. #7707

Closed

jfsimon added 2 commits July 10, 2013 06:53

added request factory

187f91c

Squashed commits: [Httpernel] added request factory [FrameworkBundle] added request_factory service moved request factory in HttpFoundation component [HttpFoundation] added request factory class check

removed unuseful kernel.request_class parameter

d805ca8

fabpot mentioned this pull request Sep 7, 2013

[HttpFoundation] added a way to override the Request class #8957

Merged

1 task

fabpot closed this Sep 13, 2013

Search code, repositories, users, issues, pull requests...

Uh oh!

Permit HttpFoundation\Request class override #7461

Permit HttpFoundation\Request class override #7461

Uh oh!

Conversation

jfsimon commented Mar 23, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmittjoh commented Mar 23, 2013

Uh oh!

docteurklein commented Mar 23, 2013

Uh oh!

jfsimon commented Mar 23, 2013

Uh oh!

igorw commented Mar 23, 2013

Uh oh!

fabpot commented Mar 23, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfsimon commented Mar 24, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beberlei commented Apr 29, 2013

Uh oh!

jfsimon commented Apr 29, 2013

Uh oh!

beberlei commented Apr 30, 2013

Uh oh!

beberlei commented Apr 30, 2013

Uh oh!

jfsimon commented Apr 30, 2013

Uh oh!

beberlei commented Apr 30, 2013

Uh oh!

jfsimon commented Apr 30, 2013

Uh oh!

jfsimon commented Jul 10, 2013

Uh oh!

fabpot commented Sep 7, 2013

Uh oh!

igorw commented Sep 8, 2013

Uh oh!

jfsimon commented Sep 8, 2013

Uh oh!

fabpot commented Sep 8, 2013

Uh oh!

fabpot commented Sep 13, 2013

Uh oh!

fabpot commented Sep 13, 2013

Uh oh!

Uh oh!