Permit HttpFoundation\Request class override #7461

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
10 participants
Contributor

jfsimon commented 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 stloyd and 2 others commented on an outdated diff Mar 23, 2013

...ymfony/Bundle/FrameworkBundle/HttpCache/HttpCache.php
*/
- public function __construct(HttpKernelInterface $kernel, $cacheDir = null)
+ public function __construct(HttpKernelInterface $kernel, $cacheDir = null, $requestClass = null)
@stloyd

stloyd Mar 23, 2013

Contributor

Maybe typehint class name instead of just comment ?

@jfsimon

jfsimon Mar 23, 2013

Contributor

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

@stloyd

stloyd Mar 23, 2013

Contributor

More like:

use Symfony\Component\HttpFoundation\Request;

// (...)

public function __construct(..., Request $requestClass = null) {}
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

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

Contributor

schmittjoh commented 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?

Contributor

docteurklein commented Mar 23, 2013

yes, a factory would be the best option.

Contributor

jfsimon commented 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.

Contributor

igorw commented 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).

Owner

fabpot commented 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 lyrixx and 1 other commented on an outdated diff Mar 23, 2013

...ymfony/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'));
@lyrixx

lyrixx Mar 23, 2013

Member

You forgot the cache dir ?

@jfsimon

jfsimon Mar 23, 2013

Contributor

@lyrixx wow, good one! thanks :)

Contributor

jfsimon commented Mar 24, 2013

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

@stof stof and 1 other commented on an outdated diff Mar 24, 2013

...mfony/Component/HttpKernel/Request/RequestFactory.php
@@ -0,0 +1,43 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\Request;
@stof

stof Mar 24, 2013

Member

Why putting it in HttpKernel ? It only depends on HttpFoundation

@jfsimon

jfsimon Mar 24, 2013

Contributor

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

@jfsimon

jfsimon Mar 25, 2013

Contributor

done

@stof stof and 1 other commented on an outdated diff Mar 24, 2013

...mfony/Component/HttpKernel/Request/RequestFactory.php
+ */
+class RequestFactory implements RequestFactoryInterface
+{
+ /**
+ * @var string
+ */
+ private $class;
+
+ /**
+ * Constructor.
+ *
+ * @param string $class
+ */
+ public function __construct($class = 'Symfony\Component\HttpFoundation\Request')
+ {
+ $this->class = $class;
@stof

stof Mar 24, 2013

Member

shouldn't you check that it is a subclass of Request ?

@jfsimon

jfsimon Mar 24, 2013

Contributor

yep, it would be nice.

@jfsimon

jfsimon Mar 25, 2013

Contributor

done

Contributor

beberlei commented 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?

Contributor

jfsimon commented 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.

Contributor

beberlei commented 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.

Contributor

beberlei commented 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.

Contributor

jfsimon commented 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).

Contributor

beberlei commented Apr 30, 2013

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

Contributor

jfsimon commented Apr 30, 2013

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

jfsimon added some commits Mar 23, 2013

@jfsimon jfsimon added request factory
Squashed commits:
[Httpernel] added request factory
[FrameworkBundle] added request_factory service
moved request factory in HttpFoundation component
[HttpFoundation] added request factory class check
187f91c
@jfsimon jfsimon removed unuseful kernel.request_class parameter d805ca8
Contributor

jfsimon commented Jul 10, 2013

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

fabpot referenced this pull request Sep 7, 2013

Merged

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

0 of 1 task complete
Owner

fabpot commented 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

Contributor

igorw commented 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.

Contributor

jfsimon commented 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?

Owner

fabpot commented 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.

Owner

fabpot commented Sep 13, 2013

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

Owner

fabpot commented 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.

fabpot closed this Sep 13, 2013

@fabpot fabpot added a commit that referenced this pull request Oct 1, 2013

@fabpot fabpot feature#8957 [HttpFoundation] added a way to override the Request cla…
…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
2cd6e00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment