phpvms icon indicating copy to clipboard operation
phpvms copied to clipboard

Remove laravelcollective/html from frontend

Open arthurpar06 opened this issue 5 months ago • 15 comments

As you may already know, laravelcollective/html will not be compatible with Laravel 11. This PR is an attempt to start preparing for Laravel 11 by removing it from the frontend and reverting to good old pure HTML.

I have tested all the forms that this PR modifies, and I did not notice any bugs.

Note:

  • The CSRF token is not added by default when creating HTML forms, so you need to remember to add @csrf each time.
  • For the values, it's the same, you need to test the existence of the field and set the value (if you are in edit mode). To retrieve the value sent in the returned form, in case there is a validation error, you can use old('field_name').

arthurpar06 avatar Jan 19 '24 17:01 arthurpar06

Closes #1708

arthurpar06 avatar Jan 19 '24 18:01 arthurpar06

For backward compatibility, we need to keep the package in for some defined time... Like maybe with a notice and 1-2 months, because if we remove the package directly it will break up lots of custom themes and people's install will start failing over and there 😄

I can work on my theme and provide an update without its features + plain html stuff but not everyone is that quick to follow.

Just my two cents 😉

FatihKoz avatar Jan 24 '24 04:01 FatihKoz

I completely agree, that's why this PR doesn't remove it. I just don't use it in the frontend anymore. This way, when we upgrade to Laravel 11, we won't have to replace all the Form:: in the frontend, just remove the package.

arthurpar06 avatar Jan 24 '24 15:01 arthurpar06

id's should be unique (don't mind my example), so it should be used only once, preferably if needed setting it only for the checkbox is ok.

And is this the only checkbox ? It think we have another one for accepting the TOC too.

PS: Contrary to id's, same name can be used multiple times, last one will be sent along with the form.

FatihKoz avatar Feb 10 '24 11:02 FatihKoz

id's should be unique (don't mind my example), so it should be used only once, preferably if needed setting it only for the checkbox is ok.

Oh yes sorry I'll fix that.

And is this the only checkbox ? It think we have another one for accepting the TOC too.

Yes but we don't send the toc_accepted to the controller, we just want it to be checked so that the js enables the submit button

arthurpar06 avatar Feb 10 '24 16:02 arthurpar06

I suggest doing another overall check cause I saw some tiny typo errors over and there... Will try to point them out with comments.

FatihKoz avatar Feb 10 '24 20:02 FatihKoz

Generally I am wondering about select2 'cause as far as I know it does NOT support readonly attribute, thus it needs to be disabled and when a form element is disabled, its data is not sent with the request.

I had to implement some logic in my theme and modules (hidden fields and disable together).

So if select2 is still not proving readonly support, this may break some of the checks v7 default theme needs 😉

FatihKoz avatar Feb 10 '24 21:02 FatihKoz

Generally I am wondering about select2 'cause as far as I know it does NOT support readonly attribute, thus it needs to be disabled and when a form element is disabled, its data is not sent with the request.

I had to implement some logic in my theme and modules (hidden fields and disable together).

So if select2 is still not proving readonly support, this may break some of the checks v7 default theme needs 😉

Fair point, I'll try to look at it tomorrow

arthurpar06 avatar Feb 10 '24 22:02 arthurpar06

I updated all my addons, removed laravelcollective/html usage completely. So dispo side ok with v3.6.0 and up 😉

When we merge this for default theme and any admin templates, all will be ready.

FatihKoz avatar Feb 11 '24 11:02 FatihKoz

I just reviewed everything again, fixed some bugs here and there, I think there are not many left now (if there are any).

I still don't remove the package from the composer.json for the admin part. Since the upgrade to Laravel 11 won't be done before the release, I don't think we should remove this package from the admin interface because after the release we will start reviewing the filament project (which is ready) and then we will remove this package by replacing the admin interface.

arthurpar06 avatar Feb 11 '24 11:02 arthurpar06

I see you removed the readonly attributes from select2 dropdowns, are you going to disable them? Or what will be the solution for readonly fields ?

FatihKoz avatar Feb 11 '24 11:02 FatihKoz

Actually, they were unnecessary, we never enter this if statement. If you look at the code a bit more broadly, you'll see that these selects are within an if/else block, and the select is only displayed if the field is not readonly. If it's readonly, then only a label and text are displayed.

arthurpar06 avatar Feb 11 '24 11:02 arthurpar06

Ok then :)

FatihKoz avatar Feb 11 '24 12:02 FatihKoz

This one makes me nervous. Is there another form helper that can be used? And what about the admin?

nabeelio avatar Mar 05 '24 16:03 nabeelio

There is https://github.com/spatie/laravel-html which is the recommended alternative, but I don't feel like it really brings anything compared to pure HTML.

For the admin I didn't take care of it because it takes a lot of time (there are many more forms) and if we plan to migrate to Filament after the release and before upgrading to Laravel 11 I didn't see the point of doing it.

arthurpar06 avatar Mar 05 '24 17:03 arthurpar06