search

kristijanhusak / laravel-form-builder

Laravel Form builder for version 5+!
https://packagist.org/packages/kris/laravel-form-builder
MIT License
1.7k stars 298 forks source link

Fix deprecation message if str_replace receive a null value in FormHelper #689

Closed ajira86 closed 2 years ago

ajira86 commented 2 years ago

str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated

Today str_replace is returning a blank string if a null value is specified as subject

rudiedirkx commented 2 years ago

But how/why/where is transformToDotSyntax() ever called with null?

ajira86 commented 2 years ago

But how/why/where is transformToDotSyntax() ever called with null?

I'll check what happens, I'm curious too ^^

ajira86 commented 2 years ago

So, it happens because getValidationRules is calling getNameKey from the Form.

In my case, my forms are not named, this is why a null value is passed to transformToDotSyntax

rudiedirkx commented 2 years ago

In that case Form::getNameKey() should probably check for name nullness. IMO transformToDotSyntax shouldn't accept null, but we can't change that now, so adding a null-check is still smart.

But why doesn't any test break on this? We have PHP 8.1 tests... With nameless forms. Maybe deprecateds don't fail tests, hmm...

rudiedirkx commented 2 years ago

I've fixed the test suite. Deprecations are now errors. And I left another one for you ;)

(I merged that into your branch, so you'll have to pull your refactor branch.)

ajira86 commented 2 years ago

Sorry for the delay I didn't had the time to check it during one week. Thank you for the update and the merge ^^

rudiedirkx commented 2 years ago

Thank you for the bugfind, and the realization that deprecations were ignored in tests. You're testing the form builder well in PHP 8.1? I'm not at 8.1 yet.

ajira86 commented 2 years ago

Happy to hear that I can help even with little contribution.

You're testing the form builder well in PHP 8.1? I'm not at 8.1 yet.

Yes I'm working on a PHP 8.1 project because Ubuntu 22.04 provide this version by default.

ajira86 commented 2 years ago

Is it possible to publish a version with this fix ?

rudiedirkx commented 2 years ago

Done. You should probably just use dev-master, because we're bad at releases, and you have to keep an eye on changes anyway.