These are chat archives for ppi/framework

20th
Nov 2015
Paul Dragoonis
@dragoonis
Nov 20 2015 01:19
@KorvinSzanto agreed
Andrew Carter
@AndrewCarterUK
Nov 20 2015 09:38
@KorvinSzanto "you can't just use both in parallel, you need to have one or the other" - exactly, let the application decide if it's going to use PSR-7 or HttpFoundation
I'd almost suggest you should do that at module level and it should be a similar decision to "which router are you using"
Paul Dragoonis
@dragoonis
Nov 20 2015 09:40
I was hoping one day for each module to have its own DiC, and thus Request object. Therefore you can begin to use Symfony DiC, and stuff… or use Laravel DiC with its own request bits ..
it’s what I was speaking to @mnapoli about
Andrew Carter
@AndrewCarterUK
Nov 20 2015 09:42
request object shouldn't go in the DiC IMO, if you ever wanted to create a version of PPI that wasn't limited to one request cycle (e.g. one that worked with PHPFastCGI) you'd need to change that
Matthieu Napoli
@mnapoli
Nov 20 2015 09:42
(jumping into the conversation) but you won't store the request in the container right?
Andrew Carter
@AndrewCarterUK
Nov 20 2015 09:42
that ^
Paul Dragoonis
@dragoonis
Nov 20 2015 09:43
at the moment you ask the Service Manager for the request. $sm->get(‘Request’) from any other service that needs it.
sounds like we have a little refactor job on our hands to get it out of there? :)
Andrew Carter
@AndrewCarterUK
Nov 20 2015 09:43
that's essentially global state though
if you can pick up the request object from the container like that it's no different really to doing $_POST $_GET
sure, you can mock the container that a service uses - but you can also override the super globals
it wont always be the case that the PPI kernel will only process one request
Matthieu Napoli
@mnapoli
Nov 20 2015 09:45
that would be a good refactoring indeed, injecting the request into services cause plenty of issues. Symfony 2.4 did this (http://symfony.com/blog/new-in-symfony-2-4-the-request-stack) by introducing a RequestStack. I'm not sure if it's the best solution but at least it's much better
Paul Dragoonis
@dragoonis
Nov 20 2015 09:46
we have RequestStack already in PPI
@noisebleed added it
RouterListener uses it
it’s unused though, by the looks of it
Vítor Brandão
@noisebleed
Nov 20 2015 09:59
Hi all
Paul Dragoonis
@dragoonis
Nov 20 2015 10:01
This message was deleted
Container interop: @mnapoli points out that there’s caveats to using container-interop.
PSR7: We need to move to symphony-only or psr7-only http stack and not hybrid. or keep 2 separate objects
Request/Response in the DiC: Guys are saying we should not store them in the SM, as they should be stateless.
Anyone want to fill in gaps?
Matthieu Napoli
@mnapoli
Nov 20 2015 10:05
Container interop: the standard allows to switch containers, but isn't very practical if you want to build modules where each module can provide services as well as override/extend services/entries from other modules (in other words, it's not easy to build a module architecture with the standard only)
Vítor Brandão
@noisebleed
Nov 20 2015 11:26
Agree with moving to PSR-7 only. The Symfony-PSR7 integration was made at a moment where PSR7 wasn't even stable.
Request should be passed to the controller action not retrieved from the SM unless I'm missing something
Paul Dragoonis
@dragoonis
Nov 20 2015 11:30
If we moved to PSR-7 only, then what about the symfony code that we have, which requires SF RequestInterface. Will we use bridge to convert from PSR-7 -> Symfony request object?
Vítor Brandão
@noisebleed
Nov 20 2015 11:31
most likely yes, haven't checked symfony-3.0 though
Paul Dragoonis
@dragoonis
Nov 20 2015 11:31
We likely wish to continue on 2.x, I think.
@noisebleed if we remove Request obj out of SM. Then if you are inside a factory and need current request then how will you get it ?
Vítor Brandão
@noisebleed
Nov 20 2015 11:35
What services are requiring the Request object?
dannym87
@dannym87
Nov 20 2015 11:35
do you even need access to the request/response object in factories?
Paul Dragoonis
@dragoonis
Nov 20 2015 11:35
I believe the Routing based ones need this.
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:39
@dragoonis the router should be handed the request object by the application class
$dispatcher = $router->match($request);
or w/e
Paul Dragoonis
@dragoonis
Nov 20 2015 11:40
Yes, we’re doing this - https://github.com/ppi/framework/blob/master/src/App.php#L743
but it’s asking SM for the request.
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:40
as soon as you have the request object being used to construct a service that service is no longer stateless
it's tied to the request scope and cant be reused
Vítor Brandão
@noisebleed
Nov 20 2015 11:41
needs to be passed in runtime, without state, agree
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:41
handleRouting() should take the request as a parameter
Paul Dragoonis
@dragoonis
Nov 20 2015 11:41
ZF2 has RequestFactory - by the way. In their SM
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:42
possible as should dispatch()
Paul Dragoonis
@dragoonis
Nov 20 2015 11:42
(please excuse the terrible App.php code, the rest of the framework is clean and App.php is just nasty glue :))
Paul Dragoonis
@dragoonis
Nov 20 2015 11:48
Okay, PPI\App no longer asks the SM for the Request object. It creates it itself using Request::createFromGlobals()
    public function getRequest()
    {
        if (null === $this->request) {
            $this->request = HttpRequest::createFromGlobals();
        }

        return $this->request;
    }
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:49
@dragoonis I'd say as long as the app has a property 'request' it's not stateless
Vítor Brandão
@noisebleed
Nov 20 2015 11:49
+1
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:50
What's the front controller method?
Vítor Brandão
@noisebleed
Nov 20 2015 11:50
how many times is getRequest() called?
Paul Dragoonis
@dragoonis
Nov 20 2015 11:50
@noisebleed getRequest is called 3 times.
@AndrewCarterUK dispatch() is called to initiate routing and controller resolving.
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:50
all that does is make the App a sort of request container ^
so yeh, in dispatch you should create the request and then pass it on to the router and controlling
Paul Dragoonis
@dragoonis
Nov 20 2015 11:51
k, stand by
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:51
it shouldn't linger as a class property
Vítor Brandão
@noisebleed
Nov 20 2015 11:51
it's used once in the controller resolver and again in the routing

it shouldn't linger as a class property

+1

^ that's a really good example, it means that when testing you can do an end to end test
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:54
$response = $application->run($request);
Paul Dragoonis
@dragoonis
Nov 20 2015 11:55
We’re almost there! I’m cleaning up as we speak, won’t be much longer now :)
Vítor Brandão
@noisebleed
Nov 20 2015 11:58
$request = $request ?: ServerRequestFactory::fromGlobals(); is what you are looking for @dragoonis
Paul Dragoonis
@dragoonis
Nov 20 2015 11:59
I’m cleaning this class up, to continue to use HttpRequest (our obj) and then break over to PSR-7 after.
I have PPI\App not using $this->getRequest() down to only 1 instance right now (from 4 instances).
Andrew Carter
@AndrewCarterUK
Nov 20 2015 11:59
I think to be fully stateless, the method App::getRequest() has to be removed
and the class property $this->request has to be removed too
Paul Dragoonis
@dragoonis
Nov 20 2015 12:00
It shall be done. 1 to go :)
some coupling here relying on it state of $this->request, between methods.
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:00
also the response at the moment is still in the container (just a reminder) :p
Paul Dragoonis
@dragoonis
Nov 20 2015 12:00
handleRouting is doing:
        $parameters['_route_params'] = $parameters;
        $this->getRequest()->attributes->add($parameters);
        return $parameters;
Called like this:
$routeParams = $this->handleRouting($request);
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:01
yup, so if it's being passed it as a method parameter
it doesnt need to do $this->getRequest()
Paul Dragoonis
@dragoonis
Nov 20 2015 12:01
It’s settings things inside request, for dispatch() to use later
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:02
so if you're using PSR-7 that's where middleware should come in
Paul Dragoonis
@dragoonis
Nov 20 2015 12:02
I’ve got it! :)
        $routeParams = $this->handleRouting($request);
        $request->attributes->add($routeParams);
No more getRequest() - removed class prop and getRequesT() method.
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:04
nice
have you got BC breaks there?
Paul Dragoonis
@dragoonis
Nov 20 2015 12:05
How’s this looking?
    public function run(RequestInterface $request = null)
    {
        if (false === $this->booted) {
            $this->boot();
        }

        $request = $request ?: HttpRequest::createFromGlobals();

        $response = $this->dispatch($request);
        $response->send();

        return $response;
    }
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:05
that's it!
dannym87
@dannym87
Nov 20 2015 12:05
looks good
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:05
it means you can do end to end tests over the dispatch method too
Vítor Brandão
@noisebleed
Nov 20 2015 12:05
\o/
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:09
the only issue you've got with that is BC breaks!
I don't know if any app is using those features
Paul Dragoonis
@dragoonis
Nov 20 2015 12:09
this is alpha copy - i’m okay with small BC - there have been no external interface changes.
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:09
but there's lots of changes to public methods
okay!
Paul Dragoonis
@dragoonis
Nov 20 2015 12:10
which public methods?
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:10
getRequest, dispatch, run
Vítor Brandão
@noisebleed
Nov 20 2015 12:11
this will get detailed in the CHANGELOG, BC breaks allowed for 2.2
Paul Dragoonis
@dragoonis
Nov 20 2015 12:26

