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

Errors not showing in child form fields #78

Closed
theshaunwalker opened this issue Apr 18, 2015 · 17 comments · Fixed by #79
Closed

Errors not showing in child form fields #78

theshaunwalker opened this issue Apr 18, 2015 · 17 comments · Fixed by #79

Comments

@theshaunwalker
Copy link
Contributor

It looks like the real_name is getting set to the individual field's name, when in the HTML the name includes the name of the child form too because its in array format.

Testing out which version of this breaks it as it seems to have broken since I composer updated it from being on an old version.

Update:

Yeah the issue is with

$errors->get(array_get($options, 'real_name', $name))

which was brought in a commit about "fixing errors in child forms" (ironically lol)

In my case, I have a child form under 'invoice' with a text field 'title'. This is keyed in laravel's error bag as 'invoice.title'. The 'real_name' of the field is 'title' and the $name equals 'invoice[title]'.

Is this working for you?

Basically the field needs a dot notation name including the names of its parent form(s) to get the error from $errors.

@theshaunwalker
Copy link
Contributor Author

Because getName() is used to get the HTML friendly name refactoring around that will be a big hassle, my "hackish" solution:


    public function getFullName()
    {
        $stripLeft = str_replace("[", ".", $this->getName() );
        $stripped = str_replace("]", "", $stripLeft);
        return $stripped;
    }

Add this function to FormField and pass it to form templates (I'm using fullName as the variable atm) and use that for error checking. This basically replaces the [ ]'s with periods.

So things like invoice[title] become invoice.title and should work for any level of children as it just works off the name generated anyway.

@kristijanhusak
Copy link
Owner

How do you set up validation rules? It works fine when you set rules with real name, for example:

$rules = [
'title' => 'required'
];

@theshaunwalker
Copy link
Contributor Author

That is pretty limiting when it comes to different child forms having the same field name. And just generally feels wrong considering it both goes against how Laravel handles the naming and just in general you wouldnt refer to a field in a form array as just that field name would you?

@theshaunwalker
Copy link
Contributor Author

Also this just breaks Laravel validation of child forms as the validation checks child fields via dot notation, so theres no way a field on a child form can not have a dot notation name.

I'm curious what setup this is working for you with?

@kristijanhusak
Copy link
Owner

Yes, agree it's limiting. Definitely needs to be fixed. Validation checks by the field name, so for example if invoice[title] is used in the form, then in rules should also be set like this:

$rules = [
    'invoice[title]' => 'required'
];

In package i do not use dot notation for that kind of stuff, because it's more understandable for all developers with [] syntax.

Dot notation is just a convention used by Laravel, but in the background it translates to the name[field] format.

@theshaunwalker
Copy link
Contributor Author

Oh I didnt know validation keys could be set like invoice[title].

I just changed my keys to that format and it is at least working for the wrapper attributes when there is an error but doesnt show the error message below the field (as the error message is trying to grab title not invoice[title]) so theres some disparity there.

I feel the commit about fixing child form error displaying ironically broke child form error displaying :P

@theshaunwalker
Copy link
Contributor Author

array_get($options, 'real_name', $name)

in the views needs to just go back to

$name

for this to work for me

@kristijanhusak
Copy link
Owner

Yeah someone else reported that it's not working with regular names, like i put in example. This needs fix. Will do it soon, thanks.

@theshaunwalker
Copy link
Contributor Author

I'm going to be updating all the views so that it does work for me so I'll submit a PR with the view changes.

@kristijanhusak
Copy link
Owner

That would be great, thank you.

@theshaunwalker
Copy link
Contributor Author

You said naming fields like invoice[title] worked in Laravel validation for you? it doesnt seem to be for me...

@kristijanhusak
Copy link
Owner

Yes, i just tested it out again. Something like this:

<?php

$rules = [
    'invoice[title]' => 'required'
];

class MainForm extends Form {

    public function buildForm()
    {
         $this->add('invoice', 'form', [
             'class' => 'App\Forms\InvoiceForm'
         ]);
    }
}

class InvoiceForm extends Form {

     public function buildForm()
     {
          $this->add('title');
     }
}

@theshaunwalker
Copy link
Contributor Author

hmmm I cant use keys like that. Validation just doesnt get touched. i.e. required rule has errors even when value provided.

And you're using purely Laravel validation in your domain?

@kristijanhusak
Copy link
Owner

Yes, you're right, i tested only to see if there are any errors, but not if passes. Will look into it, thanks.

@theshaunwalker
Copy link
Contributor Author

Do you even use this package yourself anymore? haha

I'm making changes so that it solves the issue for me, I'll put in another PR and you can look at it.

@kristijanhusak
Copy link
Owner

I didn't have need for nested forms, done only simple apps with simple forms. Great, thanks.

@theshaunwalker
Copy link
Contributor Author

Closing this as new PR makes it redundant

kristijanhusak referenced this issue Apr 19, 2015
update views to use correct name
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 a pull request may close this issue.

2 participants