Add symfony request factory in sonata runtime #1739

eerison · 2023-12-08T11:53:40Z

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

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

VincentLanglet

Why this wasn't needed before the runtime then ?

GeraudBourdin · 2023-12-08T12:38:34Z

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()

eerison · 2023-12-08T12:45:13Z

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 ;)

eerison · 2023-12-08T13:13:12Z

src/Runtime/SonataPagePathRuntime.php

@@ -27,6 +28,21 @@ public function getRunner(?object $application): RunnerInterface
            return parent::getRunner($application);
        }

+        Request::setFactory(


ok here we go, it was the reason!

into the RequestFactory, it was calling this code.

I don't know why they were adding this. does it create a main request? or does it create a new request? I am not sure. But the thing is: it solves the issue.

it is the line: https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/Request/RequestFactory.php#L80

@VincentLanglet @GeraudBourdin

ok here we go, it was the reason!

into the RequestFactory, it was calling this code.

I don't know why they were adding this. does it create a main request? or does it create a new request? I am not sure. But the thing is: it solves the issue.

yes the initial goal was to override the main request.

do you have any idea why do we need to do this?

Well it is the same behaviour of RequestFactory, my only point here is, we don't know why do we need this.
why do we need to override the main request 😢 ?

by the way issue is fixed and no extra params is needed. So this should be accepted to be merged

for your information, it was an implementation from Symfony 2 🤯

2b70f81#diff-2d4fd4a3aac457940ec248d6e8421c65f88a2fc4035a3494416b29826bac53e5

by the way issue is fixed and no extra params is needed. So this should be accepted to be merged

yeah I know, I just want to know if we should add this or check if it is a main request on event.
because after this be released, I guess we can't just remove it, probably we need to keep some BC :(

I prefer the mainRequest check, but if subrequest is used and if it is, may be siteRequest is needed. Could this Request::setFactory thing coould be deprecated for the 5.x release maybe ?

I prefer the mainRequest

me too, the current approach sounds like workaround, check the main request is more common to see in symfony's doc

haivala · 2023-12-08T13:27:24Z

Can we have release asap?

eerison · 2023-12-08T13:30:04Z

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 · 2023-12-08T13:39:07Z

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 · 2023-12-08T13:45:38Z

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 · 2023-12-08T13:48:44Z

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
2b70f81#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 · 2023-12-08T13:49:44Z

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 · 2023-12-08T13:51:38Z

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 · 2023-12-08T13:55:06Z

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 :)


<?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.

if you check the last commit I was using the old implementation Request::create, But we decided to check if it is main quest into the HostPath requests for more information check the descussion on this pr: sonata-project#1739 Note: use this approach was a try to remove a strange implementation that is not clear how it works. But there isn't other reason, it was just to try to use more symfony's example. Note 2: In case you know why Request::create is needed, please open a issue or document this somewhere.

eerison · 2023-12-08T14:27:54Z

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 · 2023-12-08T14:31:58Z

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

haivala · 2023-12-08T14:41:57Z

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 · 2023-12-08T14:48:28Z

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 · 2023-12-08T14:49:52Z

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 · 2023-12-08T14:52:56Z

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 · 2023-12-08T14:55:02Z

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

GeraudBourdin · 2023-12-08T14:59:07Z

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 · 2023-12-08T15:00:40Z

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

yes it does

eerison · 2023-12-08T15:03:00Z

@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 · 2023-12-08T15:05:05Z

@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 · 2023-12-08T16:06:27Z

Thanks

VincentLanglet reviewed Dec 8, 2023

View reviewed changes

Add symfony request factory in sonata runtime

90891f8

eerison force-pushed the fix-sub-request-for-runtime branch from 924cab2 to 90891f8 Compare December 8, 2023 13:10

eerison changed the title ~~Skip sub request for host path~~ Add symfony request factory in sonata runtime Dec 8, 2023

eerison commented Dec 8, 2023

View reviewed changes

eerison marked this pull request as ready for review December 8, 2023 13:16

eerison marked this pull request as draft December 8, 2023 13:49

eerison force-pushed the fix-sub-request-for-runtime branch from e36521b to 90891f8 Compare December 8, 2023 14:26

eerison marked this pull request as ready for review December 8, 2023 14:27

eerison requested review from VincentLanglet and GeraudBourdin December 8, 2023 14:28

GeraudBourdin approved these changes Dec 8, 2023

View reviewed changes

VincentLanglet added the patch label Dec 8, 2023

VincentLanglet approved these changes Dec 8, 2023

View reviewed changes

VincentLanglet merged commit 45c984a into sonata-project:4.x Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add symfony request factory in sonata runtime #1739

Add symfony request factory in sonata runtime #1739

eerison commented Dec 8, 2023 •

edited

Loading

VincentLanglet left a comment

GeraudBourdin commented Dec 8, 2023

eerison commented Dec 8, 2023

eerison Dec 8, 2023

eerison Dec 8, 2023

eerison Dec 8, 2023

GeraudBourdin Dec 8, 2023

eerison Dec 8, 2023 •

edited

Loading

GeraudBourdin Dec 8, 2023

eerison Dec 8, 2023

eerison Dec 8, 2023

GeraudBourdin Dec 8, 2023

eerison Dec 8, 2023

haivala commented Dec 8, 2023

eerison commented Dec 8, 2023

VincentLanglet commented Dec 8, 2023

haivala commented Dec 8, 2023

eerison commented Dec 8, 2023

VincentLanglet commented Dec 8, 2023 •

edited

Loading

eerison commented Dec 8, 2023

GeraudBourdin commented Dec 8, 2023 •

edited

Loading

eerison commented Dec 8, 2023 •

edited

Loading

haivala commented Dec 8, 2023

haivala commented Dec 8, 2023

GeraudBourdin commented Dec 8, 2023 •

edited

Loading

VincentLanglet commented Dec 8, 2023

GeraudBourdin commented Dec 8, 2023 •

edited

Loading

VincentLanglet commented Dec 8, 2023

GeraudBourdin commented Dec 8, 2023

haivala commented Dec 8, 2023

eerison commented Dec 8, 2023

eerison commented Dec 8, 2023 •

edited

Loading

VincentLanglet commented Dec 8, 2023

Add symfony request factory in sonata runtime #1739

Add symfony request factory in sonata runtime #1739

Conversation

eerison commented Dec 8, 2023 • edited Loading

Add symfony request factory into sonata runtime

Changelog

To do

Tests

VincentLanglet left a comment

Choose a reason for hiding this comment

GeraudBourdin commented Dec 8, 2023

eerison commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerison Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haivala commented Dec 8, 2023

eerison commented Dec 8, 2023

VincentLanglet commented Dec 8, 2023

haivala commented Dec 8, 2023

eerison commented Dec 8, 2023

VincentLanglet commented Dec 8, 2023 • edited Loading

eerison commented Dec 8, 2023

GeraudBourdin commented Dec 8, 2023 • edited Loading

eerison commented Dec 8, 2023 • edited Loading

haivala commented Dec 8, 2023

haivala commented Dec 8, 2023

GeraudBourdin commented Dec 8, 2023 • edited Loading

VincentLanglet commented Dec 8, 2023

GeraudBourdin commented Dec 8, 2023 • edited Loading

VincentLanglet commented Dec 8, 2023

GeraudBourdin commented Dec 8, 2023

haivala commented Dec 8, 2023

eerison commented Dec 8, 2023

eerison commented Dec 8, 2023 • edited Loading

VincentLanglet commented Dec 8, 2023

eerison commented Dec 8, 2023 •

edited

Loading

eerison Dec 8, 2023 •

edited

Loading

VincentLanglet commented Dec 8, 2023 •

edited

Loading

GeraudBourdin commented Dec 8, 2023 •

edited

Loading

eerison commented Dec 8, 2023 •

edited

Loading

GeraudBourdin commented Dec 8, 2023 •

edited

Loading

GeraudBourdin commented Dec 8, 2023 •

edited

Loading

eerison commented Dec 8, 2023 •

edited

Loading