Problem/Motivation

twig_engine::twig_render_template calls SafeMarkup::set() which is meant to be for internal use only.

Starting with #56 this issue also addresses #2505679: Remove SafeMarkup::set in FieldPluginBase::advancedRender() where the problem/motivation was:
FieldPluginBase:advancedRender calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • (done) Remove the call by refactoring the code.
  • (SafeString::create() has been documented) If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.
  • (done) Have ThemeManager return a SafeString object.
  • (done) Ensure that SafeString objects are preserved in Drupal\views\Plugin\views\field\FieldPluginBase::renderText()

Why does this patch touch so much code

  • Views is relying on the SafeMarkup::set() calls in twig_render_template() and ThemeManager to convey safeness - on converting them to SafeString objects needs to be able to pass this on up to the render system. Views interrupts the render pipeline to do it's stuff.
  • Views also uses the fact that $element is passed by reference to the render system. It extracts #markup from $element to fast render fields. This exposes a bug in HEAD where the render creates a SafeString hence the changes to doRender. See #57 for where this causes fails.

Remaining tasks

  1. (done) Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. (done. changes were made in ThemeTest.php) Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
  3. (done) If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

User interface changes

None

API changes

  • ThemeManager::render() can return a SafeString object
  • FieldPluginBase::advancedRender() can return a SafeString object
  • FieldPluginBase::renderText() can return a SafeString object
  • twig_render_template() can return a SafeString object
  • protect FieldPluginBase::renderTrimText() - nothing should be calling this externally
Files: 
CommentFileSizeAuthor
#113 2501931-3.113.patch38.89 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,453 pass(es). View
#113 107-113-interdiff.txt1.54 KBalexpott
#107 2501931-2.107.patch38.35 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,657 pass(es). View
#107 2501931.doRender.test-only.patch1.01 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,628 pass(es), 18 fail(s), and 0 exception(s). View
#97 2501931-2.97.patch37.33 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,562 pass(es). View
#97 95-97-interdiff.txt4 KBalexpott
#95 2501931-2.95.patch37.23 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,836 pass(es). View
#95 92-95-interdiff.txt4.14 KBalexpott
#92 2501931-2.92.patch34.91 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,842 pass(es). View
#92 89-92-interdiff.txt1023 bytesalexpott
#89 2501931-2.89.patch35.91 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,721 pass(es). View
#89 88-89-interdif.txt1.65 KBalexpott
#88 2501931-2.87.patch34.94 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,719 pass(es). View
#88 82-87-interdif.txt4.09 KBalexpott
#82 2501931-2.82.patch34.14 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,702 pass(es). View
#82 78-82-interdif.txt2.77 KBalexpott
#78 2501931-2.78.patch33.39 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,671 pass(es). View
#78 75-78-interdiff.txt626 bytesalexpott
#75 2501931-2.75.patch33.38 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,684 pass(es). View
#75 65-75-interdiff.txt1.33 KBalexpott
#65 2501931-2.65.patch32.95 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,699 pass(es). View
#65 63-65-interdiff.txt4.4 KBalexpott
#63 2501931-2.63.patch28.55 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,639 pass(es), 54 fail(s), and 12 exception(s). View
#63 57-63-interdiff.txt2.05 KBalexpott
#60 57-60-inerdiff.txt593 bytesRavindraSingh
#60 2501931-60-safe-markup-removed.patch26.51 KBRavindraSingh
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,604 pass(es), 67 fail(s), and 12 exception(s). View
#57 2501931-2.57.patch26.87 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,595 pass(es), 67 fail(s), and 12 exception(s). View
#57 54-57-interdiff.txt17.25 KBalexpott
#56 2501931-2.55.patch10.45 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,672 pass(es). View
#56 54-55-interdiff.txt10.45 KBalexpott
#54 interdiff-47-54.txt1.21 KBalimac
#54 document_or_remove-2501931-54.patch10.45 KBalimac
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,670 pass(es). View
#47 interdiff-46-47.txt780 bytesalimac
#47 document_or_remove-2501931-47.patch10.19 KBalimac
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,884 pass(es). View
#46 interdiff-44-46.txt2.25 KBalimac
#46 document_or_remove-2501931-46.patch9.43 KBalimac
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,890 pass(es). View
#44 interdiff-2501931-41to44.txt1.1 KBdavidhernandez
#44 document_or_remove-2501931-44.patch9.1 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,865 pass(es). View
#41 interdiff-2501931-39to41.txt1.17 KBdavidhernandez
#41 document_or_remove-2501931-41.patch9.08 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,867 pass(es). View
#39 interdiff-2501931-34-39.txt1.49 KBcmanalansan
#39 document_or_remove-2501931-39.patch8.83 KBcmanalansan
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,906 pass(es), 2 fail(s), and 0 exception(s). View
#34 interdiff-2501931-30-34.txt1.38 KBcmanalansan
#34 document_or_remove-2501931-34.patch7.34 KBcmanalansan
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,832 pass(es), 2 fail(s), and 0 exception(s). View
#30 document_or_remove-2501931-30.patch6.19 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,821 pass(es). View
#30 diff-2501931-26-30.txt1.19 KBcilefen
#27 interdiff.2501931.23.26.txt817 bytesscotself
#27 2501931.26.patch6.64 KBscotself
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,778 pass(es), 1 fail(s), and 0 exception(s). View
#23 2501931.23.patch5.85 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,569 pass(es), 1 fail(s), and 0 exception(s). View
#23 21-23-interdiff.txt2.55 KBalexpott
#21 2501931.21.patch3.3 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,542 pass(es), 21 fail(s), and 0 exception(s). View
#21 17-21-pseudo-interdiff.txt2.89 KBalexpott
#17 2501931.17.patch3.24 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,991 pass(es). View
#17 16-17-interdiff.txt484 bytesalexpott
#16 2501931.16.patch2.77 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,878 pass(es), 22 fail(s), and 0 exception(s). View
#16 12-16-interdiff.txt897 bytesalexpott
#12 2501931.12.patch1.89 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,941 pass(es), 40 fail(s), and 0 exception(s). View
#12 10-12-interdiff.txt717 bytesalexpott
#10 2501931.10.patch1.19 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,838 pass(es), 46 fail(s), and 0 exception(s). View
#1 document-2501931-1.patch736 bytespeezy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,722 pass(es). View

