Problem/Motivation

As Views does not defines a _title or a _title_callback key to its routes, TitleResolver is not able to resolve the title from the request.

Proposed resolution

Provide the static title of the view

Remaining tasks

Done

User interface changes

None.

API changes

None

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DuaelFr’s picture

Status: Active » Needs review
FileSize
4.8 KB

Sorry for that quick issue but I don't have the time to do more for now.

Status: Needs review » Needs work

The last submitted patch, 1: views_title_resolver-2516742-1.patch, failed testing.

DuaelFr’s picture

Sorry, old code base.
This patch might work but it should be optimized to avoid loading the same things twice.
I leave it there, without tests, as a work in progress and a base for discussion.

DuaelFr’s picture

Status: Needs work » Needs review
Issue tags: +VDC

Status: Needs review » Needs work

The last submitted patch, 3: views_title_resolver-2516742-3.patch, failed testing.

dawehner’s picture

Well, the general problem is that the title of a view is potentially really dynamic, so the only proper way to do that would be to even execute the view. This of course is too slow.
What we could do is to provide a title callback which tries to find at least some rough form of guess of the title.

DuaelFr’s picture

I suppose we should try to find the minimal way to ouput the view title from its route with or without arguments.

I see two different cases:

  1. we are looking for the title of the page we are already on (last part of a breadcrumb for example) - see https://www.drupal.org/node/2067859
  2. we are looking for the title of the page we are not on, based on the route name and eventually some predefined arguments

TitleResolver is currently used in 4 classes: HtmlRenderer, DialogRenderer, PathBasedBreadcrumbBuilder and ModulesListForm. I don't know the impact of Views titles not being resolvables for these classes.

@dawehner do you have a suggestion about how you'd like to see this solved or do you prefer to do it by yourself?

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
2.76 KB

Very small progress on my previous patch (temporary fix for an actual project)

Status: Needs review » Needs work

The last submitted patch, 8: views_title_resolver-2516742-8.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
758 bytes

Grrr... sorry I have to switch between D8 HEAD and beta12 for my current project so I did that last patch on t he wrong code base.
Here is the good one, based on #3 and including an interdiff.

This is still a work in progress waiting for better instructions.

Status: Needs review » Needs work

The last submitted patch, 10: interdiff.2516742.3.10.patch, failed testing.

DuaelFr’s picture

Reposting the patch in #10 without the interdiff to let the testbot quietly test it without a stupidly named interdiff.

That patch causes the modules list admin form to crash because of #2527546: ModulesListForm::buildRow() does not properly build the Request object for the TitleResolver

dawehner’s picture

I think loading the view object for itself is a no go, it is just too slow.
We should instead figure out on route compile time, whether the title is generic. How can we do that:

  • Are there arguments, is there a title area handler, no? Then its certainly static
DuaelFr’s picture

Status: Needs review » Needs work

Thank you for your feedback @dawehner.
Let me rephrase to see if I understood well.

In PathPluginBase::getRoute() rather than registering a _title_callback for all routes we should:
load the View with given view_id and display_id then

  • if the view has no argument and no title area handler put its title in a _title route param
  • else, ... keep using _title_callback ?
dawehner’s picture

In PathPluginBase::getRoute() rather than registering a _title_callback for all routes we should:
load the View with given view_id and display_id then

if the view has no argument and no title area handler put its title in a _title route param
else, ... keep using _title_callback ?

Yes its certainly in that direction. Yeah the title area handler could be something custom.
I would though simply not try to use _title_callback at all and just support static strings. Would that work for you?
_title_callback could lead to quite a performance regression if you accidentally use it, I think. Opinions?

DuaelFr’s picture

What would you think about only using _title in all cases?
Even if the View is highly dynamic, the TitleResolver is not likely to be used in a context where that View could receive all its arguments or the current request. It would be better than nothing to have the default views title.

dawehner’s picture

+1 for that idea.

DuaelFr’s picture

That's much much simpler.

