roots / acorn

Laravel components for WordPress plugins and themes
https://roots.io/acorn/
MIT License
825 stars 94 forks source link

Help with PR approach for component validation #302

Closed eavonius closed 8 months ago

eavonius commented 1 year ago

Summary

Background: I wanted to use laravel validation in blade components within an acorn project (a sage 10 theme to be specific). I read a bunch of the acorn and laravel 9 source and got it working. However, I'm not sure the best way to contribute this back to acorn, since it looks like people were working on validation already and I don't want to disrupt that if my approach conflicts at all. So I'm Looking for some help with how I should approach potentially contributing this, or helping validation move along!

First I found the file src/Illuminate/Foundation/Validation/ValidationRequests.php in the acorn source. This is a PHP trait that can be added to classes in an acorn project that are instantiated with access to the dependency injection container. Composers and Components in a sage 10 theme fit this bill.

Next I added this trait by importing it into a Component class with use Illuminate\Foundation\Validation\ValidatesRequests then adding a call to use ValidatesRequests; before the constructor of the class.

Finally I added some code to the constructor of the component class to test it:

public function __construct(Request $request) {

  if ($request->isMethod('post')) {

    try {
      $this->validate($request, [
        'name' => 'required|min:2'
      ]);
    } catch (\Exception $e) {
      error_log("JSON message dump of validation errors: " . $e->validator->messages());
    }
  }
}

When I submit a form on the component that POSTs an input field named "name", and set that field to 1 character long before posting (so the validation fails), the validation exception is thrown but the JSON representation of the message just had the path within an expected localization file to get the actual message to display. E.g:

{"name":["validation.min.string"]}

After a bunch of digging around, I found this post on StackOverflow that showed in the more recent versions of laravel, the trans() function must be passed to the constructor of the ValidationFactory when it is instantiated. Additionally, the validation.php file from laravel must be present at the default path (relative to your theme/project directory) of ./lang/en/validation.php.

So I first copied the latest commit from laravel's source for version 9.x of validation.php to that path in my project.

Next I found that on line 98 of acorn's ValidationRequest.php trait file is where the ValidationFactory is retrieved from the container. I just added a second parameter to the app function, to pass the result of the trans function as described in the StackOverflow post (since this seemed to take additional parameters to pass to the constructor):

return app(Factory::class, [trans()]);

After submitting the form. The messages come back localized!

{"name":["The category name must be at least 2 characters."]}

So it looks like that class would need to be modified in acorn. But it also seems like we would want to include instructions in acorn (and possibly update the sage 10 new project template files) to include laravel 9's default validation localization files.

Anyway, happy to answer any other questions if I'm headed in the right direction with this. If not, let me know what other approach I should be taking. Thanks!

Additional context

No response

Log1x commented 1 year ago

I haven't messed with validation but in theory it should work depending on how you're handling the request. Have you tried using the Validator Facade instead of initializing Validation directly? I assume that should work too, although something might still have to be done to register the translations for default messages.

use Illuminate\Support\Facades\Validator;

Validator::validate($data, $rules, $optionalMessages);

Don't shy away from doing a PR, even if there is stuff in progress it can always get cherry-picked.

Also, so you don't get confused, anything that lives in src/Illuminate/Foundation within the Acorn repo is straight Laravel and is pulled down/synced as-is from the Laravel framework repo.

All code changes should happen in src/Roots/Acorn.

eavonius commented 1 year ago

Interesting. I tried using the Validator facade like that first, I kept getting errors that no object named "validator" was registered in the container. It seems the factory needs to already be registered with the container for that to work.

I also forgot to mention, I had to add a reference to two other laravel packages in composer for this to work:

"illuminate/validation": "v9.52.14", "illuminate/translation": "v9.52.14"

eavonius commented 1 year ago

Thanks for the info about changes only being in acorn - this is easier actually than I thought.

All that's needed is to add those two references to composer and to add the localization file. The code change to pass the trans() function isn't needed. I just happened to do it at the same time and didn't realize it was unecessary.

Log1x commented 1 year ago

Strange, it looks like the validation service provider is supposed to register the factory with the container.

Make sure you sanity run wp acorn optimize:clear after installing illuminate/validation and illuminate/translation to ensure the provider is getting loaded. Otherwise, might have to figure out what else needs registered.

eavonius commented 1 year ago

Yeah I saw that code too. It doesn't seem to be working properly for whatever reason. I did run optimize:clear, not sure what that's supposed to do in this case, but it seems to behave the same.

Log1x commented 1 year ago

I did run optimize:clear, not sure what that's supposed to do in this case

It was just to make sure the providers weren't cached causing the new packages to not get loaded.

It's possible it is getting caught up when getting the translator during registration – will require some tinkering.

eavonius commented 1 year ago

No problem, thanks for helping out with this. It's working great actually now that I have the translation in the right place and those two references. I'll be sure to get rid of those if you are able to figure out what's going on there. For now using the trait works great. No rush.

Log1x commented 8 months ago

This should now work out of the box in v4 with the routing updates.