Comments

peezy’s picture

FileSize
736 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,722 pass(es). View
peezy’s picture

Status: Active » Needs review

Have at it, testbot.

Cottser’s picture

Cottser’s picture

Going to review this.

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/themes/engines/twig/twig.engine
@@ -103,6 +103,8 @@ function twig_render_template($template_file, array $variables) {
   return SafeMarkup::set(implode('', $output));

The only other way we could do this is SafeMarkup::format(). Is there any reason why this was not done or considered?

It doesn't invoke twig so it should be considered as an option, IMO. But there may be a reason why it wouldn't work.

Cottser’s picture

Title: Document SafeMarkup::set in twig_engine::twig_render_template » Document SafeMarkup::set in twig_render_template()
joelpittet’s picture

I think we can do it without that join because the items are finite. There are only ~4 pieces that need to be joined together. As long as we don't need to have arbitrary pieces that should be fine to join the pieces together.

Cottser’s picture

Right but there were some arguments against doing '@one@two@three@four' in another issue.

joelpittet’s picture

Status: Needs work » Postponed
alexpott’s picture

Status: Postponed » Needs review
FileSize
1.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,838 pass(es), 46 fail(s), and 0 exception(s). View

Actually I'm not sure that we actually need this SafeMarkup::set() here at all because whatever this returns is passed to the Twig engine which knows what to do with Twig_Markup.

alexpott’s picture

This change removes 4,300+ calls to SafeMarkup::set() on the user permissions page with 11 roles.

Function Name Calls Diff Calls
Diff%
Incl. Wall
Diff
(microsec)
IWall
Diff%
Excl. Wall
Diff
(microsec)
EWall
Diff%
Incl.
MemUse
Diff
(bytes)
IMemUse
Diff%
Excl.
MemUse
Diff
(bytes)
EMemUse
Diff%
Incl.
PeakMemUse
Diff
(bytes)
IPeakMemUse
Diff%
Excl.
PeakMemUse
Diff
(bytes)
EPeakMemUse
Diff%
Drupal\Component\Utility\SafeMarkup::set -4,343 -180.6% -10,200 -19.9% -10,200 -19.9% -147,636,416 -14778.7% -147,636,416 -14778.7% -2,487,144 -213.4% -2,487,144 -213.4%
Twig_Markup::__construct 3,264 135.7% 1,258 2.4% 1,258 2.4% 4,689,736 469.5% 4,689,736 469.5% 1,751,808 150.3% 1,751,808 150.3%
Twig_Markup::__toString 3,264 135.7% 29 0.1% 29 0.1% 4,688 0.5% 4,688 0.5% 0 0.0% 0 0.0%
is_scalar -1,078 -44.8% -59 -0.1% -59 -0.1% -16 -0.0% -16 -0.0% -16 -0.0% -16 -0.0%
htmlspecialchars 1,078 44.8% 62 0.1% 62 0.1% 159,440 16.0% 159,440 16.0% 11,112 1.0% 11,112 1.0%
Drupal\Component\Utility\SafeMarkup::checkPlain 1,078 44.8% 2,781 5.4% 2,101 4.1% 1,308,680 131.0% 1,139,008 114.0% 90,688 7.8% 71,856 6.2%
Drupal\Component\Utility\SafeMarkup::isSafe -1,077 -44.8% -374 -0.7% -363 -0.7% 734,928 73.6% 734,928 73.6% 18,184 1.6% 18,184 1.6%
alexpott’s picture

FileSize
717 bytes
1.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,941 pass(es), 40 fail(s), and 0 exception(s). View

We can do even better... checking Twig_Markup in SafeMarkup::isSafe() saves the additional calls to SafeMarkup::checkPlain and htmlspecialchars.

alexpott’s picture

Title: Document SafeMarkup::set in twig_render_template() » Document or remove SafeMarkup::set in twig_render_template()
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 12: 2501931.12.patch, failed testing.

The last submitted patch, 10: 2501931.10.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
897 bytes
2.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,878 pass(es), 22 fail(s), and 0 exception(s). View

Fixing some of the test fails.

alexpott’s picture

FileSize
484 bytes
3.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,991 pass(es). View

Fixed the views tests too.

The last submitted patch, 16: 2501931.16.patch, failed testing.

alexpott’s picture

Status: Needs review » Postponed

This should use the SafeString approach from #2506581: Remove SafeMarkup::set() from Renderer::doRender. Let's postpone this issue on that one.

dawehner’s picture

Status: Postponed » Needs work

This can be unpostponed now.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.89 KB
3.3 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,542 pass(es), 21 fail(s), and 0 exception(s). View

Using SafeString now and removing other unnecessary calls to SafeMarkup::checkPlain in twig_render_template().

Status: Needs review » Needs work

The last submitted patch, 21: 2501931.21.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
5.85 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,569 pass(es), 1 fail(s), and 0 exception(s). View

hmm... We need to preserve safeness across views rendering.

Status: Needs review » Needs work

The last submitted patch, 23: 2501931.23.patch, failed testing.

scotself’s picture

Taking a look at this at Drupal GovCon. Going to spend an hour or so looking at it...

scotself’s picture

Status: Needs work » Needs review

Had trouble running tests locally. Uploading this to see what the testbot does with it. Not sure yet how to make this test pass, but changing the test may not be the best way to handle and may not even address the actual issue. Will look at it again tomorrow, but wanted to at least throw this at the testbot.

scotself’s picture

FileSize
6.64 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,778 pass(es), 1 fail(s), and 0 exception(s). View
817 bytes

Oops, forgot the files. :)

