Removed quote conversion to entity inside content #25
Conversation
Encoding quotes inside element contents may be not necessary
|
This commit broke the unit tests, so I can't merge. I'm also deferring to @mattfarina to decide on this one. |
|
First, this looks like it would be a good change. To be merged the tests need to be updated in https://github.com/Masterminds/html5-php/blob/master/test/HTML5/Serializer/OutputRulesTest.php#L191. The actual test failures can be seen at https://travis-ci.org/Masterminds/html5-php/jobs/18130032. @goetas can you clean up the tests as well? |
|
I have added some tests. |
| @@ -245,7 +245,7 @@ protected function nl() { | |||
| * The encoded text. | |||
| */ | |||
| protected function enc($text) { | |||
| $flags = ENT_QUOTES; | |||
| $flags = 0; | |||
There was a problem hiding this comment.
I think we need to make an API addition here. In attribute mode a " needs to be turned into ". When not in attribute mode the < and > need to be converted. The details are at http://www.w3.org/TR/2013/CR-html5-20130806/syntax.html#escapingString.
This would mean when in attribute mode we need to use ENT_COMPAT. When and avoid encoding < and >.
If we going to fix this let's get it right. I'm the one the broke it so I'd like to see it right on the second go. We could either use a method for each mode or an argument. Because of escaping issues around < and > I'm inclined to add a new method.
What do you think? I'm happy to let you write it or I can tackle this problem if you like.
Sorry it took a second go of looking at this to realize this change. It was reading your tests that caused me to go back and re-read the spec. Testing FTW!
|
Also, I'm OK merging in your new tests. I'd like @technosophos to take a quick read of those tests first. |
|
I have added an optional parameter to enc method |
|
(The indentation with two white-spaces is so strange) |
|
Changes Unknown when pulling 7713a8b on goetas:patch-2 into * on Masterminds:master*. |
|
I'm okay with the changes here. This brings us into closer conformance to the spec. The history of the two-space indent can be traced, basically, back to the Drupal coding guidelines, which Masterminds adopted before PSR-1/2 came out. |
|
@goetas thanks. I see it's failing in php 5.3. can you take a look at it... https://travis-ci.org/Masterminds/html5-php/jobs/18208034 |
|
@mattfarina A solution can be somethig like this? $cusotmMap = \HTML5\Serializer\HTML5Entities::$map;
if (!($cusotmMap & ENT_QUOTES)){
unset($cusotmMap["'"]);
}
$ret = strtr($text, \HTML5\Serializer\HTML5Entities::$map); |
|
News? what can i do? |
| @@ -163,7 +163,7 @@ protected function attrs($ele) { | |||
| $len = $map->length; | |||
| for ($i = 0; $i < $len; ++$i) { | |||
| $node = $map->item($i); | |||
| $val = $this->enc($node->value); | |||
| $val = $this->enc($node->value, true); | |||
There was a problem hiding this comment.
Can you update true to be TRUE? For booleans values we use a coding style where these re uppercase.
|
@goetas thanks for working on this. I was mostly busy for a couple days. There are a couple other changes I'd like to see.
|
|
@mattfarina \HTML5\Serializer\HTML5Entities::$map contains |
|
Changes Unknown when pulling 1a89c6b on goetas:patch-2 into * on Masterminds:master*. |
| if (!$this->encode) { | ||
| return htmlspecialchars($text, $flags, 'UTF-8'); | ||
| if (!$this->encode && $attribute) { | ||
| return strtr($text, array('"'=>'"','&'=>'&')); |
There was a problem hiding this comment.
On the array can you add some space. '"' => '"', '&' => '&'
This will make it more readable.
| $tests = array( | ||
| '&\'<>"' => '&'<>"', | ||
| 'This + is. a < test' => 'This + is. a < test', | ||
| function getEncDataAttssribute(){ |
There was a problem hiding this comment.
Is the name getEncDataAttssribute a misspelling for the method name? Is this method needed? I didn't see where it's referenced.
|
@goetas Thanks for keeping this moving along. I "think" this last batch of comments cover everything and will lead to passing tests too. The only thing we have not done (and doesn't work right now anyway) is the encoding of |
|
@mattfarina sorry for codestyle mistakes and thank you for your patience! |
|
Sorry... I did NOT fix this issue. I just made a mistake in a commit message. @goetas Feel free to also add your info to the CREDITS file as a contributor. |
Removed quote conversion to entity inside content
|
@goetas I already added you to the CREDITS file. Let me know if that info should be changed. |
|
@mattfarina thank you for adding me in credits file |
Encoding quotes inside element contents may be not necessary