Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#38 | interdiff.txt | 5.9 KB | dawehner |
#38 | 2516742-38.patch | 6.06 KB | dawehner |
#31 | interdiff.2516742.18.31.txt | 547 bytes | DuaelFr |
#31 | views_title_resolver-2516742-31.patch | 1.23 KB | DuaelFr |
#18 | interdiff.2516742.10.18.txt | 2.77 KB | DuaelFr |
Comments
Comment #1
DuaelFrSorry for that quick issue but I don't have the time to do more for now.
Comment #3
DuaelFrSorry, 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.
Comment #4
DuaelFrComment #6
dawehnerWell, 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.
Comment #7
DuaelFrI 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:
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?
Comment #8
DuaelFrVery small progress on my previous patch (temporary fix for an actual project)
Comment #10
DuaelFrGrrr... 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.
Comment #12
DuaelFrReposting 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
Comment #13
dawehnerI 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:
Comment #14
DuaelFrThank 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
Comment #15
dawehnerYes 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?
Comment #16
DuaelFrWhat 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.
Comment #17
dawehner+1 for that idea.
Comment #18
DuaelFrThat'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.
Comment #19
DuaelFrComment #22
DuaelFrThat failure does not seems to come from this patch. Let's try again.
Comment #25
DuaelFrI cannot reproduce that failure locally...
Comment #28
DuaelFrLast try...
Comment #30
dawehnerThis is a unit test ... I think
Just use phpunit -c core to find it
Comment #31
DuaelFrWow... 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.
Comment #34
RainbowArrayYou 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.
Comment #35
DuaelFrI'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.
Comment #36
DuaelFrOk so now the problem comes from a
drupal_set_message()
call.I definitely need some help there, please.
Comment #37
DuaelFr@dawehner after your deserved holidays (I hope you'll have some), could you have a look here, please?
Comment #38
dawehnerHere is the bugfix, but we still need test coverage for the breadcrumb which is broken according to the issue summary.
Comment #39
agoradesign CreditAttribution: agoradesign commentedThe patch works for me. I needed it for use in breadcrumb.
Both things work:
Comment #40
benjy CreditAttribution: benjy at PreviousNext commentedThe 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.
Comment #41
agoradesign CreditAttribution: agoradesign commentedThanks 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
Comment #42
dawehnerFor 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
Comment #43
DuaelFrEveryone seems to be OK to say it's RTBC so let's move on :)
Thank you all for your contribution on this issue!
Comment #44
catchDiscussed with alexpott and we both agree this is a straightforward fix we could/should do during RC.
Comment #45
alexpottCommitted d467728 and pushed to 8.0.x. Thanks!