Follow-up to #2501827: Remove SafeMarkup::set() in file_save_upload() and allow render/template code to control a single-item list

Problem/Motivation

Strings are being passed to drupal_set_message() which doesn't support render arrays.

should optionally make drupal_set_message() support render arrays?

Proposed resolution

by at the minimum using the renderer itself when needed (there are pros and cons to that)?
or using hashing or suchlike to provide the array keys (pros and cons to that as well) and supporting actual render arrays in a bubbleable-and-best-practice-and-etc. fashion.

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#87 interdiffing-in-circles.txt2.13 KBxjm
#87 dsm-2505497-86.patch6.85 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,519 pass(es), 1 fail(s), and 0 exception(s). View
#74 interdiff-71-74.txt2.73 KBAki Tendo
#74 2505497-74.diff5.55 KBAki Tendo
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,456 pass(es). View
#71 interdiff.txt4.83 KBAki Tendo
#71 2505497-71.diff6.83 KBAki Tendo
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,469 pass(es). View
#69 Uses__attached_in_render_array_with_drupal_set_message_____Site-Install.png199.87 KBjoelpittet
#66 2505497-2.66.patch7.93 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,344 pass(es). View
#66 64-66-interdiff.txt2.9 KBalexpott
#64 2505497-2.64.patch7.82 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,340 pass(es). View
#64 63-64-interdiff.txt5.51 KBalexpott
#63 nits.txt2.13 KBxjm
#63 dsm-2505497-63.patch6.84 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,330 pass(es). View
#56 interdiff-56.txt3.51 KBxjm
#56 dsm-2505497-56-PASS.patch6.96 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,334 pass(es). View
#56 dsm-2505497-56-FAIL.patch1.63 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,326 pass(es), 2 fail(s), and 0 exception(s). View
#49 dsm-25054970-49.patch5.1 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,939 pass(es). View
#42 dsm-2505497-40-do-not-test.patch5.1 KBxjm
#42 dsm-2505497-40-with-other-issue.patch8.33 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,928 pass(es). View
#30 interdiff.txt967 bytesjoelpittet
#30 support_render_arrays-2505497-30.patch8.11 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,834 pass(es). View
#27 interdiff.txt1.59 KBxjm
#27 dsm-2505497-27.patch8.11 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,819 pass(es). View
#26 2505497.26.patch7.96 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,722 pass(es). View
#26 18-26-interdiff.txt6.85 KBalexpott
#18 interdiff.txt1.62 KBjoelpittet
#18 decide_how_and_whether-2505497-18.patch3.86 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,691 pass(es). View
#14 interdiff.txt3.03 KBxjm
#14 dsm-2505497-14.patch3.85 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,695 pass(es). View
#11 decide_how_and_whether-2505497-11.patch2.76 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,693 pass(es). View
#3 decide_how_and_whether-2505497-3.patch2.75 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch decide_how_and_whether-2505497-3.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

xjm’s picture

Issue tags: +SafeMarkup
joelpittet’s picture

IMO, it should accept them but render them right away to avoid having to serialize/unserialize across the session.

In this one case I'm for early rendering (not really that early...)

joelpittet’s picture

Status: Active » Needs review
FileSize
2.75 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch decide_how_and_whether-2505497-3.patch. Unable to apply patch. See the log in the details link for more information. View

Let's just spark a convo with a patch:)

cilefen’s picture

If this can be done in backwards-compatible way, could we move it to 8.1.x? It is Major but is its impact greater than its possible disruption?

joelpittet’s picture

@cilefen seems BC to me... and mostly helps unblock some of the SafeMarkup::set() issues. I'd like to keep it in 8.0.x if possible.

It's and API addition, yes, but it's also a bit of a critical blocker at the moment.

cilefen’s picture

@joelpittet A critical blocker ... that is another thing completely. It wasn't obvious looking at it. I assume it is the SafeMarkup stuff.

cilefen’s picture

This may be a silly question, but is there a way to know if we were passed a render array or an array with junk in it?

joelpittet’s picture

@cilefen AFAIK no there isn't really, render arrays aren't strongly typed they are just loose arrays with properties in '#'. And even then it may be a list of render elements in which case it wouldn't have any '#' keys. If we could reliably determine one somehow, that would be a great public static method on Renderer I'd think:)
Renderer::isRenderArray($var)?

This issue will need a re-roll with #2506581: Remove SafeMarkup::set() from Renderer::doRender

Status: Needs review » Needs work

The last submitted patch, 3: decide_how_and_whether-2505497-3.patch, failed testing.

joelpittet’s picture

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

re-rolled

xjm’s picture

So ideally I don't think the early render is the way to go here. Also, following #2506581: Remove SafeMarkup::set() from Renderer::doRender there's that string typecast because... I still don't understand why, because how would a SafeStringInterface object have been getting passed to drupal_set_message()? Edit: but the string cast would be redundant if we did render early the line above, was my point.

xjm’s picture

Now, if render arrays were objects, everything would be magic! and the typecast would merrily solve our problems. But that's D9 land.

xjm’s picture

FileSize
3.85 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,695 pass(es). View
3.03 KB

I looked into this and I don't even think we need the early render anymore. @alexpott has mentioned something about the messages being used as array keys but I can't find the code path whether that happens. In fact, using render arrays might even allow us to deprecate the confusing 'safe' flag for messages. Because the attached... seems to work.

(This is another reason to document your typecasts, because someone might just remove them if they seem to be in the way and don't explain why they're there.) :P

xjm’s picture

+++ b/core/includes/bootstrap.inc
@@ -514,10 +514,10 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
  *   (optional) The translated message to be displayed to the user. For
  *   consistency with other messages, it should begin with a capital letter and
- *   end with a period.
+ *   end with a period or a render array.

lol, I think these docs are not quite right.

xjm’s picture

So, if there is a reason for the string typecast that was added, or a place where these messages are used as array keys, neither of them seem to have test coverage.

joelpittet’s picture

@xjm I end all my sentences with a render ['#markup' => ':P'];

joelpittet’s picture

FileSize
3.86 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,691 pass(es). View
1.62 KB

@xjm, my thought on #16 was that it would be nicer to avoid sending across a serialized array because the message is put into sessions and out of session. But it looks like from your test that doesn't matter much, interesting.

Updated the docs a bit, otherwise this one looks good to go from me RTBC+1

Status: Needs review » Needs work

The last submitted patch, 18: decide_how_and_whether-2505497-18.patch, failed testing.

joelpittet’s picture

Apparently testbot thinks my comments throw migrate exceptions...

The last submitted patch, 18: decide_how_and_whether-2505497-18.patch, failed testing.

joelpittet’s picture

head was broken, angie reverted the migrate blocked ips commit.

alexpott’s picture

+++ b/core/includes/bootstrap.inc
@@ -610,6 +610,8 @@ function drupal_get_messages($type = NULL, $clear_queue = TRUE) {
+        // It's only nessasary to set the message as 'safe' when the message is
+        // not a renderable array.
         if ($message['safe']) {

So we can go a step further here and do
if (!SafeMarkup::isSafe($message['message']) && $message['safe']) because we're now passing SafeString objects through the session system since we're no longer casting to string. I've tested this and it works well even when the session is being used by the next request.

However, not casting SafeStrings to strings and using render arrays (though less likely) could cause duplicate messages. I think the only way to go would be to move message de-duplication to render time rather than store time. We can not predict how the search is going to behave with objects and array-of-anything. This solution though would mean that status-messages.html.twig will become more complex than it already is.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.85 KB
7.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,722 pass(es). View

<3 in_array and non-strictness!

Patch adds comprehensive tests around the scenario described in #25 and makes the improvement to not SafeMarkup::set() SafeString objects. It also merges all the tests in Drupal\system\Tests\Bootstrap\DrupalSetMessageTest into one because installing Drupal 3 times to get the same page (with no variance on this page) make assertions against it is extremely wasteful.

The scenario of not detecting duplicates is only an issue when comparing a render array to a string (or SafeString) since PHP's loose non strict equivalence checking can not help us here. This expected failure is tested and documented (probably could be improved) in drupal_set_message().

xjm’s picture

FileSize
8.11 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,819 pass(es). View
1.59 KB

+1 for the test coverage improvements.

I agree that one extra duplicate in the unlikely case that someone wants to pass a render array and a string of the same thing is not really a concern. I filed #2545414: Allow deduplication of the "same" message when it is passed as both a render array and string just in case and added notes about it to the test. Also fixed an AE/BE thing.

I think this is ready.

xjm’s picture

Title: Decide how and whether to support render arrays for drupal_set_message() » Support render arrays for drupal_set_message()
xjm’s picture

+++ b/core/includes/bootstrap.inc
@@ -610,6 +617,8 @@ function drupal_get_messages($type = NULL, $clear_queue = TRUE) {
+        // It's only nessasary to set the message as 'safe' when the message is

Oops, needs a spelling fix: necessary. Could be fixed on commit also.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
8.11 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,834 pass(es). View
967 bytes

Typo fix and "renderable array" to "render array" for consistency in the patch at least. Thanks for the tests @alexpott.

catch’s picture

Status: Reviewed & tested by the community » Needs work

I'm not sure about this one:

- we could avoid some of the marking safe by having drupal_set_message() to accept a raw string and arguments rather than making people call t() directly on the string. Haven't seen that option discussed.

- Earlier in the issue it was discussed doing early rendering when drupal_set_message() is called, but patch now puts the render array into session. That opens us up to storing entity objects and whatever else in $_SESSION which gets very unpredictable.

- I asked alexpott for examples of where this would be used, and the installer and batch API were mentioned - I think we need more straightforward examples than those two to justify complicating what is a very commonly used API. For me it's fine if the installer/batch need to take some extra care.

catch’s picture

More concerns:

- Renderer::renderPlain() seems fine for building up strings - this is similar to composing an e-mail message in that the string is rendered completely outside the current context.

- once we allow render arrays we're supporting #attached implicitly. That means a dsm() on one page can affect css/js/headers etc. on another page. Or if it's early-rendered in dsm() then you lose the #attached information when only the string is in $_SESSION.

xjm’s picture

@catch, for a simple example, see #2501827: Remove SafeMarkup::set() in file_save_upload() and allow render/template code to control a single-item list. HEAD currently has to do an early render on a simple file upload in order to use a render array.

catch’s picture

That can use Renderer::renderPlain() though - which is designed to convert a render array into a string with no assets etc. It's the same API as if you're building a long string to send an e-mail, and has the same limitations.

xjm’s picture

we could avoid some of the marking safe by having drupal_set_message() to accept a raw string and arguments rather than making people call t() directly on the string. Haven't seen that option discussed.

I think it'd be better DX to have a consistent API (i.e. render arrays are the base element of output). Giving up the late rendering would be unfortunate, but I'd rather do the early render if it's necessary than further diverge dsm() from everything else that takes renderable content.

xjm’s picture

FWIW, the batch example @alexpott gave me was _node_mass_update_batch_finished() which has an escaping bug in HEAD probably and no existing issue yet. Edit: and callback_batch_finished().

Wim Leers’s picture

I haven't reviewed this in detail, but supporting render arrays in general frightens me at least on first thought. Because it means messages could in fact contain any sort of HTML, including tables, vertical tabs, and so on.

- once we allow render arrays we're supporting #attached implicitly. That means a dsm() on one page can affect css/js/headers etc. on another page. Or if it's early-rendered in dsm() then you lose the #attached information when only the string is in $_SESSION.

Anything that is placeholdered can affect CSS/JS already, so that's not actually a problem.

xjm’s picture

Assigned: Unassigned » xjm

Discussed with alexpott; I'll update this to at least keep the render arrays out of the session API.

alexpott’s picture

Discussed with @xjm - views here are my own coming out of that discussion. So one thing about late rendering via Drupal_set_message() is #attached. I think if the message is set on one request and early rendered but not displayed then the assets will not be added to the next request where the message is displayed. But render arrays in session is an unknown. I guess this is a hard call.

One idea is to do the rendering inside drupal_set_message(). I'm not sure I'm a fan because drupal_set_message is used everywhere and render only works if it's in a render context.

   * @throws \LogicException
   *   When called outside of a render context. (i.e. outside of a renderRoot(),
   *   renderPlain() or executeInRenderContext() call.)

Personally I think that if we're not going support late rendering via passing render array through session it should be up to the caller to do the rendering as it is way more control of the context that drupal_set_message() is.

Created #2546176: Use SafeStrings to communicate safeness in drupal_set_message and remove SafeMarkup::set() to include the improve SafeString tests added here and remove the string cast.

xjm’s picture

I disagree with #39 and/or don't understand the point. Whether the early rendering is done in dsm() or the caller, it's still early rendering. And we'd still only be doing it in the case where there is a render array.

Also, in general, we should not teach developers that the way to solve their problems is early rendering. I can see the case for not wanting to store render arrays in $_SESSION, but doing a conditional renderPlain() in one place seems like much better DX and better control over what goes on than having early renders all over the place.

alexpott’s picture

"early renders all over the place." Using render arrays to build strings for drupal_set_message() is extremely uncommon. There's one in HEAD. We want to add one more with the installer and we might need to add more to fix the finished batch messages. Funnily enough what the batch and file validate messages have in common is that they want to add a list to the message - ie. A group of sub messages - perhaps we should make that easy instead?

xjm’s picture

Status: Needs work » Needs review
FileSize
8.33 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,928 pass(es). View
5.1 KB

Attached restores the early render and goes into a little more detail as to why it's needed. I've rolled it on top of #2546176: Use SafeStrings to communicate safeness in drupal_set_message and remove SafeMarkup::set(). Interdiff was too confusing with the rebased patch but the key change is essentially going back to #11 just with the added docs and tests.

xjm’s picture

There's one in HEAD

There are at least three in HEAD, and four if we add one in hook_requirements().

xjm’s picture

Funnily enough what the batch and file validate messages have in common is that they want to add a list to the message - ie. A group of sub messages - perhaps we should make that easy instead?

I think that's exactly what we're doing here--making lists easy in dsm()--plus supporting other usecases as well where we don't necessarily want to use a list. We already have an API for rendering things as a list -- I don't think we need to add another special one just for dsm(). We should just use the one we have, and not encourage early rendering.

Having an early render within drupal_set_message() for its special case of noodling with $_SESSION seems preferable to me to polluting module code with it (and thereby setting a bad model for devs).

alexpott’s picture

Here's a very rough sketch of what I was trying to say in #41. If we do something like that we get a simple way of adding sub messages to a message, late render, and themeability.

joelpittet’s picture

+1 to early render. I'm glad @catch brought that eventually up in #31 that you could inadvertently pass along a large object like an #entity through the session under the guise of a render array and that would be a big waste of resources.

Edit: and #42 is the answer.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Actually, I'm going to be bold here and RTBC. I've reviewed the patch and even compared diffs of #42 against @alexpott's awesome patch in #2546176: Use SafeStrings to communicate safeness in drupal_set_message and remove SafeMarkup::set() and love both, and therefore love the combo punch.

alexpott’s picture

xjm’s picture

Assigned: xjm » Unassigned
FileSize
5.1 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,939 pass(es). View
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I was worried about early render and not having a render context however this is taken care of in renderPlain(). However given the complete lack of support for bubbling assets I think it is worth at last adding an assert() to deal with this.

+++ b/core/includes/bootstrap.inc
@@ -566,6 +566,14 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+    if (is_array($message)) {
+      $message = \Drupal::service('renderer')->renderPlain($message);
+    }

What you can do is something like this...

  if (is_array($message)) {
      $rendered_message = \Drupal::service('renderer')->renderPlain($message);
      assert('empty($message[\'#attached\']);', 'Render arrays passed to drupal_set_message() cannot have attached assets.');
      $message = $rendered_message;
  }

I'm not a huge fan of passing render arrays to dsm() but it seems like the best compromise to get the SafeMarkup issues resolved and is the least worst option we have.

alexpott’s picture

+++ b/core/includes/bootstrap.inc
@@ -514,10 +514,10 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
+ *   end with a period. A render array may also be provided for markup.

I think it is worth mentioning the attachment limitation here.

xjm’s picture

Assigned: Unassigned » xjm

On it.

catch’s picture

#50 doesn't help much, #attached can be at any level of the array.

This is why I prefer calling renderPlain() explicitly since that's obvious what's happening then.

xjm’s picture

@catch, #50 actually should work because (as @alexpott pointed out) the render call bubbles the #attached to the top level so that it can be checked. I'll provide a test case to demonstrate that it still works if #attached is in a lower level.

Wim Leers’s picture

#53 is indeed incorrect for the reasons @xjm already states in #54.

(It's great if somebody writes exactly what you want to write :P)

xjm’s picture

Assigned: xjm » Unassigned
FileSize
1.63 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,326 pass(es), 2 fail(s), and 0 exception(s). View
6.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,334 pass(es). View
3.51 KB

So I could not get the assert() stuff to work nor figure out how to test it. @alexpott said that assertions were enabled in simpletest but this did not seem to be the case, and specifically setting assert.active did not seem to work either. If we're going to include them, IMO, we need much better documentation of how to enable and test them. Anyway... attached raises an exception instead.

xjm’s picture

Status: Needs work » Needs review
xjm’s picture

+++ b/core/includes/bootstrap.inc
@@ -552,6 +553,10 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
+ *
+ * @throws \UnexpectedValueException
+ *   When passed a render array with #attached.

Extra blank line above this; could be fixed on commit.

xjm’s picture

Also, reading over the assertion documentation, this did not actually seem to be one of the recommended usecases for them anyway, but I probably just don't understand it well enough.

xjm’s picture

+++ b/core/includes/bootstrap.inc
@@ -566,6 +571,20 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+      // The rendering will have bubbled any #attached asserts to the top

Also, s/asserts/assets/ (lol).

xjm’s picture

+++ b/core/modules/system/src/Tests/Bootstrap/DrupalSetMessageTest.php
@@ -45,9 +54,36 @@ function testDrupalSetMessage() {
   }
 
+  /**
+   * Tests drupal_set_message() with disallowed #attach.
+   */
+  public function testAttach() {

Also there's actually no reason for this to be in a separate method and site install anymore; I was just trying that to get the assert() thing to work.

The last submitted patch, 56: dsm-2505497-56-FAIL.patch, failed testing.

xjm’s picture

FileSize
6.84 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,330 pass(es). View
2.13 KB

Just cleaning up #58, #60, and #61.

alexpott’s picture

FileSize
5.51 KB
7.82 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,340 pass(es). View

@xjm I'm pretty sure that assert() is perfect for this. If a module gets to production with an message with #attached in a render array we don't want the site collapsing because of this. This is developer error. Assertion testing is a bit painful at the moment because #2536560: Runtime Assertion unit and functional testing has not gone it. But we can still do it with a few @todo's.

I've run the test on PHP7 too and it passes there as well.

Wim Leers’s picture

Looks great, only nits.

  1. +++ b/core/includes/bootstrap.inc
    @@ -514,10 +514,11 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
    + *   #attached is not supported.
    

    s/supported./and #cache is ignored, since messages are never cached anyway./

  2. +++ b/core/includes/bootstrap.inc
    @@ -566,6 +567,18 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
    +      $rendered_message = \Drupal::service('renderer')->renderPlain($message);
    +      // The rendering will have bubbled any #attached assets to the top level
    +      // of $message. Assert that there are none.
    +      assert('empty($message[\'#attached\']);', 'Render arrays passed to drupal_set_message() cannot have attached assets.');
    

    Technically, "attached assets" is wrong, because it's not just about assets. So "attachments" would be more accurate.

    But this is probably okay.

  3. +++ b/core/modules/system/src/Tests/Bootstrap/DrupalSetMessageTest.php
    @@ -37,6 +37,15 @@ function testDrupalSetMessage() {
    +    $this->assertUniqueText('Render array with markup!');
    

    I'd never even heard of assertUniqueText()! This is very useful :)

  4. +++ b/core/modules/system/src/Tests/Bootstrap/DrupalSetMessageTest.php
    @@ -45,9 +54,25 @@ function testDrupalSetMessage() {
    +  }
     }
    

    Needs \n in between.

alexpott’s picture

FileSize
2.9 KB
7.93 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,344 pass(es). View

Thanks @Wim Leers.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

The assert() seems to me to be a good use-case here. The nits have been picked. Seems to be consensus around the solution. Back to RTBC.

xjm’s picture

+++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
@@ -134,6 +148,26 @@ public function drupalSetMessageTest() {
+    $old_assert_active = assert_options(ASSERT_ACTIVE, 1);

So this is exactly what I did last night... it didn't work. Did you confirm that this test fails properly when it should?

joelpittet’s picture

Should show my testing, sorry. I made sure asserts were turned on in local.settings.php (from the example.local.settings.php) Rebuilt the container, enabled the "system test" module (there are two, I just enabled both...)

Then I hit up /system-test/drupal-set-message-fail

xjm’s picture

Okay I did actually confirm locally that the test coverage works:

diff --git a/core/includes/bootstrap.inc b/core/includes/bootstrap.inc
index ced9c4c..48d6aea 100644
--- a/core/includes/bootstrap.inc
+++ b/core/includes/bootstrap.inc
@@ -576,7 +576,7 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
       $rendered_message = \Drupal::service('renderer')->renderPlain($message);
       // The rendering will have all attachments bubbled to #attached in the top
       // level of $message. Assert that there are none.
-      assert('empty($message[\'#attached\']);', 'Render arrays passed to drupal_set_message() cannot have attachments.');
+      // assert('empty($message[\'#attached\']);', 'Render arrays passed to drupal_set_message() cannot have attachments.');
       $message = $rendered_message;
     }

Results in:

Fail      Other      DrupalSetMessageT   64 Drupal\system\Tests\Bootstrap\Drupa
    "Render arrays passed to drupal_set_message() cannot have attachments."
    found

Cool. Not sure what I was missing before. I'm still iffy about using these in general, but I guess we're mostly just using a PHP language feature for the most part at this point.

Thanks @alexpott and @joelpittet.

Aki Tendo’s picture

FileSize
6.83 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,469 pass(es). View
4.83 KB

Followup to #66 - this adjusts the tests to a form as close as possible to the final one they'll have after #2536560: Runtime Assertion unit and functional testing is finished. This also simplifies the test considerably.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @Aki Tendo! From your change, I can see that the goal is to bring PHP 5 behavior more in line with what PHP 7 does (throwing a typed exception when assertions are enabled). That makes sense I guess as future-proofing.

Some specific feedback:

  1. +++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
    @@ -126,10 +138,41 @@ public function drupalSetMessageTest() {
    +    # catch (\AssertionError $e) {
    

    In general we don't commit commented-out code; instead, we attach the patch to a dedicated followup issue. (In this case that'd be an issue postponed on #2536560: Runtime Assertion unit and functional testing.)

  2. +++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
    @@ -126,10 +138,41 @@ public function drupalSetMessageTest() {
    +    // End block to be removed.
    

    We also don't use inline comments at the end of code blocks in general. Better to just put the diff deleting these lines in a followup issue.

  3. +++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
    @@ -126,10 +138,41 @@ public function drupalSetMessageTest() {
    +      // If the exception is the not the one we expect, rethrow causing a test
    +      // failure.
    +      if ($e->getMessage() !== 'Render arrays passed to drupal_set_message() cannot have attachments.') {
    +        throw $e;
    +      }
    +      else {
    +        drupal_set_message('Correct runtime assertion thrown when render array contains #attached.');
    +      }
    

    This actually seems... not correct to me. This is a test specifically for drupal_set_message(), so it doesn't seem correct to me to also use drupal_set_message() for reporting the results of testing itself. I think it'd be more correct to just throw the exception and catch, rethrow if the message doesn't match, and $this->fail() if the exception is not present within the test itself.

All that said, I'm increasingly uncomfortable with introducing assertions in this patch, because:

  • We clearly do not yet have standard practices for how to use assertions in core nor how to handle them in the test runner. I know @Aki Tendo and others are working hard to define those best practices, but we are not there yet, so introducing assertions in an unrelated issue before that's done is premature and might require even more rework later.
  • Furthermore, as there aren't any assertions in core yet, introducing them only in one place is likely to mean developers just miss it, because developers are not going to expect them nor see any effect when they enable them in settings.local.php. I feel that it'd be better to simply file a followup issue that states we think this could be a correct use of assertions and include something like @alexpott and @Aki Tendo's work in that followup, as part of a concerted effort to add assertions later on.
  • I still have concerns about the assertion work in 8.0.x from a release management perspective, and the more I talk to different developers about how assertions would be used and what they gain us, the more it seems that people have very different expectations about them. Introducing new patterns in the beta, especially such a late beta, adds risk and potential technical debt. But as stated above, this is not in scope for this issue, and so shouldn't block it (nor be discussed here really, so I'll try to follow up about that elsewhere with the framework managers).
  • This issue is soft-blocking several children of a critical, and trying to figure out how to use and test assertions is delaying it. It'd be better to just make this change using current practices if we are making it at all.

So I'd really prefer to go back to the patch in #63, or even #49 if we really don't want this to cause an exception on production environments. Anything else is just not in scope. We can create a followup to explore assertion use, or maybe even a meta issue to track this and other places in core where we should consider assertions, but we shouldn't do it in this patch. In fact, #49 seems more correct because there are no side effects if a developer adds #attached to their render array other than it just does nothing.

effulgentsia’s picture

We do have one other assert() in HEAD already, introduced in #2544954: SqlContentEntityStorageSchema does not detect column-level schema changes in non-base-fields. But, if there are concerns about continuing that practice, then I think removing that from this patch and having a @todo to either add an assertion or an exception in a followup would be fine.

Aki Tendo’s picture

FileSize
5.55 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,456 pass(es). View
2.73 KB

If I may be so bold, we aren't talking about the world's most complex assertion here. Without the error message parameter it's just an empty() call.

assert('empty($message[\'#attached\']);');

I personally don't think this should be unit tested. A discussion and consensus needs to be made on this but in short unit testing an assert() call is the same as writing a unit test for a unit test. It's very redundant to do this. While this is necessary sometimes I don't think it's necessary here because of the simplicity of what is being asserted. Writing a unit test for this is like writing a unit test for PHP Type hints, and we don't do that.

In that vein attached is a patch that just lets the assert() be its own test without further examination. This makes progress on #2536560: Runtime Assertion unit and functional testing moot.

Aki Tendo’s picture

Status: Needs work » Needs review
xjm’s picture

Thanks @Aki Tendo. That seems sensible.

I'm fine with the patch in #74 I guess... Currently it's functionally more or less the same as #49. It's just adding a line of code that for now is not going to mean much to most Drupalists or have any affect on their sites. But they can look it up and read the PHP docs, and then maybe find the Drupal API docs on it.

In general though we do test expected exceptions.

Leaving at NR so that others can decide what they want to do with this.

effulgentsia’s picture

We added an assert() with no test coverage in #2544954: SqlContentEntityStorageSchema does not detect column-level schema changes in non-base-fields so I think it's fine to do so here as well. But, I think it would be better to add a @todo above both assert() statements to add test coverage for them in #2536560: Runtime Assertion unit and functional testing and to do so for all future assert() statements until that issue lands. I think it makes sense to treat assert statements more like exceptions than like type-hints, and therefore add test coverage for them when that becomes possible.

Aki Tendo’s picture

In general though we do test expected exceptions.

The thing is though, a runtime assertion is not an expected exception because there is nothing the end user (including administrators) can do to cause one to be failed (if there is then the runtime assertion mechanism is not appropriate). Only other developers run that risk by making illegal function calls to the API.

Sometime this evening I'll write a separate planning issue ticket up on this but for now, in brief I feel that an assert() should only need to be tested in these circumstances

1. When the assert uses a new function test - such as a theoretical function to examine the render array entirely and see if it is properly formatted. Even then, it's probably not the assert calls that should be unit tested but the testing function itself to see if it returns true or false correctly.

2. When the assert failure can be brought about by a commonly changed module config.yml file change. This is important because when PHP throws something like "call to member function on non-object" a beginning developer's thought isn't going to think to check the yml file first. I know about this particular gotcha from personal experience.

The following circumstances should never need to be unit tested.

1. Any assert that is performing a type hint that PHP 7 will be used for in the future - like is_string.
2. Any assert on a return type because, again, PHP 7 will be able to do this without assert() and unless we want to start unit testing all the type hints in the system it's inconsistent to just do a handful.
3. Any assert on a parameter tested by Drupal\Component\Assertion\Inspector. After all, that component already has a unit test on its returns, so any further testing is redundant.

I'm sure there's a few cases that lie in the gray area between these two and they should be handled on a case by case basis.

xjm’s picture

Thanks @Aki Tendo and @effulgentsia.

The thing is though, a runtime assertion is not an expected exception because there is nothing the end user (including administrators) can do to cause one to be failed (if there is then the runtime assertion mechanism is not appropriate). Only other developers run that risk by making illegal function calls to the API.

This doesn't make sense to me because there should also be nothing the end users can do to raise exceptions. They should only happen when, theoretically, the developer has made some kind of mistake. End users should only be able to trigger non-exceptions like validation errors.

But again this is out of scope for this issue. So I've filed #2548671: [policy, no patch] Define best practices for using and testing assertions and document them before adding assertions to core.

catch’s picture

If we just said 'use renderPlain directly', then we'd need neither an exception nor an assertion since the documented behaviour of renderPlain() is to eat attachments.

Since both modules and themes can add assets to any render array with an element type/#theme in it via alter and preprocess, it's quite possible that something as simple as rendering something via #type item_list could end up throwing an exception with one theme installed but not another. So I think we should either:

- just say use renderPlain() directly since it provides an API exactly designed for this use case (and e-mails, browser notification messages etc.) which was my suggestion in #34

- add the same documentation to drupal_set_message()

catch’s picture

So the 99% case for this issue is to pass lists to dsm().

If someone adds #attached to #type item_list then that will end up triggering this error (assuming we use #item_list in core which I think is the point).

Whose fault is it? The person who passed the render array to dsm() (after we told them to by adding this API)?

Or the person who added #attached to item_list - which is also completely allowed?

There is no developer error in either case, so we should not make one.

Aki Tendo’s picture

This doesn't make sense to me because there should also be nothing the end users can do to raise exceptions. They should only happen when, theoretically, the developer has made some kind of mistake. End users should only be able to trigger non-exceptions like validation errors.

I'm sorry but that's wrong. Exceptions get raised all the time - that's the reason for the try/catch block.

For example, validation should be handled in the following manner - the data is passed to the validator. The validator returns the sanitized data if it can, but if it can't it should throw a validation exception. This exception gets caught by the controller which then prepares a response for the end user.

While it's true a lot of PHP code has the validator return FALSE if it can't validate the data, this is a holdover from the days of PHP 4 when exceptions weren't around. Having a validator return false is incorrect because false is a possible value for an input in many circumstances (and if not false itself, a value that loosely eval's so, such as empty string, zero or NULL).

Exceptions are for exceptional circumstances and are a supplement to control structures, allowing multiple levels of a call stack to be avoided when something unusual happens. Granted, some programmers abuse try/catch to the point of using it where if/else makes more sense, but I haven't seen any instances of this in Drupal (yet - it's a large codebase).

Theoretically an exception should never escape to the final exception handler - if one does that's usually a bug - but to say that exceptions are always the developer's fault is flat out wrong. One common example of an exception throw that doesn't have a custom handler are Database Exceptions - particularly connectivity exceptions. I've seen these escape in Drupal, but it takes shutting down the SQL server unexpectedly to do it. And really, what can be done about that other than presenting a sensible error screen? We can't fix it - the admin of the system has to.

The assert() statement is for situations that cannot arise in production because they require a code level error to occur - a call to a function with the wrong datatype being the most common case. When in doubt use an exception - and with this particular issue there seems to be considerable doubt. If the API has allowed attachments before now then that means code could be out there that expects to be allowed to pass one - we should NOT be using an assert here if that is the case.

alexpott’s picture

@catch is right. You can even add attachments in the .twig file. I was wrong about using assert() here. Sorry all. We need to either document this on dsm() or ask developers to call renderPlain - I'm in favour of the latter because then the developer has to think about what they are rendering before shoving any old thing into dsm().

xjm’s picture

@catch, so the patch does currently use renderPlain() directly and has since #42. And it's already documented in the parameter documentation that #attach won't work. Are you saying we should just state that explicitly in the parameter documentation that it uses renderPlain()? (Edited.)

catch’s picture

Status: Needs review » Needs work
alexpott’s picture

Also I think this whole discussion of #attached shows that there will be issues with dsm() doing renders. I originally thought this issue was a good idea but I no longer no. There is never a time when we can't do the early render in the calling code. For example, in #2505499: Explicitly support render arrays in hook_requirements() we can gather the requirements in the installer and render them before passing to dsm().

The calling code knows what it is doing, adding the render array functionality means that if you see a message on the screen and have no idea where it's coming from you have to worrying the input params being render arrays and not just plain old message strings. Entangling rendering and drupal_set_message is adding unnecessary complexity for no reason. The calling code has the complexity and should deal with it.

xjm’s picture

Status: Needs work » Needs review
FileSize
6.85 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,519 pass(es), 1 fail(s), and 0 exception(s). View
2.13 KB

Not sure if the status change actually answers my question, so just uploading what I thought you meant.

xjm’s picture

My concern with doing early rendering in the calling code is that then we teach people that the way that they solve their double-escaping problems is to render early, and we all know how Drupal developers love to over-apply a pattern.

The calling code knows what it is doing, adding the render array functionality means that if you see a message on the screen and have no idea where it's coming from you have to worrying the input params being render arrays and not just plain old message strings. Entangling rendering and drupal_set_message is adding unnecessary complexity for no reason. The calling code has the complexity and should deal with it.

None of these statements seem true to me. There is no unnecessary added complexity. It's one is_array() check and a renderPlain() that, as stated, strips away any complexity. Calling renderPlain() in the caller or in dsm() is nearly identical, especially now that it is rendered down to a string before interacting with $_SESSION or anything.

catch’s picture

@xjm

#84 no I mean the cases that need to render complex things into messages should use renderPlain directly themselves - those are very few, complex cases, and they already are building up the render array (or doing weird string concatenation if they're not). Supporting it generically adds complexity to drupal_set_message() that doesn't need to/should not be there. Then drupal_set_message() can just deal with strings (or string and SafeString).

#88: it's not early rendering. renderPlain() is a very specific API we added for the sole purpose getting a render array and transforming it to a string with no #attached support, and it's well documented when to use it - which is for cases like this.

None of these statements seem true to me. There is no unnecessary added complexity. It's one is_array() check and a renderPlain() that, as stated, strips away any complexity. Calling renderPlain() in the caller or in dsm() is nearly identical.

Every call to drupal_set_message() will trigger the additional code execution.
Every reader of drupal_set_message() documentation will read the additional argument type despite the 99.9% case being not to need it.

xjm’s picture

Alright. If you both thought this was a wontfix and are going to continue to block it, it would have been great if that had been said before taking a week of my time.

I disagree with your reasoning and I feel there is a huge risk in adding patterns where core returns render API calls of render arrays instead of render arrays themselves. I think that render arrays should be the base units of markup and output before getting boiled down into strings. I think this is the wrong decision.

But I have lots of other issues that have been blocked on this.

xjm’s picture

Status: Needs review » Closed (won't fix)

Status: Closed (won't fix) » Needs work

The last submitted patch, 87: dsm-2505497-86.patch, failed testing.

catch’s picture

Status: Needs work » Closed (won't fix)

I said all this on August 5th which was nearly a week ago and the first I was made aware of the patch. I asked for use cases and brought up concerns with supporting render arrays (which got further confirmed the more the argument went on). Not sure how I could have done that differently except for perhaps going direct from RTBC to won't fix without giving anyone a chance to respond to my points.

I think that render arrays should be the base units of markup and output before getting boiled down into strings. I think this is the wrong decision.

There's a massive difference between this and other render calls though.

When you 'early render', you're taking a render array, rendering it to a string, relying on render contexts and similar to handle assets and cacheability for you, and then passing it up into a different render array to be rendered as part of the page. Eventually we want to get rid of that entirely, but unfortunately got nowhere near that point for 8.x.

When you renderPlain(), you're taking a render array, rendering it to a string, throwing away the assets, and passing it to something that does not deal with render arrays at all and may not render to either HTML and especially not in the same page.

E-mails are completely outside page rendering altogether.

drupal_set_message() may or may not be on the page that triggers the message (could be the next fully themed request). Or it might not be rendered on a Drupal page as such at all - there are 7.x contrib modules that take dsm() and put it into growl - list at https://groups.drupal.org/node/51088 includes more than one.

So while in 9.x we might want to completely remove support for early rendering, we won't and can't remove support for taking a render array and converting it to a raw string with no assets (or not without a new replacement for that use case). To me they're completely different things.

xjm’s picture

@catch you said several times "call renderPlain() explicitly". I updated the patch to do that. I thought I was following the feedback given. I did not understand that you meant this issue should be entirely wontfix.

effulgentsia’s picture

I don't see how "won't fix"'ing this answers #81. In HEAD, we currently have file_save_upload() doing this:

$message = array(
  'item_list' => array(
    '#theme' => 'item_list',
    '#items' => $errors,
  ),
);
drupal_set_message(\Drupal::service('renderer')->renderPlain($message), 'error');

So, if a theme overrides item-list.html.twig and adds a {{ attach_library('mytheme/fancy-lists') }} to it, what should happen?

Option 1: That's ok, no assertion needed, just show the message without the item list being fancy.

Option 2: That's not ok, add an assertion, and document system/templates/item-list.html.twig as being a common template for rendering things out of page context and that therefore it is a bug for themes to add attachments to that particular template.

Regardless of whether we pick option 1 or 2, I don't see anything becoming worse by moving the renderPlain() into the dsm() rather than requiring each caller to do it.

But, if we do move the renderPlain() into the dsm() (as #87 does), then we have the option of at some point picking an option 3: add the #attached (if there is any) to the session data and restore it in drupal_get_messages().

Every call to drupal_set_message() will trigger the additional code execution.

drupal_set_message() is never in a critical performance path. When would we ever call it thousands of times in the same request and expect that request to be performant? IMO, adding an is_array() check there seems like no big deal whatsoever.

Wim Leers’s picture

Status: Closed (won't fix) » Active

Back to "active" for #95.

alexpott’s picture

@effulgentsia for me performance has never been part of the discussion. The more important thing to me is where different responsibilities lie - should we entangle rendering with drupal_set_message or not. I'd rather not. It makes what code can called by calling drupal_set_message() impossible to predict. For example, recursive drupal_set_message() becomes possible. With respect to the quite common sub messages pattern, I think we should consider alternate solutions similar to #2546280: Add sub messages capability to drupal_set_message by using a Message class

catch’s picture

Yes the is_array() is not a performance issue, it's that there are now two different code paths, one of which has almost unlimited dependencies. We're talking about at most a dozen places in core that could use the new feature, and instead need an extra line (or inlined method call even), compared to many, many other places that call only with a string.

I was responding to

calling renderPlain() in the caller or in dsm() is nearly identical

Taking what has been a very simple bare bones system for years and adding the full weight of the rendering system to it is not 'nearly identical'.

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Active » Postponed

Pushing this up to 8.1.x for it's remaining discussions.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.