Make logout_url() and logout_path() return null if logout is not enabled?

[javiereguiluz]

Description
When using logout_url() and logout_path() Twig functions, you may trigger an exception if logout is not enabled in the app.

In an app created by you, this is not a problem. But if you are creating a template that can be used in any number of apps you don't have control over ... that's a big problem.

In EasyAdmin we display the logout URL only if logout is enabled in the app. I'd like to do this:

{% if logout_path() is not empty %}
  ...
{% endif %}

But I can't because logout_path() can throw exceptions. So we ended up recreating Symfony's LogoutUrlExtension to add a try ... catch to it (https://github.com/EasyCorp/EasyAdminBundle/blob/39a8cf2728cae00b86ba1c072e33f45716ca5d97/src/Twig/EasyAdminTwigExtension.php#L383-L394).

My questions:

• Is this a problem that only niche apps suffer and we should close this issue as "won't fix"?
• If you consider it a common enough problem, would be correct to return null as the logout URL when logout is not enabled?

Thanks!

[ro0NL]

I think from a vendor/generic template perspective you should assign a variable to indicate availability, i.e.; {% if logout_available %}.

In apps you know it works, or otherwise the exception is expected and helps you debug. With this change one might not even notice the logout helper is generating an empty url/path when doing href="{{ logout_path() }}".

So i think im 👎 on this.

[javiereguiluz]

@ro0NL so you would agree on changing the current Twig behaviour about variables to trigger an exception when some variable is undefined instead of silently returning null?

[ro0NL]

Yes in debug mode :)

[ro0NL]

@javiereguiluz we're already doing that; https://github.com/symfony/recipes/blob/master/symfony/twig-bundle/3.3/config/packages/twig.yaml#L4 +

symfony/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php

Line 133 in 7bbe78b

@trigger_error('Relying on the default value ("false") of the "twig.strict_variables" configuration option is deprecated since Symfony 4.1. You should use "%kernel.debug%" explicitly instead, which will be the new default in 5.0.', E_USER_DEPRECATED);

[javiereguiluz]

@ro0NL yes! I was talking about production. Right now:

• Var is undefined: dev = exception, prod = null
• Logout is undefined: dev = exception, prod = exception

This PR could be reworded as: should we make logout behave the same as vars and return null in prod?

[ro0NL]

we can debate one should always prefer strict mode, in any environment :}

For logout helper it could follow the same approach, e.g. be silent in prod. But as it's a known feature im not sure it really makes sense. It's not the point of your PR right.. which would also need silentness in dev...

[javiereguiluz]

@ro0NL actually, I don't need silentness in "dev". Developers would see the exception in "dev" and fix the problem ... while normal people in "prod" wouldn't face any issue and they just wouldn't see the "log out" link.

[ro0NL]

Developers would see the exception in "dev" and fix the problem

But then it will be fixed in prod also :)

So you imply end-users modify the vendor template (they own it), which defeats checking availability dont you think.

[ro0NL]

We could add new can_logout() twig function in core maybe? Not sure it's really worth it though 🤔 but i do like to keep the logic separate in favor of mixed api in logout_* functions (ie. string|null).

[javiereguiluz]

I wouldn't add can_logout() ... and I'd probably close this proposal as "won't fix". I think it looks too niche.

[javiereguiluz]

Let's close this as "won't fix". The proposed feature is not generic/useful enough. Thanks!