BasicStringFormatter's nl2br() forces escaping *and* filtering — improve this

Many ways to skin a escaped string.
http://twig.sensiolabs.org/doc/filters/nl2br.html

The value will be escaped normally by twig and the new lines will be converted to br tags. BOOM!

+1 rtbc, just clean-up "use" and inified array syntax

Is there a test for this?

Yep, there's
\Drupal\field\Tests\String\RawStringFormatterTest::testStringFormatter
\Drupal\field\Tests\String\StringFormatterTest::testStringFormatter

also \Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter::viewValue() does the same, so maybe override that too?

I'm sure better re-title

Fix broken tests, also filed new bug and add todo for #2567745: \Drupal\taxonomy\Plugin\views\field\TermName::getItems should work with links

+++ b/core/modules/field/src/Tests/String/UuidFormatterTest.php
@@ -47,11 +47,11 @@ public function testUuidStringFormatter() {
-    $this->assertIdentical($render_array[0]['#markup'], $entity->uuid(), 'The rendered UUID matches the entity UUID.');
+    $this->assertIdentical($render_array[0]['#context']['value'], $entity->uuid(), 'The rendered UUID matches the entity UUID.');
...
-    $this->assertIdentical($render_array[0]['#title']['#markup'], $entity->uuid());
+    $this->assertIdentical($render_array[0]['#title']['#context']['value'], $entity->uuid());

Can we just render the array and get the value so we don't have to keep changing this test if the way we build the data changes later?

$this->render()

Thanks @andypost, that makes all the other things consistent. I think it's good to go here too, it's a bit scope creepy but tolerable I think.

Fixed #11

Thanks setting back to RTBC.

Looks like unrelated failure.

Drupal\system\Tests\Update\UpdatePathTestBaseTest	388	1	0
									
Message	Group	Filename	Line	Function	Status
GET http://ec2-54-191-113-78.us-west-2.compute.amazonaws.com/checkout/update.php/results returned 0 (0 bytes).

I'm sure we need to extend formatters tests
- UuidFormatterTest tests build of StringFormatter
- RawStringFormatterTest tests build of BasicStringFormatter

Now tests are the same

+++ b/core/modules/field/src/Tests/String/UuidFormatterTest.php
@@ -46,13 +46,23 @@ public function testUuidStringFormatter() {
+    $this->assertIdentical($render_array[0]['#title']['#type'], 'inline_template');
+    $this->assertIdentical($render_array[0]['#title']['#template'], '{{ value|nl2br }}');
+    $this->assertIdentical($render_array[0]['#title']['#context']['value'], $entity->uuid());

@andypost the reason I asked @lauriii to change is it is because this is testing how we built something and not what we built. Though I appreciate more tests I think this the wrong kind of test. It should test the output is as expected not the "how we built the output".

@joelpittet as you see I extend tests same way as they are. Mean testing how formatter returns build, also checking how that rendered.

+++ b/core/modules/field/src/Tests/String/UuidFormatterTest.php
@@ -46,13 +46,23 @@ public function testUuidStringFormatter() {
-    $this->assertIdentical($render_array[0]['#markup'], $entity->uuid(), 'The rendered UUID matches the entity UUID.');
+    $this->assertIdentical($render_array[0]['#type'], 'inline_template');
+    $this->assertIdentical($render_array[0]['#template'], '{{ value|nl2br }}');
...
     $this->assertIdentical($render_array[0]['#type'], 'link');

for me that exactly a test of the formatter build

Yes, that's what I'm saying we shouldn't be testing "how it's built" but rather we should test "what is the end result of the render of the array". Does that make sense?

Put in other words: Right now it's testing the input is there instead of testing the output we get from that input.

Does someone mind trying to profile how much this makes for example /admin/content slower, with let's say 50 nodes?

Scenario

xdebug off, twig C extension off. xhprof on.

1. Added a plaintext long field to article,
2. set the field on teaser display view mode,
3. generated 50 nodes,
4. ensured nl2br was being rendered from twig template and back to 8.0.x branch for the xhprof.
5. Turned of render cache bins (render and dynamic_page)

=== 8.0.x..8.0.x compared (55fa31db35b03..55fa3a49350d5):

ct  : 252,217|252,217|0|0.0%
wt  : 616,541|616,601|60|0.0%
mu  : 29,857,784|29,857,784|0|0.0%
pmu : 30,011,168|30,011,168|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55fa31db35b03&...

=== 8.0.x..2554755 compared (55fa31db35b03..55fa3a641876b):

ct  : 252,217|249,109|-3,108|-1.2%
wt  : 616,541|610,817|-5,724|-0.9%
mu  : 29,857,784|29,929,568|71,784|0.2%
pmu : 30,011,168|30,083,776|72,608|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55fa31db35b03&...

Oh this is really nice. As we no longer use #markup, we no longer have to Xss::filter() the result, which saves us quite some time, see http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=Drupal%5CCom...

Oh I forgot to mention but I'm only showing 10 nodes at a time, should have changed it to 50 but I'm sure the % would be similar.

@andypost re #20 is there any reason why we should be testing the input?

reverted #17 changes and added more render tests, should be ready now

Thank you @andypost. Still not sure why some of this needs to test the input but part of that was there before this patch so as long as it's testing the output I'm happy:)

Twig making things quicker... now there is a turn up for the books! Nice work all. Committed 3137a14 and pushed to 8.0.x. Thanks!