-
Notifications
You must be signed in to change notification settings - Fork 8
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
IBX-570: Fixed dependency on session storage services #41
Conversation
$this->assertContainerBuilderHasParameter('session.save_path', 'my_save_path'); | ||
|
||
$this->assertContainerBuilderNotHasService('session.storage.native'); | ||
$this->assertContainerBuilderNotHasService('session.storage.php_bridge'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewers: technically we might not care if those services are present, so those two assertions could be removed.
👍 / 👎 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewers: technically we might not care if those services are present, so those two assertions could be removed.
/ ?
No strong feelings. Could stay to protect against merge-up mistakes, but maybe this is not that important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest after some thought I think this should be moved before the test. The most important thing is that the compiler pass does not crash the app if the session services are not there, so what we want to have is confirmation that their absence is handled.
Superseded by #42 (checks service defininition presence similarly to this PR) |
$this->assertContainerBuilderHasParameter('session.save_path', 'my_save_path'); | ||
|
||
$this->assertContainerBuilderNotHasService('session.storage.native'); | ||
$this->assertContainerBuilderNotHasService('session.storage.php_bridge'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewers: technically we might not care if those services are present, so those two assertions could be removed.
/ ?
No strong feelings. Could stay to protect against merge-up mistakes, but maybe this is not that important.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Merged into |
2.3
Follow up for #40.
This PR removes the dependency on
session.storage.native
andsession.storage.php_bridge
service definitions.Specifically, those two services do not exist in the test environment for Symfony framework (instead possibly being replaced with their mock variants (see https://symfony.com/doc/current/components/http_foundation/session_testing.html, or not used at all - when SecurityBundle is not loaded).
See https://github.com/ibexa/product-catalog/runs/2930845606?check_suite_focus=true and https://github.com/ibexa/product-catalog/runs/2929836113?check_suite_focus=true for test failures.
Blocks product catalog tests.
TODO:
$ composer fix-cs
).