Currently, 8.0.x HEAD has seven cases where there are tests to determine if text has been escaped properly. The tests look similar to
$this->assertRaw(String::checkPlain($some_string));
While this works, it isn't the best idea. For one, it forces the developer to program to the implementation rather than the interface. It also violates a testing principle where the assertion should describe what you are testing, and not just generate a pass/fail.
AssertContentTrait should have a method for testing whether a string has been escaped or not in HTML output. Something like
protected function assertEscaped($raw, $message = '', $group = 'Other') {
if (!$message) {
$message = String::format('Escaped "@raw" found', array('@raw' => $raw));
}
return $this->assert(strpos($this->getRawContent(), String::checkPlain($raw)) !== FALSE, $message, $group);
}
should be sufficient.
I'll provide a patch if anyone thinks this is a good idea.
Comment | File | Size | Author |
---|---|---|---|
#3 | interdiff-01-03.txt | 3.75 KB | mpdonadio |
#3 | create_proper_test-2340785-3.patch | 7.55 KB | mpdonadio |
#1 | assertEscaped-2340785-01.patch | 5.31 KB | mpdonadio |
Comments
Comment #1
mpdonadioQuick proof of concept patch for discussion.
Comment #2
xjmComment #3
mpdonadioFixed a few bad copy/paste lines.
Added a assertNoEscaped to parallel the other versions.
Updated all instances of `$this->assertRaw(String::checkPlain($some_string));` in core:
Comment #4
Fabianx CreditAttribution: Fabianx commentedRTBC, looks great to me and is a very valuable asset.
This is major, due to improving security by ensuring developers make less mistakes during programming of security sensitive tests.
Comment #5
alexpottThis issue is a major task that will improve performance significantly and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed 010c4df and pushed to 8.0.x. Thanks!
Comment #7
mpdonadioDo I need to make a Change Notice for this?
Comment #8
cilefen CreditAttribution: cilefen commented@mpdonadio That would be good.
Comment #10
larowlanpatch coming
Comment #11
larowlanmeh, wrong issue