Regression: routing / tabs / actions / contextual links lack way to attach replacement arguments to UI strings

Technically we can already do that if you override the class need for contextual links but yeah I agree that this should be part of the actual system, if D6/D7 supported it as well.

Well yeah we could scan the title for used "tokens" and than replace the values with the available request attributes.

It seems to be that something like this could already work.

13: title-2120235-13.patch queued for re-testing.

Oh right, good point.
There we go.

16: title-2120235-16.patch queued for re-testing.

ups.

Could you explain how one would go about using this?

Using the example of 'Translate @type' from the issue summary it is not immediately clear to me how I would make the 'type' that I want end up in the request variables.

Also it is not super problematic but a bit unfortunate that this approach seemingly does not support translating 'This is escaped: @title, this is not escaped !title' (i.e. multiple placeholders of different type with the same name). I don't know any use-case for that I just think it's unfortunate that t() actually supports that but this does not. Perhaps that should be documented somewhere?

Re 1: So you mean on /node/123 because the route has a route parameter, I can do 'Node id: !node' as title?! I don't quite get what

However, for dynamically defined routes, you can add the placeholder values which are replaced later on in title translation.

means in practice.

Re 2: I meant the use-case of:

t('@foo is different from !foo', array('@foo' => $foo_1, '!foo' => $foo_2));

where the different placeholder types correspond do different variables. Again, I realize this is very far fetched, but it's inconsistent with the general usage of t().

Maybe we should indeed also support direct placeholders like in D6/D7 where you could specify 'title arguments' in your hook_menu entry.

needs reroll due to #2053153: Allow contrib modules to provide plugins on behalf of optional modules

doing that now. patch soon.

pretty easy one. dont need to read these notes (unless it fails, or you are just curious)

conflict:
ContextualLinkManager.php

<<<<<<< HEAD
    $this->moduleHandler = $module_handler;
    $this->alterInfo('contextual_links_plugins');
=======
    $this->requestStack = $request_stack;
    $this->alterInfo($module_handler, 'contextual_links_plugins');
>>>>>>> 20

#2053153: Allow contrib modules to provide plugins on behalf of optional modules added the moduleHandler, so keeping that. it did

-    $this->alterInfo($module_handler, 'menu_local_actions');
+    $this->alterInfo('menu_local_actions');

also, so keeping that change.

And the patch here just was adding
+ $this->requestStack = $request_stack;

So that means I resolved the conflict like:

    $this->moduleHandler = $module_handler;
    $this->requestStack = $request_stack;
    $this->alterInfo('contextual_links_plugins');

pretty straightforward.

ContextualLinkManagerTest conflict was:

<<<<<<< HEAD
=======
    $request_stack = new RequestStack();
    $property = new \ReflectionProperty('Drupal\Core\Menu\ContextualLinkManager', 'requestStack');
    $property->setAccessible(TRUE);
    $property->setValue($this->contextualLinkManager, $request_stack);

    $this->moduleHandler = $this->getMock('\Drupal\Core\Extension\ModuleHandlerInterface');

>>>>>>> 20

I dont see any context lines changes... and the patch is just inserting lines. so I kept those inserted lines from the patch.

The testbot errors looked like they might be a common error that some people had seen,
in irc
@larowlan helped me sort through this phpunit error.

tldr;

+++ b/core/tests/Drupal/Tests/Core/Menu/ContextualLinkManagerTest.php
@@ -123,6 +124,13 @@ protected function setUp() {
+    $property->setValue($this->contextualLinkManager, $request_stack);
+
+    $this->moduleHandler = $this->getMock('\Drupal\Core\Extension\ModuleHandlerInterface');
+

$this->moduleHandler =
doesn't need to be added here, as it is already set earlier.

=======

$ ./vendor/bin/phpunit --group Menu --strict
PHPUnit 3.7.21 by Sebastian Bergmann.

Configuration read from /Users/ctheys/foo/d82-project/d82/core/phpunit.xml.dist

...........F.F.....

Time: 2 seconds, Memory: 48.25Mb

There were 2 failures:

1) Drupal\Tests\Core\Menu\ContextualLinkManagerTest::testGetContextualLinksArrayByGroup
Expectation failed for method name is equal to <string:alter> when invoked at sequence index 1.
The expected invocation at index 1 was never reached.


2) Drupal\Tests\Core\Menu\ContextualLinkManagerTest::testPluginDefinitionAlter
Expectation failed for method name is equal to <string:alter> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.


FAILURES!
Tests: 19, Assertions: 76, Failures: 2.

with the patch applied, http://privatepaste.com/7a44e42688#
looking for expects dealing with alter, found two moduleHandler expects:
$this->moduleHandler->expects($this->at(1)) ->method('alter') ->with($this->equalTo('contextual_links'), new \PHPUnit_Framework_Constraint_Count(2), $this->equalTo('group1'), $this->equalTo(array('key' => 'value')));
$this->moduleHandler->expects($this->once()) ->method('alter') ->with('contextual_links_plugins', $definitions);
so, were they trying to give the expected value (or something) for the first $this->moduleHandler =
and then the at(1) was what was expected for the second call?
or was it a mistake to do $this->moduleHandler = ... twice?

taking out the $this->moduleHandler =
that this patch added gave:
OK (19 tests, 79 assertions)

better.

patch attached.

@dawehner: re#26 it is not entirely clear to me what is included with raw variables. Is it possible to inject anything in there with the routing structure or would we need specific support for that? Again, this issue is to avoid custom subclassing of subtabs and contextual links and have translation support built-in for placeholders to get back what we had in D7 without each module needing to subclass it for their own. So would this in itself be able to get rid of the subclassing in config translation?

So we talk about the following piece of code:

    $type_name = Unicode::strtolower($this->mapperManager()->createInstance($this->pluginDefinition['config_translation_plugin_id'])->getTypeLabel());
    return $this->t($this->pluginDefinition['title'], array('@type_name' => $type_name), $options);

So we could have something like the following in the yml files / the derivative definition:

  _title: Translate @type_name
  _title_arguments:
    type_name: Example

Tryed(trait) to move the common code into a trait but it somehow did not worked. If someone wants to try it, feel free to do it!

If it's not straighforward to add a trait, is there somewhere we can put a static method with a @todo for the trait?

Sure.

That adds a @todo to one case of the code, but I was asking about a static method somewhere with a @todo - to avoid the copy/paste.

Realized that we don't need dynamic parameters as this is already covered by using a custom getTitle method on your plugin.

Let's just support static title arguments, like we did in D7. This at least solves the regression and covers most of the usecases.

Committed/pushed to 8.x, thanks!

I missed this when it went in, but the idea of putting static arguments in the YAML perplexes me. I don't understand the actual use case.

@Gabor: So both of those are using title_context, which seems more reasonable since that would be a static string. I'm much less sure it makes sense to ever define title arguments in the plugin definition.

Ok, I see: thee use case for this is really when creating derivatives, where each derivative would have a different static title argument.

I don't love it as a solution, but at least I see the use case now.

+++ b/core/lib/Drupal/Core/Menu/ContextualLinkDefault.php
@@ -16,13 +17,21 @@ class ContextualLinkDefault extends PluginBase implements ContextualLinkInterfac
-  public function getTitle() {
+  public function getTitle(Request $request = NULL) {

Why was this changed?

#2783375: ContextualLinkManager calls getTitle() on ContextualLinkInterface plugins without using the controller resolver is related