we are gearing up for 2.1.0 release.

2.0 and 2.1 are structurally quite different. good news is nobody is really using 2.0, ppi was quite unknown then still.

what should we do with regards to versioning and BC

Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:28
technically speaking, you're only allowed BC breaks between X.Y and X.Z if X isn't 0
or you're on a RC3/beta/alpha release
if you've released 2.0 stable, you shouldn't have a BC break to 2.1
Paul Dragoonis
@dragoonis
Nov 20 2015 12:29
i understand semver, sucks!
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:29
because a composer update could break someones project
Paul Dragoonis
@dragoonis
Nov 20 2015 12:29
I called it 2.1.0 alpha bcoz i thought we were ready, obvs we’re not
are we looking at 3 here? :) /cc @noisebleed
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:29
the bigger problem is the 2.0
I think you can break B/C from an alpha, that's why it's an alpha
Paul Dragoonis
@dragoonis
Nov 20 2015 12:30
it’s 2.1.0-alpha, but you can’t break BC from 2.0 to 2.1, so .. ?
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:31
yeh, technically you're gearing up for a 3.0
if you're following semver
Paul Dragoonis
@dragoonis
Nov 20 2015 12:31
i’m cool with just going from 2.0 -> 3.0
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:31
I'm not sure if there's another way to say we've made minor B/C breaks that wont affect 90% of users so composer don't auto update this
have you specified that you're following semver?
Paul Dragoonis
@dragoonis
Nov 20 2015 12:32
no, but i think it would be unreasonable not to
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:32
I'd agree
Paul Dragoonis
@dragoonis
Nov 20 2015 12:32
:+1: thanks!
So - we have Request cleaned up - but Response is a bit trickier and i’ll explain why.
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:32
One thing I would say, set up a 3.0-dev branch or something (not sure what you'd call it to make sure it doesn't get tagged as release)
don't rush into this, because any future 3.0 breaks and you'll be straight into 4
at some point next week I'll go through the whole framework and see if I can see anything that you might look to change
and I'd recommend getting your core contributors to do the same
Paul Dragoonis
@dragoonis
Nov 20 2015 12:34
@noisebleed is the only core contributor, but @dannym87 has been getting his feet wet recently :)
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:34
you also need to run the tests against all the components
Paul Dragoonis
@dragoonis
Nov 20 2015 12:35

@AndrewCarterUK Response object.

Controllers can return String or Response object.
This is bcoz of the way symfony works and it lets you do “return $this->render()” which is a string.

So in dispatch() we see if it’s a string, and if so we grab the response object from ServiceManager, and put the string into it as the body.

See here:
https://github.com/ppi/framework/blob/master/src/App.php#L296

Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:35
yup
Paul Dragoonis
@dragoonis
Nov 20 2015 12:36
How to continue to allow String|Response, to achieve stateless Response ?
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:36
$response = is_string($result) ? new HtmlResponse($result) : $result;
or w/e your constructor is for a response from a string
(new HttpResponse)->setBody($result)
not sure what you're using
Paul Dragoonis
@dragoonis
Nov 20 2015 12:37
we’re using Symfony Response object.
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:37
might be something like that then
Ok, here it is actually:
            $controller = $module->getController();
            $result = $module->dispatch();

            switch (true) {

                // If the controller is just returning HTML content then that becomes our body response.
                case is_string($result):
                    $response = $controller->getServiceLocator()->get('Response');
                    break;

                // The controller action didn't bother returning a value, just grab the response object from SM
                case is_null($result):
                    $response = $controller->getServiceLocator()->get('Response');
                    break;

                // Anything else is unpredictable so we safely rely on the SM
                default:
                    $response = $result;
                    break;
            }
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:38
$module->dispatch($request) no?
and yeh, if the result isn't a response, you need to make a new response object and fill it
Paul Dragoonis
@dragoonis
Nov 20 2015 12:40
module->dispatch does this:
        $content = call_user_func_array(
            [$this->controller, $this->actionName],
            [$this->controller->getServiceLocator()->get('Request')]
        );
