made session flashes closeable with configuration

[lsv]

With the following configuration

mopa:
    flash:
        closeable: true

Session flashes are now closeable, meaning it adds the X to flash message

[isometriks]

Couple things.. You should probably just pass the parameter to the twig extension instead of passing the whole container, and I'm not sure about adding another global variable to Twig, I wonder if it makes more sense to make it a function and then call it when you want the value? I'm not really sure what the best option is for doing something like that.

[isometriks]

Also, we don't need the initRuntime / environment variable.

[lsv]

I dont know how to only pass the argument in the service, in Symfony 2.4 it would be something like this

<argument type="expression">@=container.hasParameter('mopa_bootstrap.flash.closeable') ? parameter('mopa_bootstrap.flash.closeable') : false</argument>

But in <2.4 I have no idea - I have tried with

<argument type="string">@=container.getParameter('mopa_bootstrap.flash.closeable')</argument>

But this obvious does not work - anybody knows which argument type it should be?

[isometriks]

%path.to.parameter%

You don't need expression language at all On Apr 9, 2014 11:14 AM, "Martin Århof" [email protected] wrote:

I dont know how to only pass the argument in the service, in Symfony 2.4 it would be something like this

@=container.hasParameter('mopa_bootstrap.flash.closeable') ? parameter('mopa_bootstrap.flash.closeable') : false

But in <2.4 I have no idea - I have tried with

@=container.getParameter('mopa_bootstrap.flash.closeable')

But this obvious does not work - anybody knows which argument type it should be?

Reply to this email directly or view it on GitHubhttps://github.com/phiamo/MopaBootstrapBundle/pull/840#issuecomment-39975896 .

[isometriks]

And you'll either need to set the parameter regardless or do:

if (isset($config['flash']) {
    // ...
} else {
    $container->removeDefinition('mopa_bootstrap.twig.extension.bootstrap_flash');
}

[lsv]

Now if session_flash(true) is called, the X is always showed regardless of the config

If session_flash() is called, and no config is set, it will not show the X.

If the config

mopa_bootstrap:
    flash:
        closeable: true

is set, it will show the X

[isometriks]

Why are you using setting injection? Just add it to the constructor

[lsv]

Well, mostly Im fan of getters and setters, and not passing everything to the contructor - but well its a personal reference :)

[isometriks]

I mean, there's really no use for a setter here, the twig extension is pretty specific. I would just change the global variable to a function just like the mappings. No need to add global variables for this to be inserted into every single template render.

[phiamo]

@lsv any news?

[isometriks]

He made the changes. Only thing I'm not crazy about is the $close === null part, I think this could be done in twig with a |default()

[lsv]

Remember $this->closeable is always set as either true or false (false is standard)

Im pretty sure |default only gets called when the return value is null

[isometriks]

Yeah I get that, it just seems like an unnecessary thing to put in a getter. Could change it to just be like this:

close is null ? mopa_bootstrap_flash_closeable() : close

or

close|default(mopa_bootstrap_flash_closeable())

That should be fine too if you don't pass in a close argument.

[lsv]

So in the twig you want

{% if close is null %}
   {% set close %}{{ mopa_bootstrap_flash_closeable() }}{% endset %}
{% endif %}
{{ flash_messages.flash(mapping[type], message, close, use_raw, class, domain) }}

[isometriks]

{{ flash_messages.flash(mapping[type], message, close|default(mopa_bootstrap_flash_closeable()), use_raw, class, domain) }}

or

{% set close = close is null ? mopa_bootstrap_flash_closeable() : close %}
{{ flash_messages.flash(mapping[type], message, close, use_raw, class, domain) }}

I think the first is better

[lsv]

The second is so much more readable - the first is really hard to see what its actually does imo

[phiamo]

@lsv is this ready to merge ?

[isometriks]

I'd still like the weird getter to be fixed and needs a blank line before the return statement to be PSR.

[phiamo]

@lsv can you update the PR, then we could merge it asap..

[phiamo]

@isometriks what should we do with this?

[isometriks]

Eh, I don't like that variable passthrough though, I'd rather use |default like I suggested above. Plus needs to be rebased anyway so just leave it for now.