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.8] Add @error blade directive #28062

Merged
merged 2 commits into from Apr 17, 2019

Conversation

Projects
None yet
@calebporzio
Copy link
Contributor

commented Mar 29, 2019

This PR comes out of a popular tweet I wrote: https://twitter.com/calebporzio/status/1111366190444802048

// Before
@if ($errors->has('email'))
    <span>{{ $errors->first('email') }}</span>
@endif
  
// After:
@error('email')
    <span>{{ $message }}</span>
@enderror
@driesvints

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Pretty neat!

@browner12

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

minor suggestion, switch to CompilesErrors (plural) so it stays consistent with other traits?

@driesvints

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

@calebporzio I think the suggestion by @browner12 is valid. Can you update the PR?

@devcircus

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Sounds like a good start to adding blade helpers to your awesome-helpers package, or an awesome-blade-helpers package.

@rellect

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Problem with such helpers is that they are not flexible, they handle a very specific case. For example, you can't check if error does not exists, so in this case and many others you'll have to fallback to a regular if statement. Personally I try to keep my code consistent so I avoid using such helpers.

@calebporzio

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Updated - thanks!

@mo7amed-3bdalla7

This comment has been minimized.

Copy link

commented Mar 30, 2019

I made a PR before like this and closed #27944

@driesvints

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

@mo7amed-3bdalla7 this is a different implementation.

@garygreen

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

Problem I have with stuff like this is although on the face of it it looks nice - your still find yourself reaching for $errors->has('email') - often you want to add a "has-error" class to the container element, plus render a message:

<div class="form-group {{ $errors->has('email') ? 'has-error' : '' }}">
	<input type="email">
	@error('email')
		<span class="help-block">{{ $message }}</span>
	@enderror
</div>

...developers may get confused (especially newcomers) as to why they need to use both $errors and @error in their view.

Rendering errors is always something I've found a bit clumsy, so in previous projects I've experimented using a "wrapper" around the container for the field, something along these lines:

@field('email', ['class' => 'flex']) <!-- <div class="flex form-group has-error"> -->
	<input type="email">
	{{ $error }} <!-- <span class="help-block">...</span> -->
@endField

...but even that style I've started to go off because, although it reduces amount of code, it's a bit cryptic in what it's actually going to render. It also creates a disparity between how you render errors in js e.g. Vue/React, to how you do it in blade. Purely for explicitness, I've started just writing it out in full, that way developers know exactly what it's going to render, how to render errors, why it's rendering that way etc.

In summary, I've still not come up with a solution I've personally been that comfortable using in my apps, but this PR looks interesting as a way of reducing the amount of code for the "inner" message problem but doesn't solve the "outer" problem, if that makes sense. I still think there are better alternatives, so I personally would say for this reason it's maybe something best left in userland.

@calebporzio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Makes sense, what about this:

<div class="form-group @error('email', 'has-error')">
	<input type="email">
	@error('email')
		<span class="help-block">{{ $message }}</span>
	@enderror
</div>

I kinda dig that. There could be a third parameter default too: <input class="@error('email', 'border-red', 'border-gray')">

I get just wanting to keep the original $errors, but damn it's a lot of ugly code for such a common use case. If it doesn't feel ugly to you, look no further than the likes and comments on the tweet.

@m1guelpf

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

For reference: Here's my #noplanstomerge PR to 5.4 #20111

@calebporzio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

Ok, just actually needed to set an "input error" state on an input element. I much prefer this syntax to the standard blade echo ternary:

Before:
image

After:
image

So even if we don't add those additional params, the original PR is still useful in both cases.

@ankurk91

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

Does it handle named error bags? Something like?

Blade::directive('isInvalid', function ($expression) {
            $segments = explode(',', $expression);
            $input = trim($segments[0]);
            $errorBag = trim(Arr::get($segments, 1, 'default'), "'");

            return '<?php echo $errors->'.$errorBag.'->has('.$input.') ? "is-invalid" : ""; ?>';
        });

Why not two directives?

// Bootstrap css classes
Blade::directive('validationMessage', function ($expression) {
            $segments = explode(',', $expression);
            $input = trim($segments[0]);
            $errorBag = trim(Arr::get($segments, 1, 'default'), "'");

            return '<?php if ($errors->'.$errorBag.'->has('.$input.')) { ?>
                      <div class="invalid-feedback d-block" role="alert">
                        <?php echo e($errors->'.$errorBag.'->first('.$input.')); ?>
                      </div>
                  <?php } ?>';
        });
@calebporzio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@ankurk91 - the goal of this feature is to cover (what I believe to be) the 90% of use cases for $errors. I think named error bags and other features shouldn't be handled by it. It's simplicity will start to erode.

@garygreen

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I'm still not convinced this is something that should be added to core. There are so many opinionated ways you could do error handling in views, getting involved in this logic is probably a recipe for disaster down the line.

Style 1 - render second parameter if contains an error

<div class="form-group @error('email', 'has-error')">
	<input type="email">
	@error('email')
		<span class="help-block">{{ $message }}</span>
	@enderror
</div>

Style 2 - allow html as second parameter, to format error

<div class="form-group @error('email', 'has-error')">
	<input type="email">
	@error('email', '<span class="help-block">:message</span>')
</div>

Style 3 - using a custom helpers to return class/html formatted errors:

<div class="form-group @errorClass('email')">
	<input type="email">
	@errorHtml('email') <!-- outputs <span class="help-block">.... if there is an error !-->
</div>

Style 4 - using short Emmet-style syntax to reduce amount of typing of second parameter

<div class="form-group @error('email', 'has-error')">
	<input type="email">
	@error('email', 'span.help-block') <!-- will be auto-closed and contain :message -->
</div>

Style 5 - for multiple error bags.

I give up.

All of the above to avoid:

Current:

<div class="form-group {{ $errors->has('email') ? 'has-error' : '' }}">
	<input type="email">
	@if ($errors->has('email'))
		<span class="help-block">{{ $errors->first('email') }}</span>
	@endif
</div>

Or:

<div class="form-group {{ $errors->has('email') ? 'has-error' : '' }}">
	<input type="email">
	{!! $errors->first('email', '<span class="help-block">:message</span>') !!}
</div>
@calebporzio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

"All of the above to avoid" - I don't think the mentioned "above" is a valid problem statement. I never proposed offering Emmet style syntax.

Here is what I'm proposing:

@error('first_name')
    <span>{{ $message }}</span>
@enderror

to avoid having to write

@if ($errors->has('first_name'))
    <span>{{ $errors->first('first_name') }}</span>
@endif

This will cover most use cases. In the case of needing an inline style, you can do this:

<input name="first_name" class="@error('first_name') has-error @enderror">

@error('first_name)
    {{ $message }}
@enderror

I think this is cleaner. I think this is prettier. A bunch of other people think so too.

@efrain-salas

This comment has been minimized.

Copy link

commented Apr 9, 2019

protected function compileEnderror($expression)
{
return '<?php unset($message);
if (isset($messageCache)) { $message = $messageCache; }

This comment has been minimized.

Copy link
@d3jn

d3jn Apr 9, 2019

Contributor

Correct me if I'm wrong (didn't test it), but it seems this can produce side-effects if we include @error directive inside another one - original $message value will not be preserved:

$message = 'i use message in view';
...
@error('email')
    ...
    @error('phone')
        ...
    @enderror
@enderror
...
{{-- $message now contains 'email' rule message instead of original string value --}}

This comment has been minimized.

Copy link
@calebporzio

calebporzio Apr 9, 2019

Author Contributor

I believe you're wrong. Should return to the previous $message if one existed.

This comment has been minimized.

Copy link
@d3jn

d3jn Apr 9, 2019

Contributor

I believe you're wrong. Should return to the previous $message if one existed.

Believe is a strong word. Here is what I see:

  1. Initial state:
    $message: 'i use message in view', $messageCache: <unset>'

  2. First @error call:
    $message: 'email rule message', $messageCache: 'i use message in view'

  3. Second @error call:
    $message: 'phone rule message', $messageCache: 'email rule message'

  4. First @enderror call:
    $message: 'email rule message', $messageCache: 'email rule message'

  5. Second @enderror call (and our end state):
    $message: 'email rule message', $messageCache: 'email rule message'

$message looses it's original value. Where am I wrong on this?

This comment has been minimized.

Copy link
@garygreen

garygreen Apr 9, 2019

Contributor

@d3jn You wouldn't want to nest @error blocks with this PR, or forget to @enderror because you'll get side effects, as you mention. It's one of the downsides to this implementation. If used once, you can still use $message in your views, but not $messageCache as that is used internally to remember the state of $message.

This comment has been minimized.

Copy link
@d3jn

d3jn Apr 9, 2019

Contributor

@garygreen I can definitely see a need to do nested checks (when certain fields are paired or have hierarchical relationship), so I just pointed out that this implementation will produce a side-effects in such situations. It definitely diminishes it's value if you need to actually take into account specifics of it's implementation when using it.

This type of side-effect can be resolved by using a stack (array) of cached $message values and putting/popping values in/out of it. This will allow for a nesting support.

This comment has been minimized.

Copy link
@garygreen

garygreen Apr 9, 2019

Contributor

Why would you need to nest? You can use dot format to render deeply nested error messages.

This comment has been minimized.

Copy link
@calebporzio

calebporzio Apr 10, 2019

Author Contributor

Sorry, I said "believe" not to be confrontational, but to express I don't actually know for sure. I meant "believe" to be a loose word, not strong lol.

I'm pretty indifferent on supporting nesting because I can't really see myself using it. But I agree it seems like the behavior is unexpected if what you're saying is true.

Can you provide the solution?

This comment has been minimized.

Copy link
@d3jn

d3jn Apr 11, 2019

Contributor

Can you provide the solution?

Just utilize array_pop/array_push and make $messageCache into array instead. This will look something like this:

    // For opening directive
    return '<?php if ($errors->has('.$expression.')) :
        if (! isset($messageCache)) { $messageCache = array(); }
        array_push($messageCache, $message);
        $message = $errors->first('.$expression.'); ?>';
    
    ...
    
    // For closing one
    return '<?php unset($message);
        if (! empty($messageCache)) { $message = array_pop($messageCache); }
        endif; ?>';

But make sure to cover it with separate test if you do decide to get rid of this problem and introduce nesting support without potential side-effects.

@calebporzio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Yeah, Michael's example is nice. However, I am not a fan of having to use {!! !!} and passing HTML into a string losing syntax highlighting and autocompletion and such.

@garygreen

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

I think this is cleaner. I think this is prettier. A bunch of other people think so too.

I'm not disagreeing behind the intentions of this PR - reducing the amount of code to write is always a good developer experience, but I personally think we should be careful adding new blade stuff like this without properly understanding why it's needed.

This PR introduces side effects, by which I mean: what happens in the current PR you @error() into multiple blocks? What if you forget to @enderror? Can I still use $message and $messageCache in my views? What about current Laravel tutorial/guides that show all about $errors and now there's a new way to do it with @error, would that be helpful or confusing to newcomers?

Most will solve the problem in userland by writing their own helpers which can fully format the error message to whatever framework format they are using without side effects, or use IDE helpers to write their own keyboard shortcut snippets.

For example, you can write some very basic directives to do this kind of format, which IMO is even simpler and more declarative in usage than this PR.

<div class="form-group @hasError('email')"> <!-- class="form-group has-error" -->
   <input type="email" class="form-control">
   @error('email') <!-- <span class="help-block">:message</span> or NULL -->
</div>
\Blade::directive('hasError', function($expression) {
   return "<?= \$errors->has($expression) ? 'has-error' : '' ?>";
});

\Blade::directive('error', function($expression) {
   return "<?= \$errors->first($expression, '<span class=\"help-block\">:message</span>') ?>";
});

Again, I'm not totally against this PR, but I just question it's implementation and usefulness in real world applications when there are alternatives in userland.

@calebporzio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Agreed that this can be accomplished debatably better in userland, but that goes for most syntax like this.

I would say that this is such a rediculously common use case, it makes sense to make it nicer. It's like @csrf or @old. They provide pleasant syntax for common use cases.

Good point re onboarding newcomers. I think this could be the primary way of documenting and then, an example/note using the more verbose $errors to show it's available. I think this could be the idiomatic way to access validation errors and it would make onboarding even easier.

I believe it's useful for most applications using Blade.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Maybe a paranoid concern but should we be concerned about merging this into 5.8 and stomping on someone's "macros"? Do macros take precedence over Blade defined directives?

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@taylorotwell I don't believe we need to take into account whatever anyone has added in user land. If that was the case than you couldn't add anymore new methods to classes etc because you'd "risk" it that someone else would have implemented that method. New functionality which doesn't breaks the public api should always be ok.

@rellect

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

@driesvints I believe the concern is if to merge into 5.8 or master

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@rellect yes, that's what I was addressing.

@devcircus

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Usually I'd agree that we shouldn't waste too much time worrying about userland conflicts with custom macros/helpers, however I believe "error" is so common that we should consider sending to master and noting it in the upgrade guide. I have to assume there are many apps using an "error" helper. I don't think this is such a pressing matter that we should risk breaking these apps on a point release.

I'm not even sure which would take precedence, so it may not be an issue.

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@devcircus yeah, that's a good point actually.

@timacdonald

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Just confirming that a userland directive takes precedence over a core directive.

public function boot()
{
    Blade::directive('php', function ($expression) {
        return "<script>alert('You wanted PHP. You got JS. You\'re welcome.');</script>";
    });

    Blade::directive('csrf', function ($expression) {
        return "<script>alert('CSRF');</script>";
    });
}
<!-- welcome.blade.php -->
@php
@csrf

Screen Shot 2019-04-16 at 1 33 39 pm

Screen Shot 2019-04-16 at 1 33 41 pm

@devcircus

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

If that's the case then I'm 👍🏻

@taylorotwell taylorotwell merged commit 41e158f into laravel:5.8 Apr 17, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@devcircus

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

🎉🍾🍻🎉

kennith added a commit to kennith/laravel-blade that referenced this pull request May 15, 2019

Add @error directive
@error and @enderror was added in Laravel 5.8.13

See laravel/framework#28062

@kennith kennith referenced this pull request May 15, 2019

Merged

Add @error directive #180

@jsiebach

This comment has been minimized.

Copy link

commented May 22, 2019

This broke our app in a few places! We have a lot of vue directives that use @error, ie.

<vue-table @error="isError = true">

I have just changed that to

<vue-table {{'@'}}error="isError = true">

For now... Is there a better way to escape the @ symbol?

@timacdonald

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

The quickest fix would probably be to just declare a custom directive to restore your original behaviour.

public function boot()
{
    Blade::directive('error', function ($expression) {
        return '@error';
    });
}
@jsiebach

This comment has been minimized.

Copy link

commented May 22, 2019

@timacdonald thanks. I found something saying @@error would work to escape it, and it looks like tests are passing.

I would think this would be a breaking change given how many Laravel devs must use @error in Vue... but for now I'm going to implement the @@

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.