Files: 
CommentFileSizeAuthor
#52 menu-plugins-1981644-52.patch18.36 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 56,347 pass(es).
[ View ]
#52 1981644-incremental-41-52.patch.txt2.92 KBpwolanin
#41 menu-plugins-1981644-41.patch17.88 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 56,427 pass(es), 49 fail(s), and 3 exception(s).
[ View ]
#41 1981644-incremental-39-41.patch.txt9.54 KBpwolanin
#39 drupal-1981644-39.patch14.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,702 pass(es), 18 fail(s), and 1 exception(s).
[ View ]
#37 menu-plugins-1981644-37.patch14.3 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 56,435 pass(es), 18 fail(s), and 1 exception(s).
[ View ]
#36 menu-plugins-1981644-36.patch14.55 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 56,172 pass(es), 82 fail(s), and 171 exception(s).
[ View ]
#34 menu-plugins-1981644-34.patch9.43 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 56,432 pass(es), 18 fail(s), and 1 exception(s).
[ View ]
#33 menu-plugins-1981644-33.patch7.76 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 56,360 pass(es), 24 fail(s), and 4 exception(s).
[ View ]
#30 1981644-30.patch4.5 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 57,991 pass(es).
[ View ]
#26 drupal-1981644-26.patch2.31 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,435 pass(es), 26 fail(s), and 19 exception(s).
[ View ]

Comments

donquixote’s picture

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.

dawehner’s picture

Yeah I though of a title() method which get's the parameter injected like the central routing method does. This would allow you to get the arguments from the url.

donquixote’s picture

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)

dawehner’s picture

Well, so you propose a separate title controller? In general having proper injected controllers, doesn't seem to much of an overhead, even you just use it for the title.

donquixote’s picture

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..

alexpott’s picture

Issue tags:+Stalking Crell

Tagging :)

alexpott’s picture

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 :(

Crell’s picture

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. :-(

dawehner’s picture

Issue tags:-Stalking Crell

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.

dawehner’s picture

Issue tags:+Stalking Crell

Add back the tag.

catch’s picture

donquixote’s picture

@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.

donquixote’s picture

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.

vijaycs85’s picture

Priority:Normal» Major

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?

donquixote’s picture

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.)

catch’s picture

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.

dawehner’s picture

I'm wondering what have been discussed about that on the meeting on saturday. Would be great to see some logs about that.

tim.plunkett’s picture

Assigned:Unassigned» xjm

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

andypost’s picture

tim.plunkett’s picture

Assigned:xjm» Unassigned
donquixote’s picture

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.

pwolanin’s picture

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’s picture

I think pages and all kind of links are kind of different so I would suggest to keep them separate at least for now.

For local actions/local tasks and maybe even menu links in general I think we want to have an additional parameter on the route definition
which allows you to specify how to get the title.

route_example:
  defaults:
    _content: 'Drupal\example\Controller\Bar::content'
    _title: 'Drupal\example\Controller\Bar::title'

As all new systems (like local actions/tasks) have the route name available we can also easily instanciate the route controllers,
once we are rendering the titles. We could enhance that by allowing simple strings on here like 'Custom title' (without the colons),
which then would just be put into t() later.

pwolanin’s picture

@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.

tim.plunkett’s picture

@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()

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new2.31 KB
FAILED: [[SimpleTest]]: [MySQL] 58,435 pass(es), 26 fail(s), and 19 exception(s).
[ View ]

Here is a basic patch which adds menu title generation to local actions.
To be honest this is maybe not the proper place for the patch.

tim.plunkett’s picture

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.

tim.plunkett’s picture

Issue summary:View changes

related

Status:Needs review» Needs work

The last submitted patch, drupal-1981644-26.patch, failed testing.

pwolanin’s picture

@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?

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new4.5 KB
PASSED: [[SimpleTest]]: [MySQL] 57,991 pass(es).
[ View ]

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

pwolanin’s picture

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.

pwolanin’s picture

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.

pwolanin’s picture

Status:Needs review» Needs work
StatusFileSize
new7.76 KB
FAILED: [[SimpleTest]]: [MySQL] 56,360 pass(es), 24 fail(s), and 4 exception(s).
[ View ]

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

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new9.43 KB
FAILED: [[SimpleTest]]: [MySQL] 56,432 pass(es), 18 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, menu-plugins-1981644-34.patch, failed testing.

pwolanin’s picture

StatusFileSize
new14.55 KB
FAILED: [[SimpleTest]]: [MySQL] 56,172 pass(es), 82 fail(s), and 171 exception(s).
[ View ]

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

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new14.3 KB
FAILED: [[SimpleTest]]: [MySQL] 56,435 pass(es), 18 fail(s), and 1 exception(s).
[ View ]

ok, fixed the factory and some other bugs.

dawehner’s picture

Here is some feedback.

  • Let's go with @Translation in the annotation as l10n really profits from it.
+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,87 @@
+abstract class MenuLocalActionBase extends ContainerFactoryPluginBase implements MenuLocalActionInterface {

it would be also possible to just use "ContainerFactoryPluginInterface" instead of the plugin base class, but yeah it is not important at all.

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,87 @@
+      $container->get('router.route_provider')->getRouteByName($plugin_definition['route_name']),

It seems to make sense to do this in logic in the constructor, as is more then just getting some services.

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,87 @@
+  public function getTitle() {
...
+  public function getPath() {

In order to get arbitrary injection of the request parameters getTitle/getPath should not be called directly but going through the Controller Resolver.

dawehner’s picture

StatusFileSize
new14.59 KB
FAILED: [[SimpleTest]]: [MySQL] 56,702 pass(es), 18 fail(s), and 1 exception(s).
[ View ]

This should allow to use the controller resolver.

Status:Needs review» Needs work

The last submitted patch, drupal-1981644-39.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new9.54 KB
new17.88 KB
FAILED: [[SimpleTest]]: [MySQL] 56,427 pass(es), 49 fail(s), and 3 exception(s).
[ View ]

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.

tim.plunkett’s picture

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.

pwolanin’s picture

@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).

Status:Needs review» Needs work

The last submitted patch, menu-plugins-1981644-41.patch, failed testing.

tim.plunkett’s picture

+++ 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’s picture

One problem which exists at the moment is the following: As the parameter converters converts stuff, the plain values, like the UID
does not exist anymore on the request object. fubhy is working on changing the way how the converter works and he has a plan to adress that, so the upcasted values are stored different.

On the meantime we could basically say that we don't support attributes in order to move forward. Once the parameter converter patch got it, this will automatically work fine.

pwolanin’s picture

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

pwolanin’s picture

Status:Needs work» Needs review

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

pwolanin’s picture

I cannot reproduce the failures locally.

Status:Needs review» Needs work

The last submitted patch, menu-plugins-1981644-41.patch, failed testing.

pwolanin’s picture

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

pwolanin’s picture

Status:Needs work» Needs review
Issue tags:+MenuSystemRevamp, +WSCCI
StatusFileSize
new2.92 KB
new18.36 KB
PASSED: [[SimpleTest]]: [MySQL] 56,347 pass(es).
[ View ]
dawehner’s picture

+++ b/core/includes/menu.incundefined
@@ -2274,19 +2274,19 @@ function menu_get_local_actions() {
+    $route_path = $manager->getPath($plugin);

On the longrun you want to pass in the request object into getPath().

+++ b/core/modules/system/lib/Drupal/system/Annotation/MenuLocalActionPlugin.phpundefined
@@ -0,0 +1,46 @@
+  /**
+   * The static title for the local action.
+   *
+   * @var string
+   */
+  public $title;

Let's go with

   * The static title for the local action.
   *
   * @ingroup plugin_translatable
   *
   * @var \Drupal\Core\Annotation\Translation

to allow people to understand it.

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,90 @@
+   * Retun the path corresponding to the route.

Should be "Return" instead of Retun

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,90 @@
+  public function getPath() {

Then we get the request we passed in and use $request->attributes->all() into the url generate method.

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionInterface.phpundefined
@@ -0,0 +1,22 @@
+interface MenuLocalActionInterface {

I am wondering why getPath is not part of the interface.

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

To be honest, I think we should throw an exception, as there is clearly something going wrong.

pwolanin’s picture

pwolanin’s picture

Status:Needs review» Active
pwolanin’s picture

Issue summary:View changes

adding more issues

Crell’s picture

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. :-)

tim.plunkett’s picture

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())

dawehner’s picture

Added a new issue as this one is totally full of unrelated patches :) #2032535: Resolve 'title' using the route and render array

amateescu’s picture

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

xjm’s picture

Status:Active» Closed (duplicate)

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

xjm’s picture

Issue summary:View changes

add node/2031473

dawehner’s picture

Issue summary:View changes

.