Figure out how to deal with 'title/title callback'

Titles (and title callback) were also very useful for breadcrumbs and tabs in D7.
Esp, for breadcrumb items that are not in any menu.

Yes, they were also useful for the actual page title, but this could be solved with drupal_set_title() instead. And in D8 it might be possible to build it into the page controller, instead of the menu / routing system.

A method on which object?
Ideally I want this independent of the current page, so you can evaluate the method for arbitrary paths.
(typically this would be for tabs or breadcrumb items)

It could trigger a bunch of dependency loading?

Also in D7 it used to be possible to swap out a page callback independent of the title and access callback..

Tagging :)

I tried doing the following so that the path is not changed from aggregator/sources/%aggregator_feed to aggregator/sources/%

--- a/core/includes/menu.inc
+++ b/core/includes/menu.inc
@@ -2701,8 +2701,8 @@ function _menu_router_merge_route(array $router_item, $path) {
   $router_item['page callback'] = 'USES_ROUTE';
   $router_item['access callback'] = TRUE;

-  // Translate placeholders, e.g. {foo} -> %.
-  $router_item['path'] = preg_replace('/{' . DRUPAL_PHP_FUNCTION_PATTERN . '}/', '%', $router_item['path']);
+  // Translate placeholders, e.g. {foo} -> %foo.
+  $router_item['path'] = preg_replace('/{(' . DRUPAL_PHP_FUNCTION_PATTERN . ')}/', '%${1}', $router_item['path']);

   return $router_item;
 }

And whilst this worked to make the upcasting occur it broke other things :(

We discussed this on a recent WSCCI meeting Hangout. I believe what we ended up with as a recommendation was:

1) Controllers SHOULD return not a string or a render array but a SCOTCH HtmlFragment (or whatever it's called). That has a body and title and needed CSS/JS and link elements and whatever else. Title gets pulled from there for display.
2) IF the HtmlFragment has no title (i.e., $fragment->title() returns empty string), then fall back to the title for the current menu link (defined, um, somehow).
3) IF there is no current menu link, then there is no title. Well you should have returned one from your content!
4) Menu items continue to get their title info from hook_menu() as now, mostly because we don't have time/energy/ideas for doing anything else at the moment.

Since really, the content title and the menu title are NOT the same thing; them being the same thing is, nominally, an edge case. That separates that concept with fallback logic.

