berdir_wim_drupal_render_root_discussion.txt

11:27:10 berdir: WimLeers: ping
11:27:25 WimLeers: berdir: pong
11:28:38 berdir: WimLeers: did you see my e-mail? ;)
11:28:47 WimLeers: I did not
11:28:56 WimLeers: I'm very far behind on both personal and Acquia e-mail.
11:29:04 WimLeers: sorry :(
11:29:17 berdir: WimLeers: np, I just asked if I can ask you a question :p
11:29:28 WimLeers: haha
11:29:32 WimLeers: oh there it is
11:29:37 WimLeers: shoot!
11:29:44 WimLeers: berdir: check_markup() problem
11:29:46 WimLeers: interesting
11:29:47 berdir: I'm fighting with drupal_render_root() :(
11:29:49 WimLeers: hehehe
11:30:02 WimLeers: berdir: do you have recursive check_markup() calls?
11:30:17 berdir: no, just one
11:30:29 berdir: but that gives me that logic exception..
11:30:40 WimLeers: But it's happening inside another drupal_render_root()?
11:30:48 berdir: probably yes
11:30:49 WimLeers: aspilicious ran into the same problem with DS
11:30:53 berdir: but that's nothing I can do anything about
11:30:57 WimLeers: ok
11:31:00 WimLeers: there's a simple solution
11:31:17 berdir: I'm inside hook_tokens(), calling ->processed on a text item, that calls check_markup(), boom
11:31:30 WimLeers: check_markup() is designed to be used only when filtering text for non-HTML purposes.
11:31:32 berdir: I mean the information about loosing assets is nice and everything, but for my use case.. I don't really care :)
11:31:37 WimLeers:  * Note: this function should only be used when filtering text for use elsewhere

 * than on a rendered HTML page. If this is part of a HTML page, then a

 * renderable array with a #type 'processed_text' element should be used instead

 * of this, because that will allow cache tags to be set and bubbled up, assets

 * to be loaded and #post_render_cache callbacks to be associated. In other

 * words: if you are presenting the filtered text in a HTML page, the only way

 * this will be presented correctly, is by using the 'processed_text' element.


11:31:41 berdir: I've seen that
11:31:47 berdir: does not help me
11:31:52 berdir: I'm not calliing check_marup()
11:31:56 berdir: marup ;)
11:32:04 berdir: I'm calling ->processed, TextProcessed calls it
11:32:10 WimLeers: hrmmmmm
11:32:16 WimLeers: interesting
11:32:42 berdir: sure, I could duplicate the code there and do it myself, but then we can honestly just as well remove ->processed because it is .. not very useful anymore ;)
11:32:44 WimLeers: I was going to say to just duplicate the $build array from check_markup() and call drupal_render() on it
11:32:51 WimLeers: yep agreed
11:33:20 berdir: core does this as well, see node_tokens(), I guess we just don't have any tests that call this inside a render context
11:33:36 WimLeers: So in a drupal_render_root() call, accessing →processed essentially does never work
11:33:56 berdir: yes
11:34:03 berdir: I also have a second example, contact_mail()
11:34:13 berdir: I have a field formatter that displays a contact form
11:34:50 berdir: that then sends a mail when the form is submitted. same error
11:35:02 berdir: even less chance of doing anything about it :)
11:35:56 WimLeers: Thanks, 2 examples are better than 1
11:36:49 WimLeers: Can you explain the contacvt_mail() example? The error only occurs when the form is submitted, right?
11:37:06 berdir: yes
11:37:37 berdir: but the form is in the formatter, and the formatter is only called when the node is rendered. which is not great obviously, but that's how it works now..
11:38:59 WimLeers: But then why does it fail when the contact form is in a formatter, and why doesn't it fail for /contact ?
11:39:56 berdir: because that is _form, and directly called from the routing system
11:40:00 berdir: I'm called from the renderer
11:40:02 berdir: see https://gist.github.com/Berdir/6e2f8036e89cc8459710
11:40:32 WimLeers: damn
11:41:12 WimLeers: the only way that could possibly be made to work then is for contact_mail() to get the stack, store it in a temporary variable, call drupal_render_root() with a fresh stack, then restore the stack
11:41:48 WimLeers: It'd be okay if only contact_mail() had to do that
11:41:53 berdir: I mean... it's rendering stuff for an email. even if there would be assets, I would not want to have them :)
11:41:54 WimLeers: But the question is of course: is this a more general problem?
11:42:01 WimLeers: exactly
11:42:13 WimLeers: but you do want #post_render_cache callbacks to be executed
11:42:32 berdir: true
alexpott [[email protected]/user/157725/view] entered the room. (11:42:58)
11:43:02 WimLeers: Hrm I have an idea
11:43:06 WimLeers: But it might be stupid
11:45:37 WimLeers: berdir: https://gist.github.com/wimleers/a47334ef80b492997157
11:48:03 WimLeers: So far, we've been thinking from the angle "we MUST NOT support nested render root calls, show an error instead!". But this shows that there are use cases where it's necessary, even if it's only happening because of how Drupal is architected in other parts (i.e. theoretically that e-mail should be sent after the rendering, but doing that would probably require significant changes in other parts of D8). With this patch, we're saying that it's okay to have nested render root calls, but… they will not bubble up. It's indicating a strict boundary beyond which no bubbling happens. That is sufficiently sensible too.
11:49:23 WimLeers: We should've had this discussion in #drupal-performance, where Fabian would've had some ideas too probably
11:49:40 WimLeers: I can c/p this conversation later on, no worries
11:49:51 WimLeers: berdir: did you have a chance to try that?
11:50:07 WimLeers: AFAICT this also cleanly solves the check_markup() case