\Drupal\views\EventSubscriber\RouteSubscriber::routes() retrieves the view and set its display before calling PathPluginBase::collectRoutes() that calls that patched getRoutes() method so we just have to call getTitle() from the ViewExecutable object.

DuaelFr’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: views_title_resolver-2516742-18.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review

That failure does not seems to come from this patch. Let's try again.

Status: Needs review » Needs work

The last submitted patch, 18: views_title_resolver-2516742-18.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review

I cannot reproduce that failure locally...

Status: Needs review » Needs work

The last submitted patch, 18: views_title_resolver-2516742-18.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review

Last try...

Status: Needs review » Needs work

The last submitted patch, 18: views_title_resolver-2516742-18.patch, failed testing.

dawehner’s picture

This is a unit test ... I think


PHP Fatal error:  Call to undefined function Drupal\views\drupal_set_message() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/src/DisplayPluginCollection.php on line 88
Drupal\Tests\rest\Unit\CollectRoutesTest                       0 passes   1 fails
Drupal\Tests\search\Unit\SearchPageRepositoryTest              8 passes

Just use phpunit -c core to find it

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
547 bytes

Wow... Views tests are full of surprises!

In \Drupal\views\Tests\Plugin\DisplayPageTest::testPageRouterItems a bunch of assertions are never called because in a condition that's never true. That assertions are not up to date so they would fail if they were called anyway.
See the Follow-up issue: #2575853: Fix \Drupal\views\Tests\Plugin\DisplayPageTest::testPageRouterItems so all assertions are called

But what make the patch fail is a debug() call in \Drupal\views\ViewExecutable::setDisplay().
This function is only used in SimpleTests and never in Unit Tests because the common.inc file is not loaded.

Status: Needs review » Needs work

The last submitted patch, 31: views_title_resolver-2516742-31.patch, failed testing.

The last submitted patch, 31: views_title_resolver-2516742-31.patch, failed testing.

RainbowArray’s picture

You might want to keep an eye on #2476947: Convert "title" page element into a block. Hopefully that will go in soon, and I am pretty sure there are some things with views and titles in there.

DuaelFr’s picture

I'm already actively following that issue because I think that it could make all the views related pages crash.
Thank you for the advice anyway.

DuaelFr’s picture

Ok so now the problem comes from a drupal_set_message() call.
I definitely need some help there, please.

DuaelFr’s picture

@dawehner after your deserved holidays (I hope you'll have some), could you have a look here, please?

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.06 KB
5.9 KB

Here is the bugfix, but we still need test coverage for the breadcrumb which is broken according to the issue summary.

agoradesign’s picture

The patch works for me. I needed it for use in breadcrumb.

Both things work:

  1. I display the active page title as last element in the breadcrumb. This works now with the patch.
  2. On news article detail pages, the breadcrumb item to the parent view is also displayed and linked correctly
benjy’s picture

The breadcrumb will break whenever any route doesn't provide a title because it's the path based breadcrumb, i'm not sure we should be testing that here. We have the unit tests for the views getTitle call, +1 for RTBC.

agoradesign’s picture

Thanks for the clarification - you're right. I don't think either that this should be tested by Views. It's up to the breadcrumb handling, how it deals with missing route titles.

+1 for RTBC

dawehner’s picture

Category: Feature request » Bug report
Issue summary: View changes
Issue tags: +rc target triage

For me the issue is done. On the other hand I disagree that this is a feature request, but rather it fixes bugs, as described in #2516742-40: Allow Views to be resolved by TitleResolver

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community

Everyone seems to be OK to say it's RTBC so let's move on :)
Thank you all for your contribution on this issue!

catch’s picture

Issue tags: -rc target triage +rc target

Discussed with alexpott and we both agree this is a straightforward fix we could/should do during RC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d467728 and pushed to 8.0.x. Thanks!

  • alexpott committed d467728 on 8.0.x
    Issue #2516742 by DuaelFr, dawehner: Allow Views to be resolved by...

Status: Fixed » Closed (fixed)

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