Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Now that filter_xss/filter_xss_admin is objectified we can start to convert these tests to PHPUnit.
Comment | File | Size | Author |
---|---|---|---|
#24 | filter_xss_test-2006568-24.patch | 30.75 KB | ParisLiakos |
#24 | interdiff.txt | 3.01 KB | ParisLiakos |
#23 | filter_xss_test-2006568-23.patch | 28.55 KB | ParisLiakos |
#23 | interdiff.txt | 1.94 KB | ParisLiakos |
#19 | filter_xss_test-2006568-19.patch | 28.33 KB | ParisLiakos |
Comments
Comment #1
dawehnerEven I just copied and pasted the tests they are failing for some odd reasons.
Comment #3
jhedstromHuh, 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:
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.
Comment #4
jhedstrom#1: drupal-2006568-1.patch queued for re-testing.
Comment #5
jhedstromQueued for retest, as the only thing I can think of is that this test is failing on php 5.4, but not 5.3.
Comment #6
dawehnerWow yeah, I have no idea why this could be the case :(
Comment #7
jhedstromSeems 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.
Comment #8
damiankloip CreditAttribution: damiankloip commentedNot 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)?
Comment #9
dawehnerJust 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.
Comment #10
jhedstromWell, 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).
Comment #11
dawehnerYeah 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.
Comment #12
jhedstromOne 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
Comment #13
jhedstromOne 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
Comment #14
dawehnerThough as this is a phpunit test it will be runned way more often and annoy a lot of people.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedlets 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
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedok, lets see whether i messed anything up when converting to data providers
then i ll fix 5.4
Comment #17
jhedstromWe 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
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedlets see this one
Comment #21
jhedstromWhy was this removed?
16 days to next Drupal core point release.
Comment #22
damiankloip CreditAttribution: damiankloip commentedSee 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?
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedi messed up the argument order:P
this should be green..it also works under 5.4
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedalso, we missed filter_xss_admin tests
Comment #25
jhedstromI would much prefer that we mark the failing test skipped using
markTestSkipped()
so it isn't entirely forgotten (see #17).Comment #26
ParisLiakos CreditAttribution: ParisLiakos commentedah, well turns out, that only this dataset is problematic. i think the test should still run for other datasets, no point to skip it
Comment #27
jhedstromAh, I didn't realize that had to be called for an entire test, rather than a specific data set.
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commented#24: filter_xss_test-2006568-24.patch queued for re-testing.
Comment #30
dawehnerSkipping 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).
Comment #31
alexpottCommitted 81f4bc8 and pushed to 8.x. Thanks!
Comment #33
jhedstrom