Add symfony request factory in sonata runtime

[eerison]

Add symfony request factory into sonata runtime

I am targeting this branch, because runtime is released on 4.x.

Closes #1738.

Changelog

### Fixed
- Fix sonata runtime, adding Symfony request factory.

To do

• [x] Update the tests; no idea how to test it 😄

Tests

in theory just main request should be handled by This runtime, But should be good someone else give me a hint that it is working as expected.

@haivala @GeraudBourdin @tdumalin

[GeraudBourdin]

Why this wasn't needed before the runtime then ?

https://github.com/sonata-project/SonataPageBundle/issues/1738#issuecomment-1847079162

see my comment. initial MR was the right strategy. This side effect was introduced by skiping RequestFactory in the runtime and using directly SiteRequest::createFromGlobals()

[eerison]

Why this wasn't needed before the runtime then ?

#1738 (comment)

see my comment. initial MR was the right strategy. This side effect was introduced by skiping RequestFactory in the runtime and using directly SiteRequest::createFromGlobals()

it doesn't make sense works with RequestFactory, I will test it ;)

[haivala]

Can we have release asap?

[eerison]

Can we have release asap?

you can still keep using the old implementation until we release it.

But I guess it won't take too much time, I just want to hear @VincentLanglet opinion about it.

[VincentLanglet]

Can we have release asap?

you can still keep using the old implementation until we release it.

But I guess it won't take too much time, I just want to hear @VincentLanglet opinion about it.

I dont know a lot about this bundle especially the legacy code. I trust you about the fact that the solution is working.

The more code we can stop to use and deprecate, the better it is.

So if the mainrequest check solve the issue and avoid BC break, it's great. If it doesnt solve fully the issue, or add a big BC break, we should rework the runtime.

[haivala]

Can we have release asap?

you can still keep using the old implementation until we release it.

But I guess it won't take too much time, I just want to hear @VincentLanglet opinion about it.

it is more of an hassle to remove runtime than update this bundle :)

[eerison]

Can we have release asap?

you can still keep using the old implementation until we release it. But I guess it won't take too much time, I just want to hear @VincentLanglet opinion about it.

I dont know a lot about this bundle especially the legacy code. I trust you about the fact that the solution is working.

The more code we can stop to use and deprecate, the better it is.

So if the mainrequest check solve the issue and avoid BC break, it's great. If it doesnt solve fully the issue, or add a big BC break, we should rework the runtime.

Then let's check if it is main request on event, My felling is

when he introduced this feature, he needs to have SiteRequest on create method

to be able pass the correct Request here

$response = $kernel->handle($request);

But as we are using runtime, it will be done automatically.

how tests was written make me believe this https://github.com/sonata-project/SonataPageBundle/commit/2b70f8156afe64503710e0893c46ed92e8d7d6aa#diff-d1ddf4fd8188fdd6a4f3a879dad065e4fe5bbbb0377ae30e7fdd350196710193R51

he checks if it is SiteRequest or Request to pass to $response = $kernel->handle($request);

then let's check on event, in case it doesn't work, I can open a new PR doing the old way.

[VincentLanglet]

Can we have release asap?

you can still keep using the old implementation until we release it.

But I guess it won't take too much time, I just want to hear @VincentLanglet opinion about it.

it is more of an hassle to remove runtime than update this bundle :)

It depends ; you dont know the review process, the release process, when I'll have a computer in front of me soon. I'm doing this on my freetime and I'm not getting paid for this.

Downgrading take you time. A new release take my time.

I think Im already pretty quick in general. It's never nice to hear "asap".

[eerison]

Can we have release asap?

you can still keep using the old implementation until we release it.

But I guess it won't take too much time, I just want to hear @VincentLanglet opinion about it.

it is more of an hassle to remove runtime than update this bundle :)

It depends ; you dont know the review process, the release process, when I'll have a computer in front of me soon. I'm doing this on my freetime and I'm not getting paid for this.

Downgrading take you time. A new release take my time.

I think Im already pretty quick in general. It's never nice to hear "asap".

I think Im already pretty quick in general.

really quickly btw 😄

[GeraudBourdin]

Can we have release asap?

you can still keep using the old implementation until we release it. But I guess it won't take too much time, I just want to hear @VincentLanglet opinion about it.

