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 flash messages in CartBundle being ignored while sent in event data #670

Merged
merged 1 commit into from
Nov 29, 2013

Conversation

stloyd
Copy link
Contributor

@stloyd stloyd commented Nov 29, 2013

No description provided.

@ghost ghost assigned stloyd Nov 29, 2013
{
$this->session->getFlashBag()->add('error', $event->getMessage() ?: $this->translator->trans($this->messages[$event->getName()], array(), 'flashes'));
$this->addFlash('success', $event->getMessage(), $event->getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

What if message is null? I prefer generic mesage over empty one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as before: if empty use default defined in listener.

Copy link
Contributor

Choose a reason for hiding this comment

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

But you replaced $event->getMessage() ?: $this->translator->trans($this->messages[$event->getName()] with $event->getMessage(). So it will be empty. Or I am missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@stloyd lol, sorry. Perfect! :)

@umpirsky
Copy link
Contributor

We also need to make all ItemResolvingException messages translatable in https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/CoreBundle/Cart/ItemResolver.php

@stloyd
Copy link
Contributor Author

stloyd commented Nov 29, 2013

@umpirsky That it's not case of bugfix, please don't mix things that are not subject of the issue. If they need to be changed, please open new issue about it, let's keep bugfixes "clean" to faster fix issues =)

@umpirsky
Copy link
Contributor

@stloyd Nono, you break i18n here :)

Message was translated before your fix, now they are in English.

@stloyd
Copy link
Contributor Author

stloyd commented Nov 29, 2013

@umpirsky No, no, no =P Check spec =P

@umpirsky
Copy link
Contributor

OK, merge it. I will rebase and fix in #665

cc @pjedrzejewski

pjedrzejewski pushed a commit that referenced this pull request Nov 29, 2013
Fix flash messages in CartBundle being ignored while sent in event data
@pjedrzejewski pjedrzejewski merged commit a4501be into Sylius:master Nov 29, 2013
@pjedrzejewski
Copy link
Member

Thanks Joseph!

@stloyd stloyd deleted the bugfix/cart_flash_msg branch November 30, 2013 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants