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

Blade::if directive with an argument of 0 is not compiled correctly #24390

Closed
tjallingt opened this issue May 31, 2018 · 8 comments
Closed

Blade::if directive with an argument of 0 is not compiled correctly #24390

tjallingt opened this issue May 31, 2018 · 8 comments

Comments

@tjallingt
Copy link
Contributor

  • Laravel Version: 5.6.23
  • PHP Version: 7.2.3
  • Database Driver & Version: mysql 5.7

Description:

Blade::if('iseven', function ($number) {
    return $number % 2 == 0;
});

Will cause errors when used with 0 (@iseven(0)).

At: https://github.com/laravel/framework/blob/5.6/src/Illuminate/View/Compilers/BladeCompiler.php#L394-L413

The compiler checks if the directive received any arguments and if it didn't it will output a check without an argument. However since 0 is falsy you can't pass 0 into a custom blade conditional.

Steps To Reproduce:

Add the iseven blade conditional to your app service provider and add

@iseven(0)
test
@endiseven

in a blade template.

expected result would be the test but it will throw errors about expecting an argument that it didn't get.

@staudenmeir
Copy link
Contributor

As a workaround you can use a string:

@iseven('0')
test
@endiseven

@tjallingt
Copy link
Contributor Author

tjallingt commented May 31, 2018

@staudenmeir
Yes, thanks for mentioning that. I still think it is kind of strange behaviour that should be fixed even if there is an easy workaround. Interestingly this doesn't seem to happen with @iseven(false). I'm assuming the blade compiler is just parsing everything as text but while "0" is falsy "false" is not... and the workaround is based on the idea that "'0'" is not falsy either

@tjallingt
Copy link
Contributor Author

tjallingt commented May 31, 2018

that means the fix would look like

$expression || $expression === "0"
    ? /* with argument */
    : /* without argument */;

which is kinda ugly but behaves a lot more like one would expect it to behave

@staudenmeir
Copy link
Contributor

staudenmeir commented May 31, 2018

I would suggest a stricter comparison: $expression !== ''

@tjallingt
Copy link
Contributor Author

tjallingt commented May 31, 2018

@staudenmeir yea that is a much cleaner way to write the same thing 👍
Should i create a pull request updating the ternaries?
If so, is it a major or minor change?

@staudenmeir
Copy link
Contributor

Out of curiosity: Why are you using the directive with a fixed value?

@tjallingt
Copy link
Contributor Author

tjallingt commented May 31, 2018

Showing and hiding parts of blades depending on the "organizational level" of a user.
So although the value passed to the directive is fixed the directive compares that to a value that calculate elsewhere (so its just a convenience for writing out the whole check)

We could use names (strings) instead of numbers for the organizational levels but we tend to call them by their numbers (mostly because the names can be confusing).

Alternatively we could write different directives per level @level0, @level1 etc but that seems pretty verbose when I could just pass the level as a number...

@staudenmeir
Copy link
Contributor

I just saw your edit: Yes, create a pull request and add a test. Since it's a bug fix, you should target the 5.6 branch.

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

2 participants