it is more of an hassle to remove runtime than update this bundle :)

:trollface:

<?php
use App\Kernel;
use Sonata\PageBundle\Request\RequestFactory;

require_once dirname(__DIR__).'/vendor/autoload_runtime.php';

return function (array $context) {
    $request = RequestFactory::createFromGlobals('host_with_path_by_locale');
    $kernel = new Kernel($context['APP_ENV'], $context['APP_DEBUG']);
    $response = $kernel->handle($request);
    $response->send();
    $kernel->terminate($request, $response);
};

Use this in your index.php. Be aware that the main issue was 1 year old. I think you can wait for few hours/days.

[eerison]

I tried 😄

But there are some tests doing some stuffs with sub requests, then I will keep this implementation.

in the future if someone wants to do something with sub request, then we could have this more documented.

it was the errors using check into the event.

1) Sonata\PageBundle\Tests\Site\SubRequestsSiteSelectorTest::testOnKernelRequestWithSubDetectEn
Expectation failed for method name is "findBy" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

2) Sonata\PageBundle\Tests\Site\SubRequestsSiteSelectorTest::testOnKernelRequestWithSubDetectFr
Failed asserting that null is identical to 'fr'.

[haivala]

note to my self: don't use "asap" even if I mean well and meant that take the time you need.

[haivala]

What I really meant was that current latest version is broken so it would be nice to have new release. I can imagine that asap can some point come out as a curse word but that was not what I meant.

[GeraudBourdin]

What I really meant was that current latest version is broken so it would be nice to have new release. I can imagine that asap can some point come out as a curse word but that was not what I meant.

Current release do not break your app, it brings you the ability to use the new symfony way. So in the meantime use the old way that works for years :) i guess few hours to fix this is far away from what 've seen in my past jobs where new feature fix have to wait for ... the next two sprints (i've seen this yes... lol)

[VincentLanglet]

What I really meant was that current latest version is broken so it would be nice to have new release.

Sure, but:

• I'm working this week and I'm busy all the Week end.
• Currently there is no PR with a clear solution.

I let @eerison @GeraudBourdin discuss about the right approach, and I invite you @haivala to help about it if you want a quicker release. The sooner you all agree on the right solution, the sooner the PR can be merged/released.

@eerison To me it's currently not clear if we should change the runtime or add a MainRequest check. Please ping me (with some explantations) when you decided about it.

[GeraudBourdin]

the right

What I really meant was that current latest version is broken so it would be nice to have new release.

Sure, but:

• I'm working this week and I'm busy all the Week end.
• Currently there is no PR with a clear solution.

I let @eerison @GeraudBourdin discuss about the right approach, and I invite you @haivala to help about it if you want a quicker release. The sooner you all agree on the right solution, the sooner the PR can be merged/released.

@eerison To me it's currently not clear if we should change the runtime or add a MainRequest check. Please ping me (with some explantations) when you decided about it.

th right decision should be to keep the Request::setFactory in the runtime. Not the nicest, but the securiest for old apps.

[VincentLanglet]

So to merge the current state of this PR ? @haivala does it fix your issue ?

[GeraudBourdin]

I tried 😄

But there are some tests doing some stuffs with sub requests, then I will keep this implementation.

in the future if someone wants to do something with sub request, then we could have this more documented.

it was the errors using check into the event.

1) Sonata\PageBundle\Tests\Site\SubRequestsSiteSelectorTest::testOnKernelRequestWithSubDetectEn
Expectation failed for method name is "findBy" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

2) Sonata\PageBundle\Tests\Site\SubRequestsSiteSelectorTest::testOnKernelRequestWithSubDetectFr
Failed asserting that null is identical to 'fr'.

you mean that subrequest check break tests ?

[haivala]

So to merge the current state of this PR ? @haivala does it fix your issue ?

yes it does

[eerison]

@eerison To me it's currently not clear if we should change the runtime or add a MainRequest check. Please ping me (with some explantations) when you decided about it.

Yep

[eerison]

@eerison To me it's currently not clear if we should change the runtime or add a MainRequest check. Please ping me (with some explantations) when you decided about it.

@VincentLanglet, change the runtime is the correct one, because there are some tests checking sub requests, then they expect that sub request works, page bundle is doing something with this sub request.

then this PR is with the correct code to merge.

[VincentLanglet]

Thanks