Now that filter_xss/filter_xss_admin is objectified we can start to convert these tests to PHPUnit.

Files: 
CommentFileSizeAuthor
#24 filter_xss_test-2006568-24.patch30.75 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,505 pass(es).
[ View ]
#24 interdiff.txt3.01 KBParisLiakos
#23 filter_xss_test-2006568-23.patch28.55 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,303 pass(es).
[ View ]
#23 interdiff.txt1.94 KBParisLiakos
#19 filter_xss_test-2006568-19.patch28.33 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,900 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#19 interdiff.txt1.62 KBParisLiakos
#16 filter_xss_test-2006568-16.patch28.29 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,879 pass(es), 17 fail(s), and 0 exception(s).
[ View ]
#16 interdiff.txt25.05 KBParisLiakos
#11 filter_xss_test-2006568-11.patch25.65 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,325 pass(es).
[ View ]
#11 interdiff.txt1 KBdawehner
#1 drupal-2006568-1.patch25.65 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 55,940 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new25.65 KB
PASSED: [[SimpleTest]]: [MySQL] 55,940 pass(es).
[ View ]

Even I just copied and pasted the tests they are failing for some odd reasons.

Status:Needs review» Needs work

The last submitted patch, drupal-2006568-1.patch, failed testing.

Huh, I'm seeing this failure with and without this patch:

HTML scheme clearing evasion -- spaces and metacharacters before scheme.

This is reproducible using the simpletest UI, and as a quick demo, via drush on 8.x head:

jhedstrom@hyperion:~/work/drupal-8/core (8.x) (@d8)☠ drush php-eval "var_dump(filter_xss('<img src=" &#14;  javascript:alert(0)">', array('img')));"
string(43) "<img src=" &amp;#14;  javascript:alert(0)">"

The string javascript should not be in that output. Not sure if there is a duplicate for this specific issue or not.

Seems pretty critical...not sure how those fails aren't appearing on the testbot.

Status:Needs work» Needs review

#1: drupal-2006568-1.patch queued for re-testing.

Queued for retest, as the only thing I can think of is that this test is failing on php 5.4, but not 5.3.

Wow yeah, I have no idea why this could be the case :(

Seems this is a known issue for php 5.4: #1210798: In PHP 5.4, html_entity_decode() doesn't decode invalid numeric entities. Given that, we could probably proceed with this patch, although switching to use dataproviders might simplify the test a bit.

Not sure if that will make the test easier to read? As the type of assertion varies too. It would then maybe have to be split into diffrerent tests (also an option)?

Just for the security aspect I totally agree that we should have custom messages for all of them, so yeah we don't really gain something by converting to data providers. Sadly it's impossible to commit this conversion, as this will show a broken test each time you run phpunit.

Well, you get a broken simpletest as it is now, so I don't really see the difference (with phpunit, at least it's a much faster failure--and it might draw more attention to #1210798: In PHP 5.4, html_entity_decode() doesn't decode invalid numeric entities which clearly needs to be resolved before Drupal 8 can ship).

StatusFileSize
new1 KB
new25.65 KB
PASSED: [[SimpleTest]]: [MySQL] 55,325 pass(es).
[ View ]

Yeah I agree. So what about a review now?

Let's adapt some of the assertions, all the other ones aren't boolean, so they can't be negated.

One change I'd recommend is adding a todo to the issue describing the php 5.4 breakage to avoid concussion and lots of duplicate issues. #1210798: In PHP 5.4, html_entity_decode() doesn't decode invalid numeric entities

One change I'd recommend is adding a todo to the issue describing the php 5.4 breakage to avoid concussion and lots of duplicate issues. #1210798: In PHP 5.4, html_entity_decode() doesn't decode invalid numeric entities

Though as this is a phpunit test it will be runned way more often and annoy a lot of people.

lets check PHP version in setUp and if its 5.4 dont run the tests? or something like that, with a @todo to restore once the issue above is in.
Also it should definitely be moved to providers

StatusFileSize
new25.05 KB
new28.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,879 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

ok, lets see whether i messed anything up when converting to data providers
then i ll fix 5.4

We should mark the test skipped so the issue doesn't get completely forgotten. http://phpunit.de/manual/3.0/en/incomplete-and-skipped-tests.html

Status:Needs review» Needs work

The last submitted patch, filter_xss_test-2006568-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.62 KB
new28.33 KB
FAILED: [[SimpleTest]]: [MySQL] 55,900 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

lets see this one

Status:Needs review» Needs work

The last submitted patch, filter_xss_test-2006568-19.patch, failed testing.

+++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.phpundefined
@@ -89,11 +89,6 @@ public function testFilterXssNormalized($value, $expected, $message) {
-        "\xc0aaa",
-        '',
-        'HTML filter -- overlong UTF-8 sequences.',
-      ),
-      array(
         "Who&#039;s Online",

Why was this removed?

16 days to next Drupal core point release.

See providerTestInvalidMultiByte() and testInvalidMultiByte(). It's already tested there. I'm confused by the current providerTestFilterXssNormalized() data, as Xss::filter (or filter_xss from before) would not give you these expected strings?

Status:Needs work» Needs review
StatusFileSize
new1.94 KB
new28.55 KB
PASSED: [[SimpleTest]]: [MySQL] 57,303 pass(es).
[ View ]

i messed up the argument order:P
this should be green..it also works under 5.4

StatusFileSize
new3.01 KB
new30.75 KB
PASSED: [[SimpleTest]]: [MySQL] 58,505 pass(es).
[ View ]

also, we missed filter_xss_admin tests

I would much prefer that we mark the failing test skipped using markTestSkipped() so it isn't entirely forgotten (see #17).

ah, well turns out, that only this dataset is problematic. i think the test should still run for other datasets, no point to skip it

Ah, I didn't realize that had to be called for an entire test, rather than a specific data set.

Status:Needs review» Needs work
Issue tags:-phpunit

The last submitted patch, filter_xss_test-2006568-24.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+phpunit

#24: filter_xss_test-2006568-24.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Skipping them now would mean that it will be harder to correct it later, so I think the current approach is fine.

Just to be sure, I ran them locally and they did not fail (which means that the php 5.4 check works).

Status:Reviewed & tested by the community» Fixed

Committed 81f4bc8 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.