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

[5.7] Add Blade::helper(...) directive #24923

Closed
wants to merge 1 commit into from

Conversation

imliam
Copy link
Contributor

@imliam imliam commented Jul 21, 2018

tl;dr makes it easier to work with custom Blade directives


When creating new custom Blade directives using the Blade::directive(...) method, it seems to be rare that people actually parse the contents of the expression itself within the directive, opting instead to pass the entire expression as arguments to a helper function or method on another class.

Blade::directive('uppercase', function($expression) {
    return "<?php echo strtoupper($expression); ?>";
});

Since from what I've seen, this seems to be the most common use case, I wanted to help make these functions that little bit easier to define without the boilerplate of returning the string or having to consider what an expression may be when creating a directive.

To help with this, this PR introduces a helper method on the BladeCompiler class. This method accepts two arguments; the first is the name of the directive, and the second is the function that the directive should call.

// Define the helper directive
Blade::helper('uppercase', 'strtoupper');

// Use it in a view
@uppercase('Hello world.')

// Get the compiled result
<?php echo strtoupper('Hello world.'); ?>

// See what's echoed
"HELLO WORLD."

If no second argument is supplied, the directive name will attempt to call a function of the same name.

// Define the helper directive
Blade::helper('join');

// Use it in a view
@join('|', ['Hello', 'world'])

// Get the compiled result
<?php echo join('|', ['Hello', 'world']); ?>

// See what's echoed
"Hello|world"

The second argument can also take a callback. The advantage of a callback here over the Blade::directive(...) method is that the closure defined can have specific parameters defined instead of just the raw expression passed through. This has several good things that solve a previous idea I brought up:

  • Type hint the arguments for the callback
  • Manipulate and use the individual arguments when the directive is called, instead of the raw expression as a string
  • Define a directive without having to only use it as a proxy to a helper function or class in another part of the application
// Define the helper directive
Blade::helper('example', function($a, $b, $c = 'give', $d = 'you') {
    return "$a $b $c $d up";
});

// Use it in a view
@example('Never', 'gonna')

// Get the compiled result
<?php echo \Illuminate\Support\Facades\Blade::getHelper('example', 'Never', 'gonna'); ?>

// See what's echoed
"Never gonna give you up"

By default, all of the helper directives will echo out their contents to the view when used. This can be disabled by passing false as the third argument.

// Define the helper directive
Blade::helper('log', null, false);

// Use it in a view
@log('View loaded...')

// Get the compiled result
<?php log('View loaded...'); ?>

// Nothing is echoed

@imliam imliam changed the title Add Blade::helper(...) directive [5.6] Add Blade::helper(...) directive Jul 21, 2018
@garygreen
Copy link
Contributor

Maybe those examples aren't too good but is there a big difference between doing...

{{ strtoupper('blah') }}

vs

@upper('blah')

I personally don't think we should be encouraging adding a "wrapper" around every php function and turning it into a directive. If folks really want such functionality, why not just use a package that adds some php directives @upper @lower @array_first whatever you like. I wouldn't use it 😋

To me directives are more to do with "code structure" and not presentation (though admittedly some directives are helpful if you use them frequently e.g @csrf)

@imliam
Copy link
Contributor Author

imliam commented Jul 23, 2018

In hindsight, I see that I definitely could have emphasized what I imagine would be the main feature of this PR more.

I agree that we shouldn't be encouraging people to wrap individual functions one-for-one since there's no real benefit in doing that, but from my experience, it just seems to be the most common thing people do because it's not obvious to most people how to use that $expression passed in to the directive closure, and I feel like that alone can be improved - it's one of the more common questions I've seen people ask about directives on Laracasts/SO/etc.

So, the main thing here I imagine would be the closure that can be passed in, where the developer writing the directive doesn't have to care about the context of the expression or its contents, and can just treat it like they're writing a regular function.

Admittedly, these helper directives wouldn't really have any advantage in "code structure" and would only be really useful for wrapping presentation stuff, but I think there's still enough usage people would get out of that to justify it in core.


Take this as a mundane example, as a wrapper for the logic around a FontAwesome 4 icon. There's a bit of boilerplate around it you probably don't want to remember to write every time, and you wouldn't want to @include it every time. While it could be a regular function and operate just the same, this is one of those things you'd probably want that bit of syntactic sugar around because of how often it can be used - nor would there be any advantage of it being a regular function or helper as you'd never get any use of it outside views.

Blade::helper('fa', function(string $iconName, string $text = null, $classes = '') {
    if (is_array($classes)) {
        $classes = join(' ', $classes);
    }

    $text = $text ?? $iconName;

    return "<i class='fa fa-{$iconName} {$classes}' aria-hidden='true' title='{$text}'></i><span class='sr-only'>{$text}</span>";
});

@shalvah
Copy link
Contributor

shalvah commented Aug 9, 2018

@imliam Small correction for your latest example. You want:

    $text = $text ?: $iconName;

and not:

    $text = $text ?? $iconName;

😀

Also, your PR seems to be failing some style checks.

@jmarcher
Copy link
Contributor

jmarcher commented Aug 9, 2018

@shalvah They do almost the same $text ?? $iconName is somewhat safer because it does not throw a Notice in case $text does not exist.

@garygreen
Copy link
Contributor

I'm still strongly opposed to having this merged in core.

The word "helper" is templating languages generally means adding a function of some kind so you can use it to output/echo stuff into the view. That's fine, this is exactly what this PR is doing. Except in all the templating languages that implement such functionality it's because they are sandboxed.

For example, in Twig, you cannot simply do:

{{  some_helper() }}

Even if some_helper was defined globally. However, Blade has always taken a difference approach and never sandboxed you in. You can just as easily do {{ some_helper() }} if it was defined globally.

This PR has piggy-backed echo helpers onto blade directives which is simply not needed, and goes against the general convention of using directives as code structure and not echoing (minus a few exceptions).

As I said, this can easily be added as a package. I'm yet to be convinced it would even be widely used.

@GrahamCampbell GrahamCampbell changed the title [5.6] Add Blade::helper(...) directive [5.7] Add Blade::helper(...) directive Sep 3, 2018
@GrahamCampbell GrahamCampbell changed the base branch from 5.6 to 5.7 September 3, 2018 09:40
@taylorotwell
Copy link
Member

No plans on adding this right now.

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.

5 participants