Status: Needs review » Needs work

The last submitted patch, 27: 2501931.26.patch, failed testing.

onelittlebecca’s picture

Me and Brandon were working on this at the govcon sprint. We believe the function that isn't being called is on line 1198 in file core/modules/views/src/Plugin/views/field/FieldPluginBase.php... going to run debugger tomorrow.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
6.19 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,821 pass(es). View

@pwolanin helped me narrow this down - please make sure he gets credited.

pwolanin’s picture

The wrapping in SafeString and casting back is a little ugly for now, but avoiding adding a lot of strings to the safe string registry is important.

cilefen’s picture

Status: Needs review » Needs work

Needs work because we are merging the work on #2501745: Remove or document SafeMarkup::set in theme() in.

cilefen’s picture

Title: Document or remove SafeMarkup::set in twig_render_template() » Document or remove SafeMarkup::set in twig_render_template() and ThemeManager
Issue summary: View changes
cmanalansan’s picture

Status: Needs work » Needs review
FileSize
7.34 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,832 pass(es), 2 fail(s), and 0 exception(s). View
1.38 KB
pwolanin’s picture

Will the method ever return a string now?

cmanalansan’s picture

Yes, there are times that it will still return a string. Either a blank string, or whatever is returned by $render_function().

Status: Needs review » Needs work

The last submitted patch, 34: document_or_remove-2501931-34.patch, failed testing.

cmanalansan’s picture

The test needs to be adjusted to account for the fact that render() likely returns an object now. Working on this now.

cmanalansan’s picture

Status: Needs work » Needs review
FileSize
8.83 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,906 pass(es), 2 fail(s), and 0 exception(s). View
1.49 KB

Updated ThemeTest

Status: Needs review » Needs work

