Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.