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

Clean the use of flash messages #246

Closed
stof opened this issue Jul 11, 2011 · 13 comments
Closed

Clean the use of flash messages #246

stof opened this issue Jul 11, 2011 · 13 comments
Assignees
Milestone

Comments

@stof
Copy link
Member

stof commented Jul 11, 2011

Currently, the bundle does not use flash messages in a clean way, setting the translation key as message key and the CSS class as message. This works well as long as the flash message is rendered using the bundle's layout but breaks if the user replaces it.
The layout breaks also flash messages set by the user before going to a FOSUB page.

@lsmith77
Copy link
Member

I don't understand the issue. There is no "standard" for setting flash messages, though maybe there should be.

@stof
Copy link
Member Author

stof commented Jul 11, 2011

@lsmith77 putting the actual message as key of the flash message and the class as message seems wrong. As the flash message impacts the next request, we cannot be sure that they will be rendered by FOSUserBundle (or that they are set by FOSUserBundle for the first request reaching it)

@lsmith77
Copy link
Member

But what else could we do? And how would anything else ensure a better outcome?

@stof
Copy link
Member Author

stof commented Jul 11, 2011

dunno. This issue is about thinking about the issue to eventually find a better way than the current one.

@lsmith77
Copy link
Member

well maybe we should define a standard and add it to the docs as a best practice then.

@stof
Copy link
Member Author

stof commented Jul 11, 2011

@fabpot Do you have a best practice in mind about the way flash messages should be used ?

See also #132 for a previous discussion about them.

@ghost ghost assigned stof Jul 29, 2011
@stof stof closed this as completed in 3c735ad Jul 29, 2011
@bartclarkson
Copy link

Hello-

I came across a post that relates to this, I believe: https://coderwall.com/p/ybup5a

Why not pass your setFlash $value more like this:

$this->setFlash('notice', $this->container->get('translator')->trans('change_password.flash.success', array(), 'FOSUserBundle'));

In any event, thank you for the excellent bundle.

@bartclarkson
Copy link

Here's how I achieved this, probably as intended. I just extended all the controllers and overrode the setFlash method. Example here is the Registration Controller:


namespace Acme\UserBundle\Controller;

use FOS\UserBundle\Controller\RegistrationController as BaseRegistrationController;

class RegistrationController extends BaseRegistrationController
{
    protected function setFlash($action, $value)
    {
        $value = $this->container->get('translator')->trans($value, array(), 'FOSUserBundle');
        $this->container->get('session')->setFlash($action, $value);
    }

}
?>

Which allows the following in my top-level .twig template:

            {% for key, message in app.session.getFlashes() %}
            <div class="flash-message-wrapper">
                <div class="flash-message {{ key }}">
                    <span>{{ message }}</span>
                </div>
            </div>
            {% endfor %}

@stof
Copy link
Member Author

stof commented Dec 31, 2012

@bartclarkson this issue has been resolved several months ago already.

@mdjaman
Copy link

mdjaman commented Feb 9, 2014

@stof how you resolved this issue?

@mdjaman
Copy link

mdjaman commented Feb 9, 2014

I don't see any changes?

@stof
Copy link
Member Author

stof commented Mar 4, 2014

The 2.0 version (master branch) is translating messages in the listener setting them, and sets it properly (rather than using the message as key and the type as value)

@stof
Copy link
Member Author

stof commented Mar 4, 2014

btw, the commit doing the change is linked (it was the commit closing the issue)

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

No branches or pull requests

4 participants