The last submitted patch, 39: document_or_remove-2501931-39.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
9.08 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,867 pass(es). View
1.17 KB
+++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
@@ -60,11 +60,16 @@ function testAttributeMerging() {
+      if (is_string($output)) {

is_string isn't quite the right check for the test. If something happens and a string gets returned, this condition is met. What we want is to check that it's an empty string.

The test fails were caused by a missing use statement.

pwolanin’s picture

Status: Needs review » Needs work

The else is nestled up here.

Also, I think it would be better to leave the is_sting() check and then use assertIdentical($output, '') or assertTrue($output === '')

-      if (is_string($output)) {
+      if ($output === '') {
         $this->assertTrue(TRUE, format_string('\Drupal::theme() returns a string for data type !type.', array('!type' => $type)));
       } else {
YesCT’s picture

Issue summary: View changes

looking at the remaining tasks,

I think there is test coverage, cause we had to change it.

and 3. is not relevant cause it was refactored.

marking those two things done.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
9.1 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,865 pass(es). View
1.1 KB

ok

YesCT’s picture

Issue summary: View changes
Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -317,7 +318,10 @@ public function render($hook, array $variables) {
    -        $output = SafeMarkup::set($info['function']($variables));
    

    (todo) we can remove the now unused use for SafeMarkup.

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -317,7 +318,10 @@ public function render($hook, array $variables) {
    +        // Theme functions do not render via the theme engine, so the output
    +        // is not autoescaped. However, we can only presume that the theme
    +        // function has been written correctly and that the markup is safe.
    

    (nit todo) this could be wrapped closer to 80 chars.

  3. +++ b/core/modules/simpletest/src/AssertContentTrait.php
    @@ -812,7 +812,7 @@ protected function assertThemeOutput($callback, array $variables = array(), $exp
    -    $output = $renderer->executeInRenderContext(new RenderContext(), function() use ($callback, $variables) {
    +    $output = (string) $renderer->executeInRenderContext(new RenderContext(), function() use ($callback, $variables) {
    

    (ok) I was wondering about casting this, seems like render() returns either a string or a SafeString object. So that must be why we are casting, so we are explicitly saying it is a string.

  4. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -60,11 +61,17 @@ function testAttributeMerging() {
    +    // return a string or an object that implements SafeStringInterface, even though the
    +    // theme function itself can return anything.
    

    (nit todo) we are changing this line, so we can line wrap to 80 chars.

  5. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -1239,6 +1240,9 @@ public function renderText($alter) {
    +      if ($value_is_safe) {
    +        $value = SafeString::create($value);
    +      }
    

    (ok) since this comes from editing a view, can be considered administrative text. so we are trusting it. just making the safe string object and not storing it helps not pollute the list of strings set as safe.

  6. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -1275,6 +1279,11 @@ public function renderText($alter) {
    +    // Preserve whether or not the string is safe.
    
    @@ -1284,7 +1293,13 @@ public function renderText($alter) {
    +    // Preserve whether or not the string is safe.
    

    (question) in both of these cases, I dont understand the comment.

  7. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -1284,7 +1293,13 @@ public function renderText($alter) {
    +      return SafeString::create($value . $suffix);
    

    (todo) I think the return type needs to change on FieldHandlerInterface::renderText() to be string or the object (specify the interface for safe strings.

  8. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -1284,7 +1293,13 @@ public function renderText($alter) {
    +    else {
    +      return $value . $suffix;
    +    }
    

    (question) are we always assuming that anything that has been rendered is safe? and so assuming that the string result of return $value . $suffix is safe also... then the comment about preserving if it is not safe is even more confusing. and maybe we should just make a safestring object in both cases?

  9. +++ b/core/themes/engines/twig/twig.engine
    @@ -74,7 +75,7 @@ function twig_render_template($template_file, array $variables) {
    -    $output['debug_prefix'] .= "\n<!-- THEME HOOK: '" . SafeMarkup::checkPlain($variables['theme_hook_original']) . "' -->";
    +    $output['debug_prefix'] .= "\n<!-- THEME HOOK: '" . htmlspecialchars($variables['theme_hook_original'], ENT_QUOTES, 'UTF-8') . "' -->";
    

    (ok) looks similar to approach from #2506035: Do not add placeholders from SafeMarkup::format() to the safe list.

alimac’s picture

FileSize
9.43 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,890 pass(es). View
2.25 KB

Addressing nitpicks 1, 2, and 4 from #45.

alimac’s picture

FileSize
10.19 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,884 pass(es). View
780 bytes

return type needs to change on FieldHandlerInterface::renderText() to be string or the object

This patch addresses the above.

alimac’s picture

Status: Needs work » Needs review
pwolanin’s picture

In this case:

     $value = $this->last_render;
+    $value_is_safe = SafeMarkup::isSafe($value);
 
     if (!empty($alter['alter_text']) && $alter['text'] !== '') {
       $tokens = $this->getRenderTokens($alter);
@@ -1239,6 +1240,9 @@ public function renderText($alter) {
     if ($alter['phase'] == static::RENDER_TEXT_PHASE_EMPTY && $no_rewrite_for_empty) {
       // If we got here then $alter contains the value of "No results text"
       // and so there is nothing left to do.
+      if ($value_is_safe) {
+        $value = SafeString::create($value);
+      }

We don't really need to use a SafeString object at all, since the string is already on the safe list. I guess it doesn't hurt for consistency.

In the other cases that look like this, we are avoiding putting the concatenated string into the safe list since I guess we are close to the point of going to Twig. However, we need to make sure the $suffix is safe also - is that user input?

alimac’s picture

@pwolanin, the $suffix doesn't seem to be user input, but then there is bit:

$suffix = '';
//   https://www.drupal.org/node/2423913.
$more_link = \Drupal::l($more_link_text, CoreUrl::fromUserInput('/' . $more_link_path, array('attributes' => array('class' => array('views-more-link')))));

$suffix .= " " . $more_link;

CoreUrl::fromUserInput?

alimac’s picture

Preserve whether or not the string is safe. If the string is not already marked safe, it will be sanitized by Twig.

^^ This should clarify points 6. and 8. in comment #45.

pwolanin’s picture

@alimac in the link generator we have

    $result = SafeMarkup::format('<a@attributes>@text</a>', array('@attributes' => new Attribute($attributes), '@text' => $variables['text']));

so the link from \Druapl::l() should be safe.

alimac’s picture

We don't really need to use a SafeString object at all, since the string is already on the safe list. I guess it doesn't hurt for consistency.

Looking back at this issue... I see comments #21 and #23 from alexpott. A number of tests failed before SafeString code was added. So it is not just for consistency.

alimac’s picture

FileSize
10.45 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,670 pass(es). View
1.21 KB

I took a stab at improving the comments, to address point 6. from #45.

YesCT’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

that makes much more sense. and addresses all my earlier concerns. as far as I can tell, this is ready.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.45 KB
10.45 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,672 pass(es). View
+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1173,7 +1173,7 @@ public function advancedRender(ResultRow $values) {
-      $this->last_render = $value;
+      $this->last_render = (string) $value;

We need to preserve safeness here too.

alexpott’s picture

FileSize
17.25 KB
26.87 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,595 pass(es), 67 fail(s), and 12 exception(s). View

The patch in #56 wasn't right.

YesCT’s picture

I think the change in #57 is #2505679: Remove SafeMarkup::set in FieldPluginBase::advancedRender() and so we can close 2505679 as duplicate of this issue.

The interdiff looks fine, just casting back to string, since now, we are returning a SafeString object.

Status: Needs review » Needs work

The last submitted patch, 57: 2501931-2.57.patch, failed testing.

RavindraSingh’s picture

Status: Needs work » Needs review
FileSize
26.51 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,604 pass(es), 67 fail(s), and 12 exception(s). View
593 bytes
+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1176,7 +1177,7 @@ public function advancedRender(ResultRow $values) {
+    if (empty((string) $this->last_render)) {

Doesn't require to be added defined by string, as we are getting $this->last_render as a string.

Status: Needs review » Needs work

The last submitted patch, 60: 2501931-60-safe-markup-removed.patch, failed testing.

alexpott’s picture

@RavindraSingh at that point it might be a SafeString object. The problem this patch is coming up against is that we have have to ensure that the a SafeString is never cast to a string in views rendering and lose it's safeness.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
28.55 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,639 pass(es), 54 fail(s), and 12 exception(s). View

Views does really funky rendering and uses the bubbled data to assemble the row in StylePluginBase::renderFields() which points out that we're doing the SafeString creation too late in Renderer::render(). (That was fun to work out).

Interdiff is back to #57 since, as explained in #62, #60 wasn't quite right.

Status: Needs review » Needs work

The last submitted patch, 63: 2501931-2.63.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.4 KB
32.95 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,699 pass(es). View

Fingers-crossed this will fix the rest of the tests.

alimac’s picture

diff --git a/core/modules/serialization/serialization.services.yml b/core/modules/serialization/serialization.services.yml
index 4c9397a..318a90c 100644
--- a/core/modules/serialization/serialization.services.yml
+++ b/core/modules/serialization/serialization.services.yml
@@ -30,6 +30,10 @@ services:
       arguments: ['Drupal\Core\Field\Plugin\Field\FieldType\PasswordItem']
       tags:
         - { name: normalizer, priority: 20 }
+  serializer.normalizer.safe_string:
+      class: Drupal\serialization\Normalizer\SafeStringNormalizer
+      tags:
+        - { name: normalizer }

I want to update the issue summary to reflect the most recent patch changes.

@alexpott, we are wondering why the new serializer was added?

alimac’s picture

Issue summary: View changes
alimac’s picture

I found the failing test for which the new serializer was added:

The expected JSON output was found.	Other	StyleSerializerTest.php	102	Drupal\rest\Tests\Views\StyleSerializerTest->testSerializerResponses()

Symfony\Component\Serializer\Exception\UnexpectedValueException: Could not normalize object of type Drupal\Core\Render\SafeString, no supporting normalizer found. in Symfony\Component\Serializer\Serializer->normalizeObject() (line 267 of /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/serializer/Serializer.php). 
alimac’s picture

Title: Document or remove SafeMarkup::set in twig_render_template() and ThemeManager » Document or remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender
alexpott’s picture

We need the new serializer so exporting views using the serializer works. All this does is change SafeStringInterface implementing objects to strings... which is exactly what they would have been before this patch.

YesCT’s picture

I'll review this.

alexpott’s picture

+++ b/core/modules/views/src/Tests/Handler/FieldUnitTest.php
@@ -349,7 +349,7 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, $random_name, 'By default, a string should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_name, 'By default, a string should not be treated as empty.');

@@ -363,14 +363,14 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, '0', 'By default, 0 should not be treated as empty.');
+    $this->assertIdentical((string) $render, '0', 'By default, 0 should not be treated as empty.');
...
-    $this->assertIdentical($render, "0", 'By default, "0" should not be treated as empty.');
+    $this->assertIdentical((string) $render, "0", 'By default, "0" should not be treated as empty.');

@@ -382,7 +382,7 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, $random_name, 'If hide_empty is checked, a string should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_name, 'If hide_empty is checked, a string should not be treated as empty.');

@@ -396,14 +396,14 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, '0', 'If hide_empty is checked, but not empty_zero, 0 should not be treated as empty.');
+    $this->assertIdentical((string) $render, '0', 'If hide_empty is checked, but not empty_zero, 0 should not be treated as empty.');
...
-    $this->assertIdentical($render, "0", 'If hide_empty is checked, but not empty_zero, "0" should not be treated as empty.');
+    $this->assertIdentical((string) $render, "0", 'If hide_empty is checked, but not empty_zero, "0" should not be treated as empty.');

@@ -437,28 +437,28 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, $random_name, 'If the rewritten string is not empty, it should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_name, 'If the rewritten string is not empty, it should not be treated as empty.');
...
-    $this->assertIdentical($render, $random_name, 'If the rewritten string is not empty, "" should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_name, 'If the rewritten string is not empty, "" should not be treated as empty.');
...
-    $this->assertIdentical($render, $random_name, 'If the rewritten string is not empty, 0 should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_name, 'If the rewritten string is not empty, 0 should not be treated as empty.');
...
-    $this->assertIdentical($render, $random_name, 'If the rewritten string is not empty, "0" should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_name, 'If the rewritten string is not empty, "0" should not be treated as empty.');

@@ -472,7 +472,7 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, $random_name, 'If the rewritten string is empty, it should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_name, 'If the rewritten string is empty, it should not be treated as empty.');

@@ -486,14 +486,14 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, '0', 'If the rewritten string is empty, 0 should not be treated as empty.');
+    $this->assertIdentical((string) $render, '0', 'If the rewritten string is empty, 0 should not be treated as empty.');
...
-    $this->assertIdentical($render, "0", 'If the rewritten string is empty, "0" should not be treated as empty.');
+    $this->assertIdentical((string) $render, "0", 'If the rewritten string is empty, "0" should not be treated as empty.');

@@ -508,28 +508,28 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, "0", 'If the rewritten string is zero and empty_zero is not checked, the string rewritten as 0 should not be treated as empty.');
+    $this->assertIdentical((string) $render, "0", 'If the rewritten string is zero and empty_zero is not checked, the string rewritten as 0 should not be treated as empty.');
...
-    $this->assertIdentical($render, "0", 'If the rewritten string is zero and empty_zero is not checked, "" rewritten as 0 should not be treated as empty.');
...
-    $this->assertIdentical($render, "0", 'If the rewritten string is zero and empty_zero is not checked, 0 should not be treated as empty.');
+    $this->assertIdentical((string) $render, "0", 'If the rewritten string is zero and empty_zero is not checked, 0 should not be treated as empty.');
...
-    $this->assertIdentical($render, "0", 'If the rewritten string is zero and empty_zero is not checked, "0" should not be treated as empty.');
+    $this->assertIdentical((string) $render, "0", 'If the rewritten string is zero and empty_zero is not checked, "0" should not be treated as empty.');

@@ -544,7 +544,7 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, $random_value, 'If the original and rewritten strings are valid, it should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_value, 'If the original and rewritten strings are valid, it should not be treated as empty.');

@@ -558,14 +558,14 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, $random_value, 'If the original and rewritten strings are valid, 0 should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_value, 'If the original and rewritten strings are valid, 0 should not be treated as empty.');
...
-    $this->assertIdentical($render, $random_value, 'If the original and rewritten strings are valid, "0" should not be treated as empty.');
+    $this->assertIdentical((string) $render, $random_value, 'If the original and rewritten strings are valid, "0" should not be treated as empty.');

@@ -580,7 +580,7 @@ function _testHideIfEmpty() {
-    $this->assertIdentical($render, "", 'If the rewritten string is zero, it should be treated as empty.');
+    $this->assertIdentical((string) $render, "", 'If the rewritten string is zero, it should be treated as empty.');

@@ -623,20 +623,20 @@ function _testEmptyText() {
-    $this->assertIdentical($render, $empty_text, 'If a field is empty, the empty text should be used for the output.');
+    $this->assertIdentical((string) $render, $empty_text, 'If a field is empty, the empty text should be used for the output.');
...
-    $this->assertIdentical($render, "0", 'If a field is 0 and empty_zero is not checked, the empty text should not be used for the output.');
+    $this->assertIdentical((string) $render, "0", 'If a field is 0 and empty_zero is not checked, the empty text should not be used for the output.');
...
-    $this->assertIdentical($render, $empty_text, 'If a field is 0 and empty_zero is checked, the empty text should be used for the output.');
+    $this->assertIdentical((string) $render, $empty_text, 'If a field is 0 and empty_zero is checked, the empty text should be used for the output.');

@@ -645,13 +645,13 @@ function _testEmptyText() {
-    $this->assertIdentical($render, $alter_text, 'If a field is empty, some rewrite text exists, but hide_alter_empty is not checked, render the rewrite text.');
+    $this->assertIdentical((string) $render, $alter_text, 'If a field is empty, some rewrite text exists, but hide_alter_empty is not checked, render the rewrite text.');
...
-    $this->assertIdentical($render, $empty_text, 'If a field is empty, some rewrite text exists, and hide_alter_empty is checked, use the empty text.');
+    $this->assertIdentical((string) $render, $empty_text, 'If a field is empty, some rewrite text exists, and hide_alter_empty is checked, use the empty text.');

In this test (unlike others) I've kept the assertIdentical because assertEquals does not return the expected results when comparing things like 0 and '' so the actual text is very significant.

pwolanin’s picture

A couple minor things to fix that I see.

  1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -387,7 +390,7 @@ public function render($hook, array $variables) {
    +    return ($output instanceof SafeString) ? $output : (string) $output;
    

    This should probably be SafeStringInterface instead of the concrete class?

  2. +++ b/core/themes/engines/twig/twig.engine
    @@ -108,12 +109,13 @@ function twig_render_template($template_file, array $variables) {
    +  // This output has already been rendered and is therefore considered safe.
    +  return SafeString::create(implode('', $output));
    

    we should probably change the function @return doxygen?

YesCT’s picture

Looked through the patch, and I agree aside from the things in #73 it is good, each change is needed for this issue (staying in scope).

alexpott’s picture

FileSize
1.33 KB
33.38 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,684 pass(es). View

Thanks @YesCT and @pwolanin for the reviews. Here's are patch addressing #73

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/themes/engines/twig/twig.engine
@@ -45,7 +45,7 @@ function twig_init(Extension $theme) {
- * @return string
+ * @return \Drupal\Component\Utility\SafeStringInterface

I think this should be string|\Drupal\Component\Utility\SafeStringInterface

YesCT’s picture

Title: Document or remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender » Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender

looking at the approach in the patch, updating the title.

alexpott’s picture

Status: Needs work » Needs review
FileSize
626 bytes
33.39 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,671 pass(es). View

@YesCT re #76 yep, in the hopefully extremely rare instance that we call twig_render_template to render an empty string :)

dawehner’s picture

  1. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -59,12 +60,18 @@ function testAttributeMerging() {
    +    // theme_test_false is an implemented theme hook so \Drupal::theme() service
    +    // should return a string or an object that implements SafeStringInterface,
    +    // even though the theme function itself can return anything.
    

    Well, we should test against the thing which core returns, so are there examples which don't return a safe string?

  2. +++ b/core/modules/system/templates/dropbutton-wrapper.html.twig
    @@ -12,7 +12,7 @@
    -{% if children %}
    +{% if children|length %}
    

    So I guess this is done because if $object is always true basically? I'm wondering whether we could manipulate the logic inside twig to take into account SafeStringinterface. It seems quite a burden for themers to distinct that?

  3. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -1284,7 +1296,16 @@ public function renderText($alter) {
    +      return SafeString::create($value . $suffix);
    

    General question: Given now that safe string is used by a module and not only the internal systems, I'm curious whether the @internal flag on SafeString can still be justified as it is at the moment

pwolanin’s picture

PHP seems to be sadly lacking anything like a magic method to evaluate emptiness of object.

However, I think we'd have the same problem if children was a render array? Well, I guess an empty array would be empty here, but a render array with empty string markup would not be.

YesCT’s picture

the twig length change was in #17 to fix the Drupal\views\Tests\Plugin\RowRenderCacheTest fail in #16

alexpott’s picture

FileSize
2.77 KB
34.14 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,702 pass(es). View

Re #79

  1. I've improved the test
  2. We should use is not empty like core/vendor/twig/twig/test/Twig/Tests/Fixtures/tests/empty.test. The thing here is that we're not evaluating emptiness with {% if children %}
  3. Opened #2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString to remove it. I think it is not internal. I put it on it to not have huge discussions about it. However SafeString - unlike SafeMarkup::set() is side effect free so I have no qualms about it not being @internal

Status: Needs review » Needs work

The last submitted patch, 82: 2501931-2.82.patch, failed testing.

alexpott’s picture

@xjm has asked why views using SafeString in this patch. It's because all the code is fired by the rendering done in StylePluginBase::renderFields() - where it calls $renderer->renderPlain($data);

xjm’s picture

I'm concerned about using SafeStringInterface in Views and about including #2505679: Remove SafeMarkup::set in FieldPluginBase::advancedRender() in the scope of this. @alexpott said that he found this to be necessary after 5h of debugging; I asked him to explain that a little more here on the issue.

alexpott’s picture

Test name	Pass	Fail	Exception
ExpandDrupal\system\Tests\System\AdminTest	0	1	1

I can't reproduce this locally.

Status: Needs work » Needs review

alexpott queued 82: 2501931-2.82.patch for re-testing.

alexpott’s picture

FileSize
4.09 KB
34.94 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,719 pass(es). View

I'm pretty sure that we can limit all the SafeString futzing in Views to Drupal\views\Plugin\views\field\FieldPluginBase::renderText()... which helpfully complies with the current @internal directive on the SafeStringInterface since this method is a views-last-minute-whilst-rendering-stuff-change-everything-about method.

Patch also improves documentation when casting to string to explain why.

alexpott’s picture

Issue summary: View changes
FileSize
1.65 KB
35.91 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,721 pass(es). View

I guess we have outstanding questions about FieldPluginBase::renderTrimText() but atm this method is set up to completely lose safeness and that is not changed by this patch. It only has one usage in core which is FieldPluginBase::renderText() - which preserves safeness. I think we should protect this method and be done.

I've fixed the documentation of FieldPluginBase::theme() since in reviewing the interface I realised it's been wrong since rendering returns safestrings. It either returns an empty string or a safe string. I've also fixed FieldPluginBase::render() since this will also often return SafeStrings.

alexpott’s picture

It is possible to go further in FieldPluginBase::renderText() and remove some of the SafeString::create()'s by providing methods for things that are obviously safe. For example, SafeString::trim() or SafeString::stripTags(). However doing this would break the immutability of the SafeString object so I think this is a bad idea. I can't think of any other option (at the moment) for how to not have to deal with SafeString creation in FieldPluginBase::renderText().

xjm’s picture

Issue tags: +VDC
alexpott’s picture

FileSize
1023 bytes
34.91 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,842 pass(es). View

Empty strings should never be SafeStrings so actually I think the changes to the twig templates {% if children is not empty %} are not necessary.

dawehner’s picture

+++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -167,7 +167,7 @@ public function preRender(&$values);
+   * @return string|\Drupal\Component\Utility\SafeStringInterface
    *   The rendered output.

@@ -202,7 +202,7 @@ public function postRender(ResultRow $row, $output);
+   * @return string|\Drupal\Component\Utility\SafeStringInterface
    *   The advanced rendered output.

@@ -237,7 +237,7 @@ public function isValueEmpty($value, $empty_zero, $no_skip_empty = TRUE);
    * @return string|\Drupal\Component\Utility\SafeStringInterface
-   *   The rendered output, or a SafeString object.
+   *   The rendered output.

@@ -280,7 +280,7 @@ public function getRenderTokens($item);
-   * @return string|false
+   * @return string|\Drupal\Component\Utility\SafeStringInterface
    *   Returns rendered output of the given theme implementation.

I'm curious whether at least one instance of the docs could explain, under which circumstances you get hack an object and when just a simple string.

alexpott’s picture

#93 see SafeMarkup::create() ... it will a string when it is an empty string or an unsafe string. Whichever way it is the calling code should in general just treat it like a string. I guess we could add @see's everywhere to the sanitisation or render docs. But is it worth it?

alexpott’s picture

Issue summary: View changes
FileSize
4.14 KB
37.23 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,836 pass(es). View

re #93/#94 OTOH what's the harm. Added docs.

Also in discussion with @dawehner and @damiankloip making FieldPluginBase::renderTrimText() protected to reduce the surface area of the interface and make all render functions it has behave the same as core render functions which is to return a SafeStringInterface object where correct and possible.

I'm not sure that the API changes listed in the issue summary actually warrant a CR.

dawehner’s picture

Yeah I'm not sure whether its worth, most code should work the same anyway, unless you do stupid things ...

+1 for making that method protected. The static issue is what actually matters as its kinda a useful tool

alexpott’s picture

FileSize
4 KB
37.33 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,562 pass(es). View

Rerolled to use the new ViewsRenderPipelineSafeString object. I think this is good to go now.

Wim Leers’s picture

Just one nitty question:

  1. +++ b/core/modules/comment/src/Tests/Views/CommentFieldNameTest.php
    @@ -91,11 +91,11 @@ public function testCommentFieldName() {
    -    $this->assertIdentical($this->comment->getFieldName(), $output);
    +    $this->assertEqual($this->comment->getFieldName(), $output);
    ...
    -    $this->assertIdentical($this->customComment->getFieldName(), $output);
    +    $this->assertEqual($this->customComment->getFieldName(), $output);
    
    +++ b/core/modules/user/src/Tests/Views/HandlerFieldUserNameTest.php
    @@ -63,7 +63,7 @@ public function testUserName() {
    -    $this->assertIdentical($render, $username, 'If the user is not linked the username should be printed out for a normal user.');
    +    $this->assertEqual($render, $username, 'If the user is not linked the username should be printed out for a normal user.');
    

    In all other assertions in this patch, the rendered output is cast to a string so that the identical comparison can remain. Why not also here?

  2. +++ b/core/modules/serialization/src/Normalizer/SafeStringNormalizer.php
    @@ -0,0 +1,29 @@
    +class SafeStringNormalizer extends NormalizerBase {
    

    This didn't even need to be changed, nice :)

alexpott’s picture

@Wim Leers re #98.1 this is because identical-ness is super important to FieldUnitTest. See #72.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Alrighty :)

joelpittet’s picture

This touches so many things! And what is a ViewsRenderPipelineSafeString?

Wim Leers’s picture

alexpott’s picture

Issue summary: View changes

Adding some answers to why this patch touches so much code to the issue summary.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

FileSize
1.01 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,628 pass(es), 18 fail(s), and 0 exception(s). View
38.35 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,657 pass(es). View

@xjm asked about test coverage of the changes in doRender and pointed out we're missing explicit test coverage. We only have implicit in Views testing. Patch attached fixes that. The test only patch is the interdiff :)

The last submitted patch, 107: 2501931.doRender.test-only.patch, failed testing.

Wim Leers’s picture

Even better :) RTBC++

catch’s picture

Status: Reviewed & tested by the community » Fixed

Just a couple of nits which I fixed on commit, otherwise looks good.

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1213,7 +1217,14 @@ public function isValueEmpty($value, $empty_zero, $no_skip_empty = TRUE) {
+    // Xss admin filtered. See self::renderAltered() as an exmaple that does.

Fixed on commit - XSS / example.

  • catch committed 9ccb44e on 8.0.x
    Issue #2501931 by alexpott, alimac, cmanalansan, davidhernandez,...

  • alexpott committed 3f0ff21 on 8.0.x
    Revert "Issue #2501931 by alexpott, alimac, cmanalansan, davidhernandez...
alexpott’s picture

Status: Fixed » Needs review
FileSize
1.54 KB
38.89 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,453 pass(es). View

Here's a fix that allows us to run all the phpunit tests from the command line again. Also included @catch's fix on commit.

To test do "cd core; phpunit --testsuite unit" from the command line.

And this is reason #142 why SafeMarkup is very very tricky to work with.

alexpott’s picture

So testbot can not pick up failures like #113 because it runs each phpunit test individually in a separate process. The long and short of it is - static properties are bad and, yes, we knew that but it didn't stop us from using them anyway. /me dreams of drupal_static() from drupal since at least that allowed us to reset the lot.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

New testbot came back green. That means old testbot should come back green too. 50 minutes in, it's still waiting to begin testing. So expect that to come back green in ~25 minutes at best.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 45a48e8 on 8.0.x
    Issue #2501931 by alexpott, alimac, cmanalansan, davidhernandez,...

Status: Fixed » Closed (fixed)

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

ExTexan’s picture

This issue was closed 9 months ago, and the patch was supposedly committed to core, but I'm on v8.0.5 and am still having this problem. The HTML markup I put in the "rewrite" text area is being shown.

I checked this patch (and a couple others that seemed to be related) and they do, in fact, appear to have been committed to core. However, the issue still remains. Any suggestions?

cilefen’s picture

@ExTexan This issue was a refactoring task. The issue you are having sounds specific to Views and I think there have been a few double-escaping issues. Open a new issue in the Views component if none exists.

ExTexan’s picture

@cilefen, Yes, will do. Thanks.

I just found the trigger is aggregation on or off. When off, HTML shows fine - when on, it is escaped.