Contextual links support options but not use them to generate links

Good catch :)

Simple and straightforward, good find indeed. Committed.

Oops, looks like we missed the test coverage for this. I've reverted so we can add an explicit test. Thanks!

Working on test coverage.

Note this is a blocker of #2893978: Use newly supported 'options' in contextual links in Settings Tray contextual links.

The test coverage for this was forgotten in #2084463: Convert contextual links to a plugin system similar to local tasks/actions.

Here's the requested test coverage. The test-only patch is also the interdiff.

+++ b/core/tests/Drupal/Tests/Core/Menu/ContextualLinkManagerTest.php
@@ -278,20 +278,7 @@ public function testGetContextualLinksArrayByGroup() {
-      $plugin = $this->getMock('Drupal\Core\Menu\ContextualLinkInterface');
-      $plugin->expects($this->any())
-        ->method('getRouteName')
-        ->will($this->returnValue($definition['route_name']));
-      $plugin->expects($this->any())
-        ->method('getTitle')
-        ->will($this->returnValue($definition['title']));
-      $plugin->expects($this->any())
-        ->method('getWeight')
-        ->will($this->returnValue($definition['weight']));
-      $plugin->expects($this->any())
-        ->method('getOptions')
-        ->will($this->returnValue($definition['options']));
-      $map[] = [$plugin_id, [], $plugin];
+      $map[] = [$plugin_id, [], new ContextualLinkDefault([], $plugin_id, $definition)];

I don't really understand how this change relates to this test coverage.

You're right, that's pure clean-up. I made that change because I first thought there was a bug in that original code. After a bunch of debugging, it turned out to be correct. But this is just so much simpler that I figured I'd keep it.

I can remove it if you prefer.

@Wim Leers thanks for explanation. Leaving it seems fine

RTBC. The non-test code was already reviewed and committed once. And the test coverage looks good. Tests will kick back if I am wrong.

+++ b/core/modules/contextual/src/Element/ContextualLinks.php
@@ -78,7 +78,7 @@ public static function preRenderLinks(array $element) {
+        'url' => Url::fromRoute(isset($item['route_name']) ? $item['route_name'] : '', isset($item['route_parameters']) ? $item['route_parameters'] : [], $item['localized_options']),

I'm not sure, but should we add the new array item to the doc block since if someone sets that array item in the #contextual_links array, it overrides the plugin value.

@laurii I don't think we need a comment specify that you can not pass "options" or "localized_options" in $element['#contextual_links']

The doc block for \Drupal\contextual\Element\ContextualLinks::preRenderLinks for says exactly what keys can be passed in $element['#contextual_links']

*   all enabled modules). The value contains an associative array containing
   *   the following keys:
   *   - route_parameters: The route parameters passed to the url generator.
   *   - metadata: Any additional data needed in order to alter the link.

so I don't think

if someone sets that array item in the #contextual_links array, it overrides the plugin value

Nowhere does it specify that these will be used as parameters to Url::fromRoute(). So you can't say overriding a key in the array we just not passing it on. This would have been true before this patch. If you set $element['#contextual_links']['options'] or $element['#contextual_links']['localized_options'] these would not be used in making the renderable array representing contextual links that function returns.

"route_name" is in the same situation. If you passed $element['#contextual_links']['localized_options']["route_name]" it would not be used.

Only the 2 keys specifically in the doc block will be used in $element['#contextual_links'].

@tedbow thank you for explaining. My comment was incorrect and there's no reason to add any documentation about this doesn't make any API changes.

Committed 4c8c5e1 and pushed to 8.4.x. Thanks!

@lauriii thanks for the commit and review. Also good to raise the concern, better to mention something that turns out not be a problem than not to mention something that turns out to be problem 🏅😀

Yay!

And yep, this is merely making the API actually do/support what the API docs say!