Broken title in modal dialog when title is a render array

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

Yesss, Drupal is dead....

Attaching draft patch for Drupal test system without attachments.

Trying to trigger test system.

The last submitted patch, 101: 2663316-100.drupal.Broken-title-in-modal-dialog-when-title-is-a-render-array.patch, failed testing. View results →

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

omkar.podey → made their first commit to this issue’s fork.

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, for the Warning in logs:
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

omkar.podey → changed the visibility of the branch 2663316-broken-title-in to hidden.

omkar.podey → changed the visibility of the branch 2663316-broken-title-in to active.

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!

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

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.

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: 🐛 DialogRenderer::getTitleAsStringable() does not support all return types of TitleResolverInterface::getTitle() Fixed . Working on a fix for that there.

@sakiland - thank you for your patch, it looks like it works fine for Drupal 10.2x however only for nodes. When you try patch #137 to render in modal/dialog/off canvas USER, there is an error:

Error: Call to a member function render() on array in Drupal\Core\Render\MainContent\DialogRenderer->getTitleAsStringable() (line 149 of /core/lib/Drupal/Core/Render/MainContent/DialogRenderer.php).

#0 /core/lib/Drupal/Core/Render/MainContent/OffCanvasRenderer.php(63): Drupal\Core\Render\MainContent\DialogRenderer->getTitleAsStringable()
#1 /core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php(90): Drupal\Core\Render\MainContent\OffCanvasRenderer->renderResponse()
#2 [internal function]: Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray()
#3 /core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func()
#4 /vendor/symfony/http-kernel/HttpKernel.php(186): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch()
#5 /vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw()
#6 /core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle()
#7 /core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle()
#8 /core/lib/Drupal/Core/StackMiddleware/ContentLength.php(28): Drupal\Core\StackMiddleware\KernelPreHandle->handle()
#9 /core/modules/big_pipe/src/StackMiddleware/ContentLength.php(32): Drupal\Core\StackMiddleware\ContentLength->handle()
#10 /core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\big_pipe\StackMiddleware\ContentLength->handle()
#11 /core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass()
#12 /core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\page_cache\StackMiddleware\PageCache->handle()
#13 /core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle()
#14 /core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(36): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle()
#15 /core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\Core\StackMiddleware\AjaxPageState->handle()
#16 /core/lib/Drupal/Core/DrupalKernel.php(704): Drupal\Core\StackMiddleware\StackedHttpKernel->handle()
#17 /index.php(19): Drupal\Core\DrupalKernel->handle()
#18 {main}

why this ticket is fixed. I just try with use-ajax with a simple view, it always shows the table instead of the view title

I try debug it show $main_content['#title'] have #markup

Assuming this is fixed in 10.3? The original commit in #132 plus the related issue in #135 was committed as well. Good to go?