The main downside of this approach is that it is SCOTCH-dependent, and HtmlFragments are not yet in core. :-(

This looks like a near-duplicate of #1830588: [META] remove drupal_set_title() and drupal_get_title().

@Crell (#8):
As mentioned in #1, I see the main purpose of title / title callback not for the page title, but to easily determine a default title for a link that is not the current page.

Breadcrumbs, tabs, menu links, contextual links, etc: These are (or should be) independent subsystems, but it does make a lot of sense to let them reuse the title information defined in one place.

So, while I am perfectly happy with SCOTCH for the page title, but this leaves the question of page-independent link titles.

Menu items continue to get their title info from hook_menu() as now, mostly because we don't have time/energy/ideas for doing anything else at the moment.

Ok, if hook_menu() stays in the game, then the other points become obsolete. We can continue to use menu_get_item() to determine a title for any link on the planet.
The only thing is that originally everybody wanted to kill hook_menu(), and at least some links have already moved away from it.

It seems to be a real overhead to require a second object for most cases. This object might also need dependency injection etc. as you don't know what a title needs.

That's the new equivalent of a "callback": A class with one public method to implement the callback, plus DIC.
Although it would be imaginable to use one object (service) for multiple title callbacks.

Can we say it is major as it is blocking at least 41 items (which have 'title callback') and almost all items that have title in menu item?

Menu items continue to get their title info from hook_menu() as now, mostly because we don't have time/energy/ideas for doing anything else at the moment.

Do I understand correctly that anything that uses the new Symfony routing system sf-based route declaration can not at the same time use hook_menu() ? And thus, won't have a title or title callback?
This does not sound like an acceptable solution.

I can see these options (not mutually exclusive):

1. Completely go back to hook_menu(), or let hook_menu() be the preferred way of declaring routes, leaving Sf-based route declaration as an exotic side phenomenon.
   I don't think anyone wants this.
2. Having a separate hook / yml / event subscribers / etc to declare titles and callbacks (*) for routes.
   I had the impression from the other thread that this would be the most preferred direction to take.
   Personally I don't really like it, because it is too easy in contrib development to forget the title, or that routes and titles get out of sync. And DX suffers because you need to maintain two places instead of one.
3. Somehow add the title / title callback (*) to routes.yml.
4. Introduce a new hook / yml / event subscriber / etc to declare routes along with the title.

(*) TItle callback could be a method on the page object, or on a separate service, or a callback (function), either of these can be allowed. (see #4 and #13)

(We could go beyond this and allow more than one (conditional) title callback per route, or allow the title callback to be altered after the route is loaded (so different entity bundles can have different title callback). But the basic questions and options remain the same.)

For the actual page title #1830588: [META] remove drupal_set_title() and drupal_get_title() might be enough - just don't use the routing system at all and have the controller return it.

There was a google doc with awesome notes that I can't find. xjm you're our only hope!

Probably we have meta for that #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff
PS: updated subbary with related links

https://docs.google.com/document/d/1sgxD0l2OM91EqTekXryhdaXQn_n4zPUnLgq_... is the doc in question it could use a summary.

Had a look at the docs.
And again we are only talking about page title, thus ignoring the essential half of the problem :(
drupal_set_title() needs to go away, and be replaced by something returned by the page controller. I am all for that.
But this should be discussed in its own issue.

This issue *should* be about the "multiple-purpose navigation link title" which can be used in breadcrumbs, menus, tabs, any other navigational elements that might be added in contrib, for paths other than the current page.

Breadcrumbs in D8 core:
In #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service (breadcrumbs in D8 core) we still follow the quite boring D7 approach: One component (usually one that represents the current request, or some aspect of it) builds the entire breadcrumb.
The problem: The active breadcrumb builder needs to know not only the nature of the current request, but also of all parents it wants to have in the breadcrumb.
I do support this, because we lack the time to do something else *in core*, but I also believe that D8 contrib can do much better.

Breadcrumbs in D8 contrib:
Here is how it should work:
- For every path/route/etc (*) you can determine a parent path/route. Starting from the current page, you find the parent, the parent of the parent, etc, until you end at the front page. This gives you a reverse trail of breadcrumb paths.
- Different plugins (with weight) can negotiate what is the parent for a given path.
- Fallback for parent-finding: Chop off the last fragment of the path, consider the result as the parent path.
- For every path, you can find a (navigational) title.
- Different plugins can negotiate on the title.
- Fallback for the title is the one produced by title/title_callback, or whatever this becomes in the future. (**)
- Note: Navigational title typically wants to be shorter than the page title.
- paths + titles make up the breadcrumb.

(*) In D7 it was easy to just use a path (string) for this. Maybe in D8 we need to switch to an array or route + parameters.

(**) I hope this step explains why I desperately want to keep the title / title_callback, or have it replaced by something equivalent.
It also shows how a generic title / title_callback can be used by a completely new system in contrib.

discussing this broadly with tim.plunkett in terms of #2023795: REGRESSION: hook_local_actions doesn't use title callback.

We shoudl have a consistent way to handle this for pages, local actions, and local tasks (tabs)

@dawehner - I think that's good for pages, but indeed - we still need a framework for handling titles for local actions and local tasks (tabs) within the new routing system. the current D8 decouples them from the page title, which I think is useful, but makes this a bit harder.

@pwolanin what @dawehner is suggesting is only for local actions/tasks, the page title is being handled in #1830588: [META] remove drupal_set_title() and drupal_get_title()

I think that belongs in #2023795: REGRESSION: hook_local_actions doesn't use title callback, but all of these issues are tied together. Adding to the issue summary.

related

@dawehner - I think the title for local actions, etc should be decoupled from the route title? Let's just use the route title for the page itself?

Per discussion with dawehner, this patch allows the route to define a default title, and allows the local action to override it.

New plan with dawehner and fubhy - we'll define a plugin type each for local actions and local tasks (tabs). Annotation based discover. The methods on these plugins will handle the generation of the path and title.

Note, the initial implementation of this may define the api (plugin type) but not be optimized, while we can later compile the result to make it faster.

ok, here's a start at a plugin patch for feedback on direction.

Works locally for the views UI action. Need to convert the test

this fixes up the test code too.

Note so far the patch is just switching from hook to plugin discovery.

The more interesting pieces will come as we create an instance and use it for e.g. generating the path and title.

The plugins aren't being constructed right so this patch is broken - posting for dawehner to see the direction

ok, fixed the factory and some other bugs.

Here's a patch that uses the url generator. Had to apply a trim or the leading '/' causes two action links.

Also fixes up other hook implementations that were not included above.

What's the reasoning for a plugin that will only ever be an annotation?
If we're just using it for metadata and never instantiate it, it should be YAML.
I'm guessing I'm missing the justification for a full plugin.

@tim.plunkett - so the base implementation is not just annotation. Methods on the class are invoked by the manager. The interesting use cases come in overriding the base plugin implementation to e.g. provide a dynamic title or path.

So, while most of these plugins will look like just annotation, this is restoring the dynamic title & path features where needed. Also, we can next copy this same kind of implementation for local tasks (tabs).

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,83 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getRouteName() {
+    return $this->pluginDefinition['route_name'];
+  }
+
+  /**
+   * Return the title to be shown for this action.
+   */
+  public function getTitle() {
+    // Subclasses may pull in the request or specific attributes as parameters.
+    return $this->pluginDefinition['title'];
+  }
+
+  /**
+   * Retun the path corresponding to the route.
+   */
+  public function getPath() {
+    // Subclasses may set a request into the generator or
+    // use any desired method to generate the path.
+    return $this->generator->generate($this->getRouteName());

Ah, I missed this part... Hmm.

@dawehner - yes, I think we should see this as laying the groundwork for dynamic titles/paths working once that other patch is in.

#41: menu-plugins-1981644-41.patch queued for re-testing.

I cannot reproduce the failures locally.

These failures are being caused by a core bug with the URLgenerator when Drupal is installed in a subdir

new patch includes a work-around for bug noted at #2031353: URLgenerator broken for Drupal installed in a subdirectory - doesn't have a way to get a Drupal path

moving the local actions patch to #2031473: Convert menu local actions to plugins so that we can generate dynamic titles and paths

moved patch for local actions to #2031473: Convert menu local actions to plugins so that we can generate dynamic titles and paths

adding more issues

I only sort of understand what's going on here, but...

+++ b/core/modules/system/lib/Drupal/system/Annotation/MenuLocalActionPlugin.php
@@ -0,0 +1,46 @@
+  public $appears_on;

appearsOn

And all of these should be protected. Why do we have public properties?

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.php
@@ -0,0 +1,111 @@
+    // @todo What to do if it is not callable.
+    return '';

It sounds like that would only happen if someone screwed up their $local_action object. That's an exceptional case, so throw an exception or fatal.

Actually, is getPath() part of the MenuLocalActionInterface definition? If so, then it should never not be callable as we'd get a fatal just calling this method. If not... it should be for us to use it here. :-)

Those are the annotation properties.

1. They must be public
2. They must match the case of our array keys cases, which is foo_bar

This class is never used directly by developers, but rather turned into an array by plugin discovery ($manager->getDefinition())

I think another issue with the exact same title and summary is a little confusing..

I think the idea was to close this as a dupe in favor of the un-confusing issue?

add node/2031473