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

Added Laravel 5.6 stable support #484

Merged
merged 3 commits into from
Feb 8, 2018
Merged

Added Laravel 5.6 stable support #484

merged 3 commits into from
Feb 8, 2018

Conversation

mxmtsk
Copy link
Contributor

@mxmtsk mxmtsk commented Feb 8, 2018

Support for 5.6, tests do pass.
While checking the tests I found a bug, where the $optGroupAttributes wheren't added to the optgroup but where added to the child options, instead of their respective attributes. (The bug came up with the changes made in #470)

I'd like to mention that I opted out double encoding (it is now default in the e helper function in 5.6, so that this package works like before. If this package should, from now on, use double encoding by default I'm happy to revert the changes and fix the tests instead.

@tshafer
Copy link
Contributor

tshafer commented Feb 8, 2018

Thanks! I'll get this merged and tagged today.

@Swanty
Copy link

Swanty commented Feb 8, 2018

Double encoding is a default in PHP itself.
Laravel since beginning chose to override the PHP security default and set it to false.
I would heavily recommend not changing double encode back to false.

If you choose to opt out of double encoding then imagine scenario like this:

Lets say you put the code below in input field and submit the form

<p>&lt;div&gt;&lt;/div&gt;</p>

once the form is submitted and you are returned back to the same input field you will NOT see the same thing you put there, but you will see this instead

<p><div></div></p>

Do you see the reason why double encoding is True by default in PHP? =)

@mxmtsk
Copy link
Contributor Author

mxmtsk commented Feb 8, 2018

I do agree @Swanty.
Just wanted to keep migration issues as low as possible. What do you think @tshafer?

@tshafer
Copy link
Contributor

tshafer commented Feb 8, 2018

We could always add a option to let the user enable and disable.

Something like adding this to the app service provider.

Form::doubleEncode();

or

Form::textarea($name, $value, $options)->doubleEncode();

Form::textarea($name, $value, $options)->disableDoubleEncode();

@Swanty
Copy link

Swanty commented Feb 8, 2018

If it's possible to somehow get the value from
https://github.com/laravel/framework/blob/b6f7cfbc1efcc3866eef336b6cb4f9ac1b221c32/src/Illuminate/View/Compilers/BladeCompiler.php#L91
then you don't have to force a specific value for double encode.

Those who want double encoding can do so in the laravel framework itself and those who don't want can also do the same now.

Those who upgrade to 5.6 will have to go through upgrade guide either way and decide whether they wish to leave double encoding disabled or go with the defaults in Laravel and PHP.
I'm guessing developers by default would also assume that laravelcollective/html will be following the same standards as Laravel blade.

What do you think? =)

@khaledelmahdi
Copy link

Could you update the version support for now? ☺️

New features can take time to discuss and implement.

@tshafer
Copy link
Contributor

tshafer commented Feb 8, 2018

Yea, lets get this out and come up with something.

@tshafer tshafer merged commit e649632 into LaravelCollective:master Feb 8, 2018
@Swanty
Copy link

Swanty commented Feb 8, 2018

@tshafer

Laravel since version 5.5 comes with new option \Blade::doubleEncode() that you can put in a service provider.
Now with Laravel 5.6 it has upgrade to be even better and it's now \Blade::withoutDoubleEncoding() and \Blade::withDoubleEncoding()

I think laravelcollective/html should just encode based on that and not create extra functionality that might just complicate and/or confuse developers in the long run.

Though one thing I might add is that conditional override for input fields like you mentioned "might" be a feature request in future that someone might ask.
I'm talking about Form::textarea($name, $value, $options)->disableDoubleEncode();
It's a pretty good idea as long as we follow the defaults/overrides in Laravel blade compiler.

Edit:
If there is such a feature added to conditionally disable double encoding for specific input fields then we should also stick to the Laravel naming withoutDoubleEncoding and withDoubleEncoding ?

@mxmtsk mxmtsk deleted the master branch February 8, 2018 16:19
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.

4 participants