So it’s calling RequestFactory again ..
Andrew Carter
@AndrewCarterUK
Nov 20 2015 12:40
yup, so module->dispatch should accept the request as a parameter
rather than getting it from the service locator
Paul Dragoonis
@dragoonis
Nov 20 2015 14:42
@AndrewCarterUK done
Paul Dragoonis
@dragoonis
Nov 20 2015 15:20
            $result = $module->dispatch($request);

            switch (true) {

                // If the controller is just returning HTML content then that becomes our body response.
                case is_string($result):
                    $response = $controller->getServiceLocator()->get('Response');
                    break;

                // The controller action didn't bother returning a value, just grab the response object from SM
                case is_null($result):
                    $response = $controller->getServiceLocator()->get('Response');
                    break;

                // Anything else is unpredictable so we safely rely on the SM
                default:
                    $response = $result;
                    break;
            }

            $response->setContent($result);
        }
Korvin Szanto
@KorvinSzanto
Nov 20 2015 16:18
Not sure about using a service locator to get the response object, don't you want it to be passed in?
Paul Dragoonis
@dragoonis
Nov 20 2015 16:20
Yes. @dannym87 has a PR to pass Response jnto action too.
But what happens of you pass in response, but user returns string.
dannym87
@dannym87
Nov 20 2015 16:21
probably still needs looking at because the response object is coming from the container
Korvin Szanto
@KorvinSzanto
Nov 20 2015 16:21
@dragoonis you'd load the string into the response that they passed
dannym87
@dannym87
Nov 20 2015 16:21
if the user returns a string then it’s written into the stream object of the response
Paul Dragoonis
@dragoonis
Nov 20 2015 16:21
Do we revert back to $response we paased in originally
Korvin Szanto
@KorvinSzanto
Nov 20 2015 16:22
you shouldn't be making new instances of response, other than through the with* methods
People might have an application above yours that adds headers to the response, if you just create a response object from nowhere, those headers get lost
dannym87
@dannym87
Nov 20 2015 16:25
indeed. the response needs to go into the app the same way as the request probably. through the run() method and then passed to the controllers or what ever
Paul Dragoonis
@dragoonis
Nov 20 2015 16:25
how about this?
function dispatch($request) { 
    $result = $module->dispatch($request, $response);
    if(is_string($result)) {
        $response = $response->setContent($result);
    } else if($result instanceof ResponseInterface) {
        $response = $result;
    }
}
Korvin Szanto
@KorvinSzanto
Nov 20 2015 16:26
Yeah but with a request param too
Paul Dragoonis
@dragoonis
Nov 20 2015 16:26
shuld have function dispatch($request, $response) ?
Korvin Szanto
@KorvinSzanto
Nov 20 2015 16:26
Yeah
dannym87
@dannym87
Nov 20 2015 16:26
yup
Korvin Szanto
@KorvinSzanto
Nov 20 2015 16:26
Both params
Paul Dragoonis
@dragoonis
Nov 20 2015 16:26
Got it .
@dannym87 @KorvinSzanto I am on a bus right now - is somsone able to check out my feature/cleaning-request-handling branch and patch the ‘dispatch’ function with this ?
dannym87
@dannym87
Nov 20 2015 16:28
should also go into the run() method too i’d say. so you can pass an initial response into the app with extra headers or whatever as mentioned
Paul Dragoonis
@dragoonis
Nov 20 2015 16:29
:+1:
dannym87
@dannym87
Nov 20 2015 16:29
im not going to have time to look at it today unfortunately. sorry @dragoonis
Korvin Szanto
@KorvinSzanto
Nov 20 2015 16:31
:+1: I'll see if I'm comfortable making changes in there
Paul Dragoonis
@dragoonis
Nov 20 2015 16:32
@KorvinSzanto I’ve made some mods now. Got my laptop out, but I’m sure there’s more cleanup bits you want to make.
Give me 1 minute to push.
        $result = $module->dispatch($request);
        if(is_string($result)) {
            $response->setContent($result);
        } else if ($result instanceof HttpResponse) {
            $response = $result;
        } else {
            throw new \Exception('Invalid response type returned from controller');
        }
Is this okay?
Paul Dragoonis
@dragoonis
Nov 20 2015 17:06
@KorvinSzanto tadaaa! I pushed my changes. ppi/framework#137
This message was deleted
Ready for you
Since Request is no longer in the SM - we have 1 instance in the framework that is broken.
https://github.com/ppi/framework/blob/master/src/ServiceManager/Config/TemplatingConfig.php#L103
$serviceManager->setFactory('templating.helper.assets', function ($serviceManager) {
    return new AssetsHelper($serviceManager->get('request')->getBasePath());
});
Paul Dragoonis
@dragoonis
Nov 20 2015 17:24
How will the contoller perform redirects now? This is the current code
protected function redirect($url, $statusCode = 302)
{
    $this->getServiceLocator()->set('Response', new RedirectResponse($url, $statusCode));
}