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.

Sign up for GitHub

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

Jump to bottom

[Symfony 6] Fix zone member integrity listener #14065

Merged
merged 7 commits into from
Jun 8, 2022
Merged

[Symfony 6] Fix zone member integrity listener #14065

merged 7 commits into from
Jun 8, 2022

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Jun 7, 2022 •
edited
Loading

Q A
Branch? 1.11
Bug fix? yes (for Symfony 6)
New feature? no
BC breaks? not yet
Deprecations? no
Related tickets partially #13274 related #14041
License MIT

@loic425 loic425 requested a review from a team as a code owner June 7, 2022 16:54
@loic425 loic425 mentioned this pull request Jun 7, 2022
Closed
@GSadee GSadee added the DX Issues and PRs aimed at improving Developer eXperience. label Jun 8, 2022
@loic425 @GSadee
Apply suggestions from code review
9ade6f2 
Co-authored-by: Grzegorz Sadowski <sadowskigp@gmail.com>
Zales0123
Zales0123 reviewed Jun 8, 2022
View reviewed changes
src/Sylius/Bundle/AddressingBundle/EventListener/ZoneMemberIntegrityListener.php Outdated
use Symfony\Component\HttpFoundation\Session\Flash\FlashBagInterface;
use Symfony\Component\HttpFoundation\Session\SessionInterface;
use Webmozart\Assert\Assert;

final class ZoneMemberIntegrityListener
{
public function __construct(
private SessionInterface $session,
private SessionInterface|RequestStack $sessionOrRequestStack,
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just use RequestStack here? It already has getSession() method in Symfony 5, so we will not have a problem with removing it later 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about what you mean. If you talk about the argument type SessionInterface|RequestStack, we cannot getting session from RequestStack on Symfony 4.4.
If you talk about the argument name "$requestStack", I agree, we can use this name to make it simpler while dropping Symfony 4 support (next release).

@GSadee GSadee merged commit 4166b1b into Sylius:1.11 Jun 8, 2022
@GSadee
Copy link
Member

GSadee commented Jun 8, 2022

Thanks, Loïc! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zales0123 Zales0123 Zales0123 left review comments

@GSadee GSadee GSadee approved these changes

Assignees
No one assigned
Labels
DX Issues and PRs aimed at improving Developer eXperience.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@loic425 @GSadee @Zales0123