Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix menu in long-running process (RoadRunner, ReactPHP, Amphp) #7885

Merged
merged 8 commits into from Jul 28, 2022

Conversation

Warxcell
Copy link
Contributor

@Warxcell Warxcell commented Jul 28, 2022

In order to have new instance for every request in Long-running process (ReactPHP, Amphp, RoadRunner).
Otherwise menu is created once and then re-used - and that doesn't work if there is dynamic content in the menu, since first request creates the menu and then all other requests uses the menu created by first request.

Subject

I am targeting this branch, because bugfix.

Changelog

### Changed
- Sidebar menu is now non-shared. (new instance is created everytime its requested)

In order to have new instance for every request in Long-running process (ReactPHP, Amphp, RoadRunner)
VincentLanglet
VincentLanglet previously approved these changes Jul 28, 2022
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise menu is created once and then re-used - and that doesn't work if there is dynamic content in the menu.

@Warxcell Is there a way to add a test about the use-case which didn't work ?

@VincentLanglet VincentLanglet requested review from jordisala1991 and a team July 28, 2022 15:21
@@ -33,6 +33,7 @@
])

->set('sonata.admin.sidebar_menu', MenuItem::class)
->share(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we cant add a test, we should at least leave a comment about why we do this, imho

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise menu is created once and then re-used - and that doesn't work if there is dynamic content in the menu.

@Warxcell Is there a way to add a test about the use-case which didn't work ?

Sure. Adding it atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. :)

phpunit.xml.dist Outdated
@@ -33,5 +33,6 @@ It's auto-generated by sonata-project/dev-kit package.
<php>
<ini name="precision" value="8" />
<env name="SYMFONY_DEPRECATIONS_HELPER" value="max[self]=0" />
<env name="KERNEL_CLASS" value="Sonata\AdminBundle\Tests\App\AppKernel" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is autogenerated and cannot be touched.

Please add

protected static function getKernelClass(): string
{
    return AppKernel::class;
}

instead in your MenuTest file if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Warxcell Warxcell changed the title Make Menu non-shared Fix menu in long-running process (RoadRunner, ReactPHP, Amphp) Jul 28, 2022
@Warxcell
Copy link
Contributor Author

Failure in PSALM seems unrelated 🤷

@VincentLanglet
Copy link
Member

Failure in PSALM seems unrelated 🤷

I'm on it

public function testDynamicMenuInLongRunningProcess(): void
{
$client = static::createClient();
$client->disableReboot();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what makes the client share the same container? If yes, can you add a comment here to explain this line (ex: this ensures the container is reused across multiple request to simulate the behavior of ...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, adding

@VincentLanglet
Copy link
Member

For psalm it's related to vimeo/psalm#8330 ; we will need to ignore the issue

@VincentLanglet VincentLanglet merged commit 2b3c098 into sonata-project:4.x Jul 28, 2022
@VincentLanglet
Copy link
Member

Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants