Broken title in modal dialog when title is a render array

Tested the patch and it does what it should. Apart from Testbot thinking it doesn't, of course. :)

I've had another case where a non-view related context caused a similar problem. Digging a tiny bit deeper, I realized that the DialogRenderer and its child ModalRenderer may return a render array for the title. Nothing wrong with it, except that the OpenDialogCommand expects a string for the $title parameter (unlike Content, which may be a render array).

So a trully global solution to the problem may be quite simple: check in OpenDialogCommand if the title is an array and render it immediately. This should solve most issues with these kinds of problems.

Let's see if the Testbot will complain.

Well that was rude.

Ok, fixed a bug where I'm an idiot, let's try this again.

I can confirm that the patch in #9 corrects this issue on 8.3-beta1. Modal windows for views are now displaying appropriate titles. No more array->string notices in the log. Changing the component to "ajax system" since this isn't technically a bug with views.

Patch from #9 also working against Drupal Version 8.3.0-rc1

I'm wondering whether the better alternative would be to ensure there is no array passed to it in the first place?

Seeing as Views is one of the pages that sends titles as an array, and it's buried DEEP in core now, I'd assume that there is a good reason that the titles are being sent as a render array instead of a string. While it may be better to have consistency, I think this array to string conversion patch would be a much smaller impact given that the 8.3.x branch is in the RC phase as changing views and other's title handling might be considered a more disruptive change. Just my $0.02...

Yeah I strongly doubt this will land in 8.3 before the release, there is nothing fundamentally broken without it.

Rerolled the patch, please review.

Hello everyone, i've tested the patch on Drupal 8.3.6-dev and on Drupal 8.2.5 and it works on both environments like a charm.

Hello everyone, i've tested the patch 2663316-16-broken_view_title_in_modal_dialog.patch on Drupal 8.3.5. patch 2663316-16-broken_view_title_in_modal_dialog.patch works fine.

#16 worked for me, thanks Pavan

Patch from #16 works fine! :)

I'm wondering whether the better alternative would be to ensure there is no array passed to it in the first place?

Indeed. With the current state of things #16 looks like a possibly unneeded/unwanted API change in OpenDialogCommand. Also, it might be better to fix all instances of this bug in one patch. Be it in the Commands or in the Renderers.

Docs state that these constructors get strings passed for title. So I guess \Drupal\Core\Render\MainContent\DialogRenderer::renderResponse() is doing it wrong. Same for ModalRenderer::renderResponse() and probably other renderers that call these type of commands.

@Eric_A: Drupal expects either a render array or a string to be passed almost everywhere before an item is rendered. To expect only a string here would deviate from this expectation. We would need to modify almost every location where the modals are used today.

Tested on 8.5.3, #16 worked like a charm.

#14 & #24 make great counter points to #23.

I discussed on Slack with @alexpott and we agreed that it would be better if we could move the responsibility for the caller (Drupal\Core\Render\MainContent\DialogRenderer). However, we can't do that because our current APIs don't allow passing attachments into commands. We can take inspiration from \Drupal\Core\Ajax\CommandWithAttachedAssetsTrait::getRenderedContent() which already collects attachments while rendering the main content.

Added test for when the $title parameter of OpenDialogCommand() is an array. (and minor correction to docblock)

This should help.

What was your intention, @gawaksh? It seems you have simply removed the test.

Here is the test with the container provided.

@gawaksh your patch is no different than #16. Also, you accidentally deleted the closing bracket on the construct, which should throw an error.

As #26 mentions, the core people want to take a different approach than #16 to fix it.

Returning to Needs Review for patch in #33.

@lauriii, if you are requesting an alternative approach (in #26) then can you provide more details, please?

The problem is that render arrays might contain attachments for assets, but the current approach doesn't collect the assets and include the information in the ajax command.

This render array requires the title library from system module, which could be missing unless we collect the attachments from the render array.

$title = [
  '#markup' => '<span class="title">Home page</span>',
  ''#attached' => [
    'library' => ['system/title'],
  ],
];

Luckily, this is not the first time we do rendering in ajax commands, so we can take inspiration from the pre-existing code. For example, \Drupal\Core\Ajax\CommandWithAttachedAssetsTrait::getRenderedContent() already does this while rendering the main content.

I added the needs tests tag back since we are still missing test coverage for this use case.

@lauriii before I write the new test(s), can you confirm that I understand you suggestion, please.

:# Embarrassing copy and paste error!

MethodProphecy cannot handle pass-by-reference parameters to converted to InvocationMocker (and pass-by-reference parameter value set).

How about rendering the title in render() and not in __construct(). I also think we could add a followup to deprecate \Drupal\Core\Ajax\OpenDialogCommand::getDialogOptions() it's not used in core and I'm not sure it is needed. But I think it is okay that this returns the un-rendered title.

I ran into this while working on #2962525: Create a field widget for the Media library module - the fix here is similar to what I was writing before I found this issue.

The current patch doesn't test the collected assets from the title yet, so we need to address that. Sorry for removing the tag.

The logic needs updated as well per #44 - the getRenderedContent() call in render() will override any assets collected in the constructor.

Working on this.

Test inclusion of assets from title.
Render the title in render().

Sorry, @samuel.mortenson, I should've tagged that I was working on this.

Hopefully between the 2 of us we can produce a satisfactory patch. TBH I'm not happy with mine, but it's time to go home!

Cross posted! It's alright @jo-fitzgerald - my patch is pretty similar but I'll upload it anyway. :-)

The difference between #48 and #50 is that I used NestedArray::mergeDeep and test coverage for that, and added a new method to the \Drupal\Core\Ajax\CommandWithAttachedAssetsTrait trait in case other commands are doing multiple renders.

It looks like \Drupal\Core\Ajax\AjaxResponse didn't handle the case where getAttachedAssets() was NULL, even though that's documented as a valid return.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.php
@@ -128,6 +128,15 @@ public function setDialogTitle($title) {
+      $rendered_title = \Drupal::service('renderer')->renderPlain($this->dialogOptions['title']);
+      $this->addAttachedAssets(AttachedAssets::createFromRenderArray($this->dialogOptions['title']));
+      $this->dialogOptions['title'] = $rendered_title;

I'm curious, do we care about the other cacheability metadata from the rendered title, like cache tags?

@dawehner That's a good question - looking through other parts of core I can only see caching logic around the main content, not the title if it's a render array, but someone more experienced should confirm this.

The logic being updated here was introduced in #2347469: Rendering forms in AjaxResponses does not attach assets automatically (CR: https://www.drupal.org/node/1911578).

I'm curious, do we care about the other cacheability metadata from the rendered title, like cache tags?

Great question! AJAX commands are never cacheable, nor are AJAX responses and have therefore never cared about cacheability. Maybe we'll change this in the future, but until then, cacheability won't matter.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.php
@@ -128,6 +128,15 @@ public function setDialogTitle($title) {
+      $this->addAttachedAssets(AttachedAssets::createFromRenderArray($this->dialogOptions['title']));

When does a rendered title have attached assets? If you look at \Drupal\Core\Render\MainContent\HtmlRenderer::prepare(), you can see that either we call the title resolver service (for which \Drupal\Core\Controller\TitleResolverInterface documents that only #markup and #allowed_tags should be used in case of returning a render array) or just a string.

Note:

this reminds me of the issue(s) in which we aimed to simplify the resolving of a title, part of that IIRC was ensuring that it's always just a string, and never a render array. IIRC we got very close, but not 100%. It's sad to see that we see the consequence of that here too. See #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks and #2403359: Use _title and _title_callback where possible.

Would be good to get @lauriii's response to #56 since they requested the title attachment handling.

When does a rendered title have attached assets?
...
\Drupal\Core\Controller\TitleResolverInterface documents that only #markup and #allowed_tags should be used in case of returning a render array

I read the documentation and it seems more like an example, rather than limitation. I didn't find any use case in core where attachments would be added to a title but theoretically it should be possible and might be used by other projects.

When does a rendered title have attached assets?

Theoretically? Javascript that renders a Help button if Help exists for the given page/title. Not how I would implement that, but theoretically it would be possible and according to \Drupal\Core\Controller\TitleResolverInterface, it isn't wrong.

I just closed #3005201: Ajax Error when Browsing media filed by @jasonschweb and #3012207: Notice: Array to string conversion in Drupal\Component\Render\PlainTextOutput::renderFromHtml() filed by @robin.ingelbrecht as duplicates of this issue. Thanks also to @harrrrrrr, who already mentioned this issue as a possible solution in the first issue.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.php
@@ -128,6 +128,15 @@ public function setDialogTitle($title) {
+    $this->dialogOptions['title'] = PlainTextOutput::renderFromHtml($this->dialogOptions['title']);

Why are we running the title through PlainTextOutput::renderFromHtml? It seems like there's no point in attaching assets if we do this since there's no HTML rendered as a result of the render array.

If the title is supposed to be plain text, should we instead remove instances titles returned as render arrays and document that it's no longer supported?

I confirm the patch #52 is working as expected on Drupal 8.6.4 & 8.6.5!

#52 is working as expected for me on Drupal 8.6.4 with no issues. Thank you!

#58:

I read the documentation and it seems more like an example, rather than limitation. I didn't find any use case in core where attachments would be added to a title but theoretically it should be possible and might be used by other projects.

Right, it's not a hard limitation. But I don't think it makes sense to let OpenDialogCommand support attachments to titles if even HtmlRenderer::prepare() (used to turn every render array received from a controller into a HTML response!) does not support that. I think it makes sense to keep feature parity with that. Allowing attachments is merely out of scope IMHO. Same for #54's remark about cacheability.

We can choose to support that, but that should then happen everywhere where page titles are generated or rendered. Which IMHO means it should be a separate issue, since it's a separate scope. Based on that, I'm marking this Needs work, to remove the asset handling portion.

Why are we running the title through PlainTextOutput::renderFromHtml?

I just used git blame to answer that. This is already used in HEAD, since #2207247: Dialog titles double escaped for views handlers and delete forms.

Patch #52 works for Drupal 8.6.10 and resolves the issue.

Just a notice if someone else has the same issue:
I realized (after 8 hours of frustrated debugging) that it doesn't work without a proper domain name. I was using http://mydomain.com/~myworkspace as URL to our hosting provider, getting the error all the time.
After adding proper DNS records (or using /etc/hosts) everything worked after patching #52 (don't know if it worked before patching...).
(I'm using Drupal 8.6.10).

Patch from #52 fixes issue for me in Drupal 8.6.10

Updated patch for Drupal 8.7 (and 8.8).

Confirming patch #70 is working for Drupal 8.7.1 as below:

patch -p1 < 2663316-70.patch

patching file core/lib/Drupal/Core/Ajax/AjaxResponse.php
patching file core/lib/Drupal/Core/Ajax/CommandWithAttachedAssetsTrait.php
patching file core/lib/Drupal/Core/Ajax/OpenDialogCommand.php
patching file core/tests/Drupal/Tests/Core/Ajax/AjaxCommandsTest.php
Hunk #2 succeeded at 353 (offset 57 lines).

Setting back to Needs Review & Triggering bots.

I can confirm that patch #70 is working for Drupal 8.7.5 correctly.

#70 Patch Failed to Apply on 8.8.x.

Patch rerolled for 8.8.x.

I can confirm that patch in #76 works for Drupal 8.8.
Is there any chance that the patch gets committed? It has been almost four years now. Good years though. By the way, happy New Year to everybody!

So we have multiple confirmations that the patch worksforthem and the patch contains tests. So strongly looks like RTBC.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.php
@@ -57,8 +58,8 @@ class OpenDialogCommand implements CommandInterface, CommandWithAttachedAssetsIn
+   * @param string|array $title

@@ -128,6 +128,15 @@ public function setDialogTitle($title) {
+    $this->dialogOptions['title'] = PlainTextOutput::renderFromHtml($this->dialogOptions['title']);

It seems a bit odd to me that we allow passing a render array here given that we actually want a plain string. How about we type hint this as string and fix all the instances that are passing an array here? This seems like a bit pain to fix now, but in the end it would lead into better DX because there's less room for confusion.

I talked about this with @alexpott and one interesting valid use case for markup in titles is <bdo> tag. I don't think it's something we can support easily that at the moment, so it might be something we want to research in a follow-up.

I don't disagree that it would be better to fix all instances passing an array, but this patch is a big step forward. Could we move this to RTBC and then make a child issue to clean up all the places that are passing an array?

I triggered tests for 9.2.x branch to see if the latest patch still applies.
Also tagged the issue with "Bug Smash Initiative" trying to bring more people to the discussion.

#76 forking for me on 9.1.8

Working for me. My use case is a Views page and we alter a link to open it in a dialog. The Views title gets pulled into the dialog and shows as just "Array". Patch at #76 is working for 9.2.6.

Regarding comment at #79, to me this a simpler solution than requiring upstream changes. It could be argued that we should explore DialogRenderer::renderResponse as the place to render to pain text, but I'm not sure if all callers go through that path to get to OpenDialogCommand. DialogRenderer::renderResponse is where the Views title is placed into a parameter for OpenDialogCommand.

In my mind, making OpenDialogCommand handle plain text or a render array is only a benefit.

Worked for me, at least the errors are gone from logs.

Using Views in NG lightbox

Triaged at DrupalSouth with camerongreen and silverham.
I have added the issue summary update which needs to be completed. This does have a test. Does this perhaps need manual testing? This also needs steps to reproduce starting at install Drupal Core. Also, before and after screenshots are needed, they are to be added to the Issue Summary. It looks comment #79 needs to be resolved (but it is late and I the sprint is ending).

The patch still applies to 10.1.x but I did not test it or run the test locally.

Latest patch at #76 - applies cleanly and tested on:
- 9.4.6
- 9.5.x-dev
- 10.0.0-beta2
- 10.1.x-dev

Tested attachments manually (via hook_element_info_alter()) by adding drupalSettings and libraries - works as expected. Automated tested also include this.

Summary updated with before and after screenshots and reproduce steps.

I suggest we keep current functionality of rendering the render array as is (updated in summary):
- Keep rendering assets / libraries so behavior works as expected.
- Consider follow up ticket to determine if views system title should be changed to output string instead of array.
- Consider follow up ticket to determine if NOT processing attachments in title should be done.

Setting to RTBC as multiple users have also tested this. Flagging @quietonen and @camerongreen to raise awareness.
Keen get this fixed as it presented as a bug in Drupal South 2022.

Discussed this with @lauriii. He pointed me towards https://github.com/jquery/jquery-ui/blob/main/ui/widgets/dialog.js#L460 - there's no way the dialog title can have html in it. Also we are attaching assets and then removing the html which all feels quite odd. I'm inclined to agree with @lauriii - I think we should only support passing in strings.

Perhaps what we can do is a helpful message when title is an array and tell people to render it first.

I realise that the majority on the issue feel that the solution here is useful but for me the fact that we're attaching assets and then stripping html makes the solution feel incorrect.

FWIW I agree with @alexpott in #97

Guys, is that project dead already ? ....will that patch #76 work also on D10 ?

Attaching draft patch for Drupal test system without attachments.

Trying to trigger test system.

Create a new patch for this issue. This patch issue fixed on my local.

I have tested for view content and node edit link found it is working fine.

Was previously tagged for issue summary update, which may be good but should be verified.

But as a bug it will need a test case showing the issue.

Thanks

Created a MR with rerolled against 11.x. Please someone address the comment #107.

Steps to reproduce
1. Install Drupal using standard profile
2. Edit the Content view (/admin/structure/views/view/content)
3. Add a "Custom text" field to thw view with content:

{{ attach_library('core/drupal.dialog.ajax') }}
<p><a class="use-ajax" data-dialog-type="modal" href="/admin/content">Content</a></p>

4. View the content view page and click the custom text link
This will cause the title of the modal to be 'Array' and will produce a warning log message:

Before applied MR, Warning in log message:
Warning: Array to string conversion in Drupal\Component\Render\PlainTextOutput::renderFromHtml() (line 22 of /app/core/lib/Drupal/Component/Render/PlainTextOutput.php)

Tested MR !6569, After applied MR Warning messages is no longer coming in logs.

Need to fix pipeline, so not updating status.

pipeline issue fixed moving NR.

Hiding patches for clarity.

Appears issue summary and screenshots were added in #93 so removing those tags

Ran test-only feature so removing that tag too

1) Drupal\Tests\views\FunctionalJavascript\Plugin\views\Handler\FieldTest::testModalDialogTitle
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Content'
+'Array'
/builds/issue/drupal-2663316/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-2663316/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FieldTest.php:112
/builds/issue/drupal-2663316/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Reviewing the code the check for array makes sense.

Looking at the remaining tasks seems there's still decisions to be made or follow ups created

It doesn't look like the MR is implementing the feedback from #97.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The solution looks, although I realized there isn't a perfect solution available to this because people may still continue using render arrays for #title. I think what's here is fine for now and we can revisit this later in case that we find a dialog solution that supports markup for the title.

Discussed with @lauriii we agreed that the common code in DialogRenderer, ModalRenderer and WideModalRenderer should be refactored into a method so that we don;t repeat ourselves. Also the renderer service is injected so there's no need to do \Drupal::service('renderer')

FWIW, the latest commit looks good 👍

Hiding all the patches.

The deprecation we're adding here is odd... because it is fixing some that is broken only to tell people we're going to break it again. I think that given things are broken if you pass this an array we should add a typehint and be done. Since this is a contructor it does not affect classes in contrib that extend from it.... unless they call parent with $title as an array - and that is broken so already.

I agree that the deprecation approach is a bit strange because it's fixing something to break it later. It tried to optimize to minimize the pain, and I don't think the cost for that was too high. That said, I don't have a strong preference to any particular direction so long as we get the bug fixed. 😇

Added a CR to inform people about the new helper - https://www.drupal.org/node/3426632

Committed and pushed 4add05d05f to 11.x and 4b835fad4e to 10.3.x. Thanks!

I'm not sure if this is the right place to put this (the issue is fixed and merged to the 10.3.x and 11.x) or I need to open another bug, but this issue needs more work.

The fix doesn't check type of the returning value for titleResolver

$title = $this->titleResolver->getTitle($request, $route_match->getRouteObject())->render();

Function getTitle() can return an array or a string

<?php
namespace Drupal\Core\Controller;

interface TitleResolverInterface {

  /**
   * Returns a static or dynamic title for the route.
   *
   * @return array|string|\Stringable|null
   *   The title for the route.
   */
  public function getTitle(Request $request, Route $route);

}

This can produce error if some module overwrite this function and return a string. For example, with Webform module I'm getting following error:
Error: Call to a member function render() on string in Drupal\Core\Render\MainContent\DialogRenderer->getTitleAsStringable() (line 145 of /var/www/html/web/core/lib/Drupal/Core/Render/MainContent/DialogRenderer.php).

So, we need to put additional check, for example:

  protected function getTitleAsStringable(array $main_content, Request $request, RouteMatchInterface $route_match): \Stringable|string|null {
    $title = NULL;
    if (array_key_exists('#title', $main_content)) {
      if (is_array($main_content['#title'])) {
        $title = $this->renderer->renderInIsolation($main_content['#title']);
      }
      else {
        $title = $main_content['#title'];
      }
    }
    elseif ($titleObj = $this->titleResolver->getTitle($request, $route_match->getRouteObject())) {
      if (is_string($titleObj) || $titleObj instanceof \Stringable) {
        $title = (string) $titleObj;
      }
      else {
        $title = $this->titleResolver->getTitle($request, $route_match->getRouteObject())->render();
      }
    }
    return $title;
  }

Here's the patch if somebody needed it until final fix.

Here's the patch for Drupal 10.2 (this version of Drupal use $this->renderer->renderPlain() instead of $this->renderer->renderInIsolation()

thank you, it works fine for Drupal 10.2.5

I'm seeing this also when rendering a view page in a dialog. Patch #137 worked for me as well. Thanks!

Seeing fatal errors caused by the problem mentioned in #135 on 10.3.

@sakiland: The approach to handle such an issue is either to revert the original issue, which would be too complicated at this point or create a follow-up, did that now: #3443959: DialogRenderer::getTitleAsStringable() does not support all return types of TitleResolverInterface::getTitle(). Working on a fix for that there.