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.4] Allow the first validation error to be found on wildcard lookups #15217

Merged
merged 1 commit into from Sep 2, 2016
Merged

[5.4] Allow the first validation error to be found on wildcard lookups #15217

merged 1 commit into from Sep 2, 2016

Conversation

deefour
Copy link
Contributor

@deefour deefour commented Sep 1, 2016

With validation rules like

'photos.image' => 'required|image',
'photos.caption' => 'required|min:20',

If an image and no caption are provided and we ask the messagebag if photos.* has any errors

$errors->has('photos.*');

an \ErrorException occurs:

Undefined offset: 0

because the structure of the errors array in this situation is like

[
    'photos.caption' => [
        'The photos.caption field is required'
    ]
]

This PR flattens and filters empty validation messages regardless of depth, returning the first non-empty message if present.

@@ -2585,6 +2585,7 @@ public function testValidateImplicitEachWithAsterisksConfirmed()
$this->assertFalse($v->passes());
$this->assertTrue($v->messages()->has('foo.0.password'));
$this->assertTrue($v->messages()->has('foo.1.password'));
$this->assertTrue($v->messages()->has('foo.*.password'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test not related to the change?

Copy link
Contributor Author

@deefour deefour Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's related. The above test causes the \ErrorException without this PR. Can you suggest a better place for it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support/SupportMessageBagTest.php

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@themsaid thank you 🍻

@taylorotwell
Copy link
Member

@themsaid what do you think about this?

@deefour
Copy link
Contributor Author

deefour commented Sep 2, 2016

If a use case matters, I have a series of fieldsets containing multiple fields. I want to mark a <fieldset> with an error class if any of the fields within the fieldset have an error.

@foreach ($photos as $photo)
    <fieldset class="{{ $errors->has("photos.{$loop->index}.*") ? 'error' : '' }}">
        ...

@GrahamCampbell GrahamCampbell changed the title Allow the first validation error to be found on wildcard lookups [5.4] Allow the first validation error to be found on wildcard lookups Sep 2, 2016
@themsaid
Copy link
Member

themsaid commented Sep 2, 2016

I think it's useful, but don't think it's breaking a breaking change.

@taylorotwell
Copy link
Member

You don't think it's a breaking change or you do? :)

@GrahamCampbell
Copy link
Member

don't think it's breaking a breaking change.

So cryptic. :trollface:

@themsaid
Copy link
Member

themsaid commented Sep 2, 2016

haha I donno how I wrote that :D

It's not a breaking change.

@taylorotwell
Copy link
Member

Well the "but" threw me off, haha. Was expecting an "and" :)

@themsaid
Copy link
Member

themsaid commented Sep 2, 2016

The entire phrase doesn't make much sense :D.

Friday morning here is your Saturday morning, maybe that explains it :D

@taylorotwell taylorotwell merged commit a306440 into laravel:master Sep 2, 2016
@deefour deefour deleted the bugfix/wildcard-messagebag-check branch September 2, 2016 14:05
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.

None yet

4 participants