Updated: Comment #55

Problem/Motivation

hook_menu() is still responsible for contextual links, so we can't remove the old menu router.

In addition the system for finding contextual links is based on a confusing combination of path hierarchy and hook_menu() type constants.

Proposed resolution

A totally new mechanism for finding contextual links is added using plugins that are very similar to those used for local tasks.

The plugin type which is discovered using YAML files and use routes to render the contextual links.

AJAX is still used to fetch all the rendered contextual links for a page. A minor change is made to the format of the contextual links ID that's rendered in the HTML so that we have named parameters compatible with route variables.

Remaining tasks

Follow-ups to convert remaining contextual links and update Views to use the new system (possible via plugin derivatives).

User interface changes

none

API changes

Define contextual links via grouped plugins (discovered via YAML) instead of via hook_menu.

see change notice: https://drupal.org/node/2165243

CommentFileSizeAuthor
#138 interdiff.txt1.77 KBdawehner
#138 contextual_links-2084463-138.patch85.49 KBdawehner
#136 interdiff.txt3.27 KBtim.plunkett
#136 contextual-2084463-136.patch85.19 KBtim.plunkett
#134 contextual_links-2084463-134.patch84.86 KBtim.plunkett
#131 contextual_links-2084463-131.patch84.85 KBdawehner
#131 interdiff.txt2.61 KBdawehner
#129 contextual_links-2084463-129.patch85.26 KBdawehner
#129 interdiff.txt6.06 KBdawehner
#123 contextual_links-2084463-123.patch79.92 KBdawehner
#121 contextual_links-2084463-121.patch79.5 KBdawehner
#121 interdiff.txt2.33 KBdawehner
#117 contextual_links-2084463-117.patch78.9 KBdawehner
#117 interdiff.txt520 bytesdawehner
#114 contextual_links-2084463-114.patch78.81 KBDavid_Rothstein
#114 interdiff-113-114.txt11.76 KBDavid_Rothstein
#113 contextual_links-2084463-113.patch70.98 KBDavid_Rothstein
#113 interdiff-102-113.txt21.5 KBDavid_Rothstein
#113 interdiff-111-113.txt1.92 KBDavid_Rothstein
#111 contextual_links-2084463-111.patch71.38 KBDavid_Rothstein
#111 interdiff-102-111.txt20.94 KBDavid_Rothstein
#102 interdiff.txt671 bytesdawehner
#102 contextual_links-2084463-102.patch71.8 KBdawehner
#99 contextual_links-2084463-99.patch71.81 KBdawehner
#99 interdiff.txt544 bytesdawehner
#97 interdiff.txt9.79 KBdawehner
#97 contextual_links-2084463-97.patch71.81 KBdawehner
#95 contextual_links-2084463-95.patch70.62 KBdawehner
#95 interdiff.txt3.88 KBdawehner
#93 interdiff.txt9.99 KBdawehner
#93 contextual_links-2084463-93.patch72.12 KBdawehner
#91 contextual_links-2084463-91.patch68.54 KBdawehner
#88 interdiff.txt16.07 KBdawehner
#88 contextual_links-2084463-88.patch64.16 KBdawehner
#84 contextual_links-2084463-84.patch56.46 KBdawehner
#84 interdiff.txt2.86 KBdawehner
#82 contextual_links-2084463-82.patch53.98 KBdawehner
#82 interdiff.txt13.3 KBdawehner
#80 contextual_links-2084463-80.patch47.33 KBdawehner
#80 interdiff.txt8.83 KBdawehner
#78 contextual_links-2084463-78.patch47.47 KBdawehner
#74 interdiff.txt1.38 KBdawehner
#74 contextual_links-2084463-74.patch47.89 KBdawehner
#70 contextual_links-2084463-70.patch47.96 KBdawehner
#70 interdiff.txt14.08 KBdawehner
#64 2084463-62-63.incrememnt.txt677 bytespwolanin
#63 contextual_links-2084463-63.patch39.17 KBpwolanin
#62 contextual_links-2084463-62.patch39.17 KBdawehner
#62 interdiff.txt6.04 KBdawehner
#58 contextual_links-2084463-58.patch38.58 KBdawehner
#58 interdiff.txt2.47 KBdawehner
#52 contextual_links-2084463-52.patch38.5 KBpwolanin
#52 2084463-48-52.increment.txt462 bytespwolanin
#48 contextual_links-2084463-48.patch38.51 KBpwolanin
#48 2084463-47-48.increment.txt615 bytespwolanin
#47 contextual_links-2084463-47.patch38.57 KBpwolanin
#46 contextual_links-2084463-46.patch38.5 KBdawehner
#44 contextual_links-2084463-49.patch38.46 KBdawehner
#40 contextual_links-2084463-49.patch38.46 KBdawehner
#38 contextual_links-2084463-38.patch38.48 KBdawehner
#38 interdiff.txt5.15 KBdawehner
#33 contextual_links-2084463-33.patch38.57 KBdawehner
#33 interdiff.txt4.65 KBdawehner
#31 contextual_links-2084463-31.patch35.82 KBdawehner
#31 interdiff.txt10.33 KBdawehner
#30 contextual_links-2084463-30.patch26.18 KBpwolanin
#30 2084463-26-30.increment.txt991 bytespwolanin
#26 contextual_links-2084463-26.patch26.18 KBpwolanin
#26 2084463-25-26.increment.txt3.11 KBpwolanin
#25 contextual_links-2084463-25.patch26.18 KBpwolanin
#25 2084463-24-25.increment.txt2.62 KBpwolanin
#24 contextual_links-2084463-24.patch27.74 KBpwolanin
#24 2084463-22-24.increment.txt10.36 KBpwolanin
#22 interdiff.txt3.6 KBdawehner
#22 contextual_links-2084463-22.patch25.37 KBdawehner
#20 contextual_links-2084463-20.patch22.74 KBdawehner
#20 interdiff.txt3.65 KBdawehner
#17 contextual_links-2084463-17.patch19.66 KBdawehner
#6 drupal_2084463_6.patch19.29 KBXano
#1 contextual_links-2084463-1.patch20.15 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
20.15 KB

Let's see how much do actually break.

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-1.patch, failed testing.

Xano’s picture

Assigned: Xano » Unassigned
  1. +++ b/core/core.services.yml
    @@ -177,6 +177,9 @@ services:
    +    class: Drupal\Core\Menu\ContextualLinksManager
    

    Let's call the service plugin.manager.contextual_links (plural) for consistency with everything else.

  2. +++ b/core/includes/theme.inc
    @@ -1718,8 +1718,13 @@ function theme_links($variables) {
    +          if (isset($link['href'])) {
    

    Is URL path support supposed to be temporary, until everything is made route-based?

  3. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkBase.php
    @@ -0,0 +1,52 @@
    +  public function getTitle() {
    +    return $this->pluginDefinition['title'];
    

    Is there a reason why you opted for "title" rather than "label", which is what we use in most places in D8?

  4. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkBase.php
    @@ -0,0 +1,52 @@
    +    return isset($this->pluginDefinition['options']) ? $this->pluginDefinition['options'] : array();
    
    +++ b/core/lib/Drupal/Core/Menu/ContextualLinkBase.php
    @@ -0,0 +1,52 @@
    +    return isset($this->pluginDefinition['weight']) ? $this->pluginDefinition['weight'] : 0;
    

    We need an annotation class for these plugins and we can make the options definition default to an empty array, so we don't need these checks.

Xano’s picture

Assigned: Unassigned » Xano
neclimdul’s picture

Assigned: Unassigned » Xano

No annotation class because you aren't annotating. There's a defaults array in the manager. "options" is actually set in the defaults. weight is missing though.

Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
19.29 KB

Yup, that's what I found out too. My mind thought "Derivatives? Annotated classes!".

This patch adds a default value for the weight, removes an erroneous copypaste code comment, and some Views-related code that @dawehner told me to get rid of. I have to go somewhere, so I can't look into the test failures just yet, but let's see what the removal of that Views-related code does for them.

Status: Needs review » Needs work

The last submitted patch, drupal_2084463_6.patch, failed testing.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkInterface.php
    @@ -0,0 +1,57 @@
    +   *   The options as expected by LinKGeneratorInterface::generate()
    

    s/LinK/Link/

  2. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,116 @@
    +   * Gets the contextual links as array as expected by theme_links.
    

    Weird function doc.

  3. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,116 @@
    +      /** @var $plugin \Drupal\Core\Menu\ContextualLinkInterface */
    

    That's a weird comment.

  4. +++ b/core/modules/contextual/contextual.module
    @@ -256,10 +256,18 @@ function contextual_pre_render_placeholder($element) {
    +
    

    Unnecessary whitespace change.

  5. +++ b/core/modules/contextual/contextual.module
    @@ -349,8 +360,14 @@ function _contextual_id_to_links($id) {
    -    list($module, $parent_path, $path_args, $metadata_raw) = explode(':', $context);
    +    list($module, $parent_path, $path_keys, $path_args, $metadata_raw) = explode(':', $context);
    

    Why is this additional $path_args necessary?

  6. +++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualUnitTest.php
    @@ -33,11 +33,13 @@ function _contextual_links_id_testcases() {
    -          array('14031991'),
    +          array(
    +            'node' => '14031991',
    +          ),
    

    Without further explanation as to why it is necessary, my first impression is: "this is adding keys just for additional explicitness".

  7. +++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualUnitTest.php
    @@ -33,11 +33,13 @@ function _contextual_links_id_testcases() {
    -      'id' => 'node:node:14031991:',
    +      'id' => 'node:node:node:14031991:',
    

    And what's worse: the colon was used to separate each of the main components, but now it is apparently also being used to serialize the values of this component, so I don't see how this can be reliably deserialized. At least, this now seems to have become more complex/confusing.

pwolanin’s picture

I agree re: the serialization. Maybe we should just use a standard format like JSON instead?

Wim Leers’s picture

Or, why not stick to what we already have?

dawehner’s picture

The new routing system uses an parameter array all over the place which basically consists of values + keys with the name of the slug appearing in the route pattern.

This information is needed in order to generate the actual contextual links. so we somehow have to pass that information along. The patch just adds the keys of the parameters additional to the values.

Wim Leers’s picture

I think Peter has a point: the serialization used to be super simple, but now there's apparently a need for key-value pairs to be serialized as well. Then it might be wiser to just switch to JSON.

Furthermore, it seems that this is excessive now:

list($module, $parent_path, $path_keys, $path_args, $metadata_raw)

It seems we can get rid of $module now, since every route must be unique anyway, and we don't need to call a module-specific hook anymore? And shouldn't $parent_path be renamed to $route_name now?

Also, where/how are the contextual links being defined right now? The combination of ContextualLinkInterface being added and then not being implemented by any of the things that provide contextual links is very confusing.

Which brings me to my final point: the resulting code is now so opaque that I don't understand anymore what's going on. It'd be useful to explain the code flow in the issue summary.

Xano’s picture

Also, where/how are the contextual links being defined right now? The combination of ContextualLinkInterface being added and then not being implemented by any of the things that provide contextual links is very confusing.

Which brings me to my final point: the resulting code is now so opaque that I don't understand anymore what's going on.

Contextual links are defined through YAML. Any definitions will automatically be applied to a base class that implements ContextualLinkInterface. Any developers who wish to add more logic to their links or override the base class can add a "derivative" key to the YAML definitions which points to a derivative discovery class which can dynamically declare link definitions with their own "class" keys that point to other classes than the base class.

pwolanin’s picture

Well, and you can also just directly specify the class key to hae a single custom implementation. This is this sort of thing I'l do for local actions and maybe a contextual link for clone module when I upgrade it.

I aslo agreee with Wim that we can flatten this a bit, but the top level key is really the "group" not the route.

Wim Leers’s picture

#13: but I don't seen any routing YML file being changed in this patch yet? I'd like an explanation that covers the entire flow, from where developers define contextual links, to how it is rendered.

Wim Leers’s picture

#14: Yeah, I'm not sure anymore what's what in the current state of thing :) :(

dawehner’s picture

Regarding 3: This is the only proper way to typehint variables in code. This makes the code easier to understand as you exactly know what is expected here.

I haven't done the conversion to JSON as the JS somehow can't deal with the different IDs anymore etc.

Wim Leers’s picture

Interesting, I didn't know that! :)

I'm not surprised that the JS doesn't work as expected anymore, because I don't see how the modified serialization can work reliably.

What changed in #17? (Interdiff is missing.)

dawehner’s picture

#17 just fixed some of the points you mentioned but not the serialization parts. Sorry.

Do you think you can help on getting the JS side of it working?

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
22.74 KB

Let's see how much this fixes.

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-20.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
25.37 KB
3.6 KB

Fixed the remaining failures.

dawehner’s picture

#22: contextual_links-2084463-22.patch queued for re-testing.

pwolanin’s picture

Priority: Normal » Major
FileSize
10.36 KB
27.74 KB

Ok, I can see why JSON encoding is maybe not the best idea - it uses " chars which breaks out of the attribute, so they'd have to be escaped and then... ?

Here's a simpler approach that's already used in the module - just encode as for a query string (or form POST data) and use the parse_string() built-in.

Also, makes doc fixes and renames the "Base" class to "Default" to align with what we did with the LocalTaskDefault

to get the easy $this->t() for the default plugin we need: #2087231: Add a PluginBase in the Core namespace with t() as a helper method

pwolanin’s picture

oops, patch #22 had some test methods commented out. Fixing that and making variable naming more consistent.

pwolanin’s picture

Fixes some tests I missed

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -MenuSystemRevamp, -WSCCI, -VDC

The last submitted patch, contextual_links-2084463-26.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#26: contextual_links-2084463-26.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +MenuSystemRevamp, +WSCCI, +VDC

The last submitted patch, contextual_links-2084463-26.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
991 bytes
26.18 KB

missed one link ID in MenuTest

dawehner’s picture

Issue tags: +phpunit.
FileSize
10.33 KB
35.82 KB

Added a unit test.

pwolanin’s picture

Is there a module or system whose links we could convert now?

dawehner’s picture

  • Convert the blocks, custom_block as well as menu links
  • Some other things.

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -MenuSystemRevamp, -WSCCI, -VDC, -phpunit.

The last submitted patch, contextual_links-2084463-33.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +MenuSystemRevamp, +WSCCI, +VDC, +phpunit.

#33: contextual_links-2084463-33.patch queued for re-testing.

Wim Leers’s picture

Great progress here! I'll try to review soonish.

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-33.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
38.48 KB
  • Delete of blocks seemed to not have contextual links...

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-38.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.46 KB

Just a simple rerole.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in - the last few conversions can be done in a follow-up.

pwolanin’s picture

Issue tags: -Needs issue summary update, -MenuSystemRevamp, -WSCCI, -VDC, -phpunit.

#40: contextual_links-2084463-49.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +MenuSystemRevamp, +WSCCI, +VDC, +phpunit.

The last submitted patch, contextual_links-2084463-49.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.46 KB

Just a rerole.

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-49.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.5 KB

bla.

pwolanin’s picture

Re-roll for change to NodeRenderController.php in #1605290: Enable entity render caching with cache tag support

pwolanin’s picture

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -MenuSystemRevamp, -WSCCI, -VDC, -phpunit.

The last submitted patch, contextual_links-2084463-48.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review

#48: contextual_links-2084463-48.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +MenuSystemRevamp, +WSCCI, +VDC, +phpunit.

The last submitted patch, contextual_links-2084463-48.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
462 bytes
38.5 KB

Need to use the new Core not existing Component PluginBase to get the t() method.

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -MenuSystemRevamp, -WSCCI, -VDC, -phpunit.

The last submitted patch, contextual_links-2084463-52.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +MenuSystemRevamp, +WSCCI, +VDC, +phpunit.

#52: contextual_links-2084463-52.patch queued for re-testing.

pwolanin’s picture

Updating the summary.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

#52: contextual_links-2084463-52.patch queued for re-testing.

dawehner’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Looks good architecturally... Just a couple of remarks.

  1. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkInterface.php
    @@ -0,0 +1,57 @@
    +  /**
    +   * Returns the weight of the contextual link.
    +   */
    +  public function getWeight();
    

    Lacks @return

  2. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +   * Constructs a new ContextualLinksManager instance.
    

    ContextualLinkManager (singular)

  3. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +    foreach ($this->getContextualLinkPluginsByGroup($group_name) as $plugin_id => $plugin_definition) {
    +      /** @var $plugin \Drupal\Core\Menu\ContextualLinkInterface */
    +      $plugin = $this->createInstance($plugin_id);
    

    Are we allowed to do this now?

  4. +++ b/core/modules/contextual/contextual.module
    @@ -256,6 +256,17 @@ function contextual_pre_render_placeholder($element) {
    +  foreach ($element['#contextual_links'] as $module => $args) {
    +    $res = $contextual_links_manager->getContextualLinksArrayByGroup($args[0], $args[1]);
    +    if (!empty($res)) {
    

    Not introduced here, i know... But: abbreviated variable names--

  5. +++ b/core/modules/menu/menu.contextual_links.yml
    @@ -0,0 +1,4 @@
    +  title: 'Edit menu'
    

    Is 'title' really appropriate for contextual links? It's rather a 'label' really.

  6. +++ b/core/tests/Drupal/Tests/Core/Menu/ContextualLinkManagerTest.php
    @@ -0,0 +1,269 @@
    +   * The tested contextual link manager manager.
    

    manager manager.

  7. +++ b/core/tests/Drupal/Tests/Core/Menu/ContextualLinkManagerTest.php
    @@ -0,0 +1,269 @@
    +    $this->controllerResolver = $this->getMock('Symfony\Component\HttpKernel\Controller\ControllerResolverInterface');
    +    $this->pluginDiscovery = $this->getMock('Drupal\Component\Plugin\Discovery\DiscoveryInterface');
    +    $this->factory = $this->getMock('Drupal\Component\Plugin\Factory\FactoryInterface');
    +    $this->cacheBackend = $this->getMock('Drupal\Core\Cache\CacheBackendInterface');
    +
    +    $property = new \ReflectionProperty('Drupal\Core\Menu\ContextualLinkManager', 'controllerResolver');
    +    $property->setAccessible(TRUE);
    +    $property->setValue($this->contextualLinkManager, $this->controllerResolver);
    +
    +    $property = new \ReflectionProperty('Drupal\Core\Menu\ContextualLinkManager', 'discovery');
    +    $property->setAccessible(TRUE);
    +    $property->setValue($this->contextualLinkManager, $this->pluginDiscovery);
    +
    +    $property = new \ReflectionProperty('Drupal\Core\Menu\ContextualLinkManager', 'factory');
    +    $property->setAccessible(TRUE);
    +    $property->setValue($this->contextualLinkManager, $this->factory);
    +
    

    Sucks that we have to repeat this boilerplate code for every plugin manager unit test.

dawehner’s picture

Is 'title' really appropriate for contextual links? It's rather a 'label' really.

Mh, I see your point but I prefer consistency over correctness.

Sucks that we have to repeat this boilerplate code for every plugin manager unit test.

We could also extend the class and just add some setters but you know both are bad

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

RTBC if green.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Review of ##5, which I started during DrupalCon but was unable to finish. I apologize for the duplicate remarks, but considering I have many more remarks, the duplication should be limited.

  1. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    + * Defines a plugin manager to deal with contextual links.
    

    Contextual link plugin manager.

  2. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +    'weight' => NULL,
    

    Shouldn't this be 0? A weight of NULL seems weird.

  3. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +   * Constructs a new ContextualLinksManager instance.
    

    s/Links/Link/

  4. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +    parent::processDefinition($definition, $plugin_id);
    

    I personally find it more legible if there's a newline after a call to the parent class; but that might just be me.

  5. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +     // If there is no route name, this is a broken definition.
    ...
    +     // If there is no group name, this is a broken definition.
    

    Leading spaces.

  6. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +      throw new PluginException(sprintf('Plugin (%s) definition must include "route_name"', $plugin_id));
    ...
    +      throw new PluginException(sprintf('Plugin (%s) definition must include "group"', $plugin_id));
    

    Missing trailing periods.

  7. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +   *   A list of contextual links plugin definitions, which should be shown.
    

    The "which should be shown" part seems inaccurate?

  8. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +   * Gets the contextual links prepared as expected by theme_links.
    

    s/theme_links/theme_links()/

  9. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,153 @@
    +   *
    

    Extraneous newline.

  10. +++ b/core/modules/block/block.module
    @@ -115,7 +115,7 @@ function block_menu() {
    +    'context' => MENU_CONTEXT_NONE,
    

    Can't we delete all "context" properties of menu items?

  11. +++ b/core/modules/block/custom_block/custom_block.contextual_links.yml
    @@ -0,0 +1,10 @@
    +custom_block.block_edit:
    +  title: 'Edit'
    +  group: custom_block
    +  route_name: 'custom_block.edit'
    +
    +custom_block.block_delete:
    +  title: 'Delete'
    +  group: custom_block
    +  route_name: 'custom_block.delete'
    +  weight: 1
    

    I think this should include the language ID? How else will these be translatable? And: how would e.g. French Drupal developers be able to define these in French first, English second?

  12. +++ b/core/modules/block/custom_block/custom_block.module
    @@ -116,14 +116,11 @@ function custom_block_menu() {
    -    'route_name' => 'custom_block.delete',
    

    This seems wrong? However, I can still delete custom blocks … so maybe not?

    Please just double-check :)

  13. +++ b/core/modules/contextual/contextual.module
    @@ -256,6 +256,17 @@ function contextual_pre_render_placeholder($element) {
    +    $res = $contextual_links_manager->getContextualLinksArrayByGroup($args[0], $args[1]);
    

    What does $res mean? You're receiving contextual link plugin definitions. In the called function, it's called $contextual_links. At the very least, this variable should be named more clearly.

  14. +++ b/core/modules/contextual/contextual.module
    @@ -256,6 +256,17 @@ function contextual_pre_render_placeholder($element) {
    +  // @todo Remove once all contextual links are converted.
    

    Why not convert all contextual links? Follow-ups are not really accepted in this phase of D8 anymore AFAIK.

  15. +++ b/core/modules/contextual/contextual.module
    @@ -320,18 +331,14 @@ function contextual_contextual_links_view_alter(&$element, $items) {
    -    $id .= $module . ':' . $parent_path . ':' . $path_args . ':' . $metadata;
    +    $ids[] = "{$module}:{$parent_path}:{$path_args}:{$metadata}";
    

    Much, much better — thanks! :)

  16. +++ b/core/modules/contextual/contextual.module
    @@ -302,15 +314,14 @@ function contextual_contextual_links_view_alter(&$element, $items) {
    - *  - node:node:1:
    - *  - views_ui:admin/structure/views/view:frontpage:location=page&view_name=frontpage&view_display_id=page_1
    - *  - menu:admin/structure/menu/manage:tools:|block:admin/structure/block/manage:bartik.tools:
    + *  - node:node:node=1:
    + *  - views_ui:admin/structure/views/view:view=frontpage:location=page&view_name=frontpage&view_display_id=page_1
    + *  - menu:menu:menu=tools:|block:block:block=bartik.tools:
      *
      * So, expressed in a pattern:
      *  <module name>:<parent path>:<path args>:<options>
      *
    - * The (dynamic) path args are joined with slashes. The options are encoded as a
    - * query string.
    + * The path args and options are encoded as query strings.
      *
      * @param array $contextual_links
      *   The $element['#contextual_links'] value for some render element.
    @@ -320,18 +331,14 @@ function contextual_contextual_links_view_alter(&$element, $items) {
    
    @@ -320,18 +331,14 @@ function contextual_contextual_links_view_alter(&$element, $items) {
      *   use in a data- attribute.
      */
     function _contextual_links_to_id($contextual_links) {
    -  $id = '';
    +  $ids = array();
       foreach ($contextual_links as $module => $args) {
         $parent_path = $args[0];
    -    $path_args = implode('/', $args[1]);
    +    $path_args = drupal_http_build_query($args[1]);
    

    "path args" are no longer exactly what they used to be before. They appear to be route parameters instead now. Which is fine. But then we should update the variable names as well.

    More importantly, at least one of the parts of the pattern is now in fact the group name that is defined in the contextual links YAML files. But it's impossible to tell which: the module name, or the parent path? I think the parent path. If it's the parent path, then that variable name should be updated as well.

  17. +++ b/core/modules/menu/menu.module
    @@ -351,7 +351,7 @@ function menu_block_view_system_menu_block_alter(array &$build, BlockPluginInter
    -      $build['content']['#contextual_links']['menu'] = array('admin/structure/menu/manage', array($build['content'][$key]['#original_link']['menu_name']));
    +      $build['content']['#contextual_links']['menu'] = array('menu', array('menu' => $build['content'][$key]['#original_link']['menu_name']));
    
    +++ b/core/modules/node/lib/Drupal/node/NodeRenderController.php
    @@ -83,7 +83,7 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
    -      $build['#contextual_links']['node'] = array('node', array($entity->id()));
    +      $build['#contextual_links']['node'] = array('node', array('node' => $entity->id()));
    

    This structure change is most peculiar. Due to the values, it used to be very clear/explicit which is the module name and which is the path. Now it's impossible to tell which is which. Furthermore, I can't help but wonder why we need a nested array here?

pwolanin’s picture

re: #11 see: #2088371: YAML discovery incompatible with translations

Given that the breadcrumb patch went in, we should be able to delete most or all of the hook_menu entries.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.04 KB
39.17 KB

Nice review!!

Shouldn't this be 0? A weight of NULL seems weird.

NULL was used for local tasks in order to provide a different weight for the default tab, though yeah this is not needed here.

I personally find it more legible if there's a newline after a call to the parent class; but that might just be me.

I prefer that as well.

The "which should be shown" part seems inaccurate?

Yeah let's just drop that addition. The context is the function call, that's it.

Can't we delete all "context" properties of menu items?

You are totally right.

I think this should include the language ID? How else will these be translatable? And: how would e.g. French Drupal developers be able to define these in French first, English second?

Well, it was always the case in drupal that everything which is wrapped with t(), both in the .info file, .module/.php files or whatever is considered as some sort of English. There is not need to special case that here.

This seems wrong? However, I can still delete custom blocks … so maybe not?

Please just double-check :)

Ups ... this was a mistake.

Why not convert all contextual links? Follow-ups are not really accepted in this phase of D8 anymore AFAIK.

The problem is that we still have non-route menu_router entries.

This structure change is most peculiar. Due to the values, it used to be very clear/explicit which is the module name and which is the path. Now it's impossible to tell which is which. Furthermore, I can't help but wonder why we need a nested array here?

Well, we need the route parameters as additional array that is clear.

This structure change is most peculiar. Due to the values, it used to be very clear/explicit which is the module name and which is the path. Now it's impossible to tell which is which. Furthermore, I can't help but wonder why we need a nested array here?

I agree that this nested structure is odd. I guess the reason is to easily add new links to an existing module but yeah what about adding a follow up to improve that, so we don't have to touch existing non-converted code?

pwolanin’s picture

NULL as the default weight is consistent with FAPI and also allows us to distinguish the plugin setting it to 0 from it being the default.

We should go back to that - it's the correct default.

pwolanin’s picture

FileSize
677 bytes
Wim Leers’s picture

The problem is that we still have non-route menu_router entries.

I agree that this nested structure is odd. I guess the reason is to easily add new links to an existing module but yeah what about adding a follow up to improve that, so we don't have to touch existing non-converted code?

This sounds to me like this patch should (sadly :() be blocked on having completed routing system conversions? Only then will we be able to make all this very explicit, very clear, non-ambiguous?

dawehner’s picture

This sounds to me like this patch should (sadly :() be blocked on having completed routing system conversions? Only then will we be able to make all this very explicit, very clear, non-ambiguous?

I am against the approach of doing everything at once. You know, what really matters is progress and this can be achieved with this patch. Ripping out things once it is done is damn simple.

pwolanin’s picture

I'd agree - we are surely going to have to iterate a bit on this as we actually do the conversions, but we need something in place we can start working with.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Given that, let's put it back to RTBC.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
  1. This patch removes all access control from the contextual links system. For example, someone with permission to use contextual links but not to configure blocks will get a "Configure block" link on the block with this patch applied.
  2. +block_configure:
    +  title: 'Configure block'
    +  route_name: 'block.admin_edit'
    +  group: 'block'
    ......
       $items['admin/structure/block/manage/%block/configure'] = array(
         'title' => 'Configure block',
         'type' => MENU_DEFAULT_LOCAL_TASK,
    -    'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,
       );
    
    ================
    
    +custom_block.block_delete:
    +  title: 'Delete'
    +  group: custom_block
    +  route_name: 'custom_block.delete'
    +  weight: 1
    ...
       $items['block/%custom_block/delete'] = array(
         'title' => 'Delete',
         'weight' => 1,
         'type' => MENU_LOCAL_TASK,
    -    'context' => MENU_CONTEXT_INLINE,
         'route_name' => 'custom_block.delete',
       );
    

    I'm not particularly religious about this (especially because I was originally on the other side of the debate in Drupal 7) but this is adding extra work for developers; if they want their contextual links to have the same title and weight as regular links, they now need to duplicate that information in two places.

    See for example @sun's comments in #473268-139: D7UX: Put edit links on everything and elsewhere.

    It's quite possible the extra flexibility is worth it, but it's surprising that this issue has managed to sidestep that whole debate, and adding extra work for developers is something we should always be careful with. It's not actually obvious to me that contextual link definitions should be separated from regular link definitions (i.e. a large part of the premise of this issue), since they aren't really separate links so much as they are different presentations of the same links.

  3.    $items['block/%custom_block/delete'] = array(
         'title' => 'Delete',
         'weight' => 1,
         'type' => MENU_LOCAL_TASK,
    -    'context' => MENU_CONTEXT_INLINE,
         'route_name' => 'custom_block.delete',
    

    Why are you turning "Delete" into a tab? Shouldn't this entry just be removed?

  4. Are there any performance concerns with this patch? I'm noticing, for example, that menu_contextual_links() has static caching which this new system doesn't seem to repeat.
  5. +    $this->alterInfo($module_handler, 'contextual_links');
    

    If I'm reading this right, this is a totally new and different hook compared to drupal_alter('menu_contextual_links', $links, $router_item, $root_path) in menu_contextual_links().

    Seems like this needs to be documented.

    It's also cached (whereas hook_menu_contextual_links_alter() isn't) so if you want to use it to e.g. dynamically change the titles or otherwise dynamically edit the links, you'd be out of luck?

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.08 KB
47.96 KB

Thank for your feedback, highly appreciated.

This patch removes all access control from the contextual links system. For example, someone with permission to use contextual links but not to configure blocks will get a "Configure block" link on the block with this patch applied.

Added the functionality and a test.

I'm not particularly religious about this (especially because I was originally on the other side of the debate in Drupal 7) but this is adding extra work for developers; if they want their contextual links to have the same title and weight as regular links, they now need to duplicate that information in two places.

I totally see the point that coupling paths, menu links and routes into a single syntax helps to keep the amount of typing as low as possible. On the other hand though
we need to get rid of the old routing system. One additional point is that we already moved local actions and local tasks to its own file, so we should try to make things consistent and as simple as possible. Therefore
we sadly have to port many examples in order to find out common problems. (we did that for example for local actions which really helped).

Why are you turning "Delete" into a tab? Shouldn't this entry just be removed?

On this was not on purpose. Good catch, really!

Are there any performance concerns with this patch? I'm noticing, for example, that menu_contextual_links() has static caching which this new system doesn't seem to repeat.

We are caching the result of the contextual links by group:

  public function getContextualLinkPluginsByGroup($group_name) {
    if ($cache = $this->cacheBackend->get($this->cacheKey . ':' . $group_name)) {

which means that we end up with really a few cache entries (there are like ~5 groups in core at the moment). This could be a potential improvement for big sites can cache this information in memcache.

I think a static cache would totally make sense given that contextual links, on node for example, appear multiple times per page. Do you think we should get in the static cache already in this patch or open a follow up?

If I'm reading this right, this is a totally new and different hook compared to drupal_alter('menu_contextual_links', $links, $router_item, $root_path) in menu_contextual_links().

You are right, this is not about the actual rendering contextual links, but about the definitions of them. This is needed for some advanced usecases like for example views. I renamed the actual hook here.

It's also cached (whereas hook_menu_contextual_links_alter() isn't) so if you want to use it to e.g. dynamically change the titles or otherwise dynamically edit the links, you'd be out of luck?

The amount of alter hooks is a bit confusing. Is it enough to have one alter information for the static information and one for the links determined before returning to the rendering?

pwolanin’s picture

Do we really even need a run-time alter hook? These are plugins so if you need a dynamic title just define your class to provide it.

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-70.patch, failed testing.

David_Rothstein’s picture

Reading through the new patch those code changes look good overall.

I think a static cache would totally make sense given that contextual links, on node for example, appear multiple times per page. Do you think we should get in the static cache already in this patch or open a follow up?

Normally I'd say a followup (in the spirit of not adding static caches unless they're known to be necessary) but since there was already one in the existing menu_contextual_links() code I think it would be good to add here. And I think it was there for the reasons you mention above.

The amount of alter hooks is a bit confusing. Is it enough to have one alter information for the static information and one for the links determined before returning to the rendering?
...
Do we really even need a run-time alter hook? These are plugins so if you need a dynamic title just define your class to provide it.

I would say the second one (the dynamic one) is important to have; it's not just about dynamic titles but also about things like removing links based on certain conditions, etc. Given the second, perhaps the first (the static one) isn't necessary though.

+      if (!$this->accessManager->checkNamedRoute($route_name, $route_parameters)) {
+        continue;
+      }
+
       $links[$plugin_id] = array(
.....
+        'access' => $this->accessManager->checkNamedRoute($route_name, $route_parameters),
       );

Are both of these necessary? If so, it could be stored in a variable rather than determined twice.

+function hook_contextual_links_alter(array &$links, $group, array $route_parameters) {
...
+    $links['menu_edit']['title'] = 'Edit menu: ' . $menu->label();

Not sure offhand if this example is correct, either security-wise or translation-wise...

dawehner’s picture

Status: Needs work » Needs review
FileSize
47.89 KB
1.38 KB

Are both of these necessary? If so, it could be stored in a variable rather than determined twice.

The second one was a leftover from some of my experiments but I decided to not at the contextual link in the first place, as the old menu based system did the same.

Not sure offhand if this example is correct, either security-wise or translation-wise...

Can you think of a better idea what to provide as example?

I would say the second one (the dynamic one) is important to have; it's not just about dynamic titles but also about things like removing links based on certain conditions, etc. Given the second, perhaps the first (the static one) isn't necessary though.

I would like to keep the static one, as every plugin type in core basically has this alter hook.
For local tasks really special usecases like views have to provide local tasks based on other ones,
so you can use a static alter method.

This fixes the existing test errors ... aka. I have just ran the unit tests, shame on me.

Status: Needs review » Needs work
Issue tags: -MenuSystemRevamp, -WSCCI, -VDC, -phpunit.

The last submitted patch, contextual_links-2084463-74.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#74: contextual_links-2084463-74.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +MenuSystemRevamp, +WSCCI, +VDC, +phpunit.

The last submitted patch, contextual_links-2084463-74.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
47.47 KB

Rerolled.

Wim Leers’s picture

Status: Needs review » Needs work

#70: thank you so much for that review, it very clearly shows that this patch was definitely inappropriately RTBC'd earlier.


Translation handling is still opaque to me.

I agree that this nested structure is odd. I guess the reason is to easily add new links to an existing module but yeah what about adding a follow up to improve that, so we don't have to touch existing non-converted code?

I am against the approach of doing everything at once. You know, what really matters is progress and this can be achieved with this patch. Ripping out things once it is done is damn simple.

I agree that progress matters. But quite often, these kinds of follow-up tasks are not completed timely, which results in people not knowing how to make things right.
Furthermore, I still feel that the nested structure is highly problematic. It's very, very hard to understand. Previously, it was implicitly clear because of its values. But now that the values have become so similar, it's a puzzling mess.
Hence I ask again to let this patch wait until all non-route menu_router entries have been converted.


If I can still find problems in this patch so easily, then I don't think this should be RTBC'd unless David Rothstein and maybe another person reviews this patch another time after a reroll for these problems I just found:

  1. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkInterface.php
    @@ -0,0 +1,60 @@
    +   * Returns the group this contextual link should be rendered on.
    

    s/on/in/?

    Also, the concept of a "contextual link group" is not explained anywhere.

    And the important fact that it must be *unique* inside all of Drupal should also be mentioned, I think.

  2. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,175 @@
    +      throw new PluginException(sprintf('Plugin (%s) definition must include "route_name".', $plugin_id));
    ...
    +      throw new PluginException(sprintf('Plugin (%s) definition must include "group".', $plugin_id));
    

    s/Plugin/Contextual link plugin/ ?

  3. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,175 @@
    +   * @param array $route_parameters
    +   *   The incoming route parameters.
    

    So the route parameters must be the same for all contextual links. Which makes sense. But I think it's valuable to have that documented/explained explicitly here.

    It wasn't necessary before, because you could only do "node/X/OPERATION" things. But now that you can add arbitrary routes, i.e. arbitrary paths, so an additional explanation is warranted IMO.

  4. +++ b/core/modules/contextual/contextual.module
    @@ -320,18 +331,14 @@ function contextual_contextual_links_view_alter(&$element, $items) {
    +    $group = $args[0];
    +    $route_parameters = drupal_http_build_query($args[1]);
    +    $options = drupal_http_build_query((isset($args[2])) ? $args[2] : array());
    +    $ids[] = "{$module}:{$group}:{$route_parameters}:{$options}";
    

    So much better than before :)

  5. +++ b/core/modules/contextual/contextual.module
    @@ -349,8 +356,9 @@ function _contextual_id_to_links($id) {
    +    list($module, $parent_path, $args_raw, $metadata_raw) = explode(':', $context);
    

    Again, since that still hasn't been fixed:
    - parent path -> group
    - args_raw -> route_parameters
    - metadata_raw -> options

    In fact, why was $metadata renamed to $options? That is such a broad term, that it makes things even harder to grok.

  6. +++ b/core/modules/system/system.api.php
    @@ -979,6 +979,62 @@ function hook_menu_contextual_links_alter(&$links, $router_item, $root_path) {
    + * links may be added or existing links can be altered.
    

    s/or/and/

  7. +++ b/core/modules/system/system.api.php
    @@ -979,6 +979,62 @@ function hook_menu_contextual_links_alter(&$links, $router_item, $root_path) {
    + * Each contextual link must at least contain:
    

    s/must at least contain/must contain at least/

  8. +++ b/core/modules/system/system.api.php
    @@ -979,6 +979,62 @@ function hook_menu_contextual_links_alter(&$links, $router_item, $root_path) {
    + *   The contextual links group name being requested.
    

    The group of contextual links being rendered.

  9. +++ b/core/modules/system/system.api.php
    @@ -979,6 +979,62 @@ function hook_menu_contextual_links_alter(&$links, $router_item, $root_path) {
    + *   So for example it is an array like that:
    

    Replace all this with just "e.g."

  10. +++ b/core/modules/system/system.api.php
    @@ -979,6 +979,62 @@ function hook_menu_contextual_links_alter(&$links, $router_item, $root_path) {
    +    // Dynamically use the menu name for the title of the menu edit contextual
    

    s/menu edit/menu_edit/

  11. +++ b/core/modules/system/system.api.php
    @@ -979,6 +979,62 @@ function hook_menu_contextual_links_alter(&$links, $router_item, $root_path) {
    +    $links['menu_edit']['title'] = 'Edit menu: ' . $menu->label();
    ...
    +  $contextual_links['menu_edit']['title'] = 'Edit the menu';
    

    Should be wrapped in t()?

  12. +++ b/core/modules/system/system.api.php
    @@ -979,6 +979,62 @@ function hook_menu_contextual_links_alter(&$links, $router_item, $root_path) {
    + *   An array of contextual_links plugin definitions, keyed by plugin ID.
    

    It says "keyed by plugin ID" here, but AFAICT this is not a plugin ID, it's a "contextual link ID".

  13. +++ b/core/tests/Drupal/Tests/Core/Menu/ContextualLinkManagerTest.php
    @@ -0,0 +1,393 @@
    +   * Tests the getContextualLinkPluginsByGroup with a prefilled cache.
    

    "method" missing in this sentence.

  14. +++ b/core/tests/Drupal/Tests/Core/Menu/ContextualLinkManagerTest.php
    @@ -0,0 +1,393 @@
    +    // Setup mocking of the plugin factory.
    

    s/Setup/Set up/

  15. +++ b/core/vendor/phpunit/phpunit/PHPUnit/Framework/Comparator/Array.php
    @@ -165,7 +165,7 @@ public function assertEquals($expected, $actual, $delta = 0, $canonicalize = FAL
    -             'Failed asserting that two arrays are equal.'
    +             'Failed asserting that two arrays are equal. ' . print_r(array_diff_assoc($expected, $actual), TRUE)
    

    Accidentally ended up in patch, I think :)


Overall, I still feel this makes for an unnecessarily complex system. Mitigating facts:

  • very few developers will ever use this API
  • the old API was cumbersome as well

Only by rationalizing it this way, I can convince myself that it'd be acceptable for this to go in. So: I'm fine with this if e.g. David Rothstein is fine with it. However, I continue to think that nested array structure is a major WTF.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.83 KB
47.33 KB

Thank you very much for this long review.

Also, the concept of a "contextual link group" is not explained anywhere.

And the important fact that it must be *unique* inside all of Drupal should also be mentioned, I think.

Hopefully the new lines match your requirement.

So the route parameters must be the same for all contextual links. Which makes sense. But I think it's valuable to have that documented/explained explicitly here.

It wasn't necessary before, because you could only do "node/X/OPERATION" things. But now that you can add arbitrary routes, i.e. arbitrary paths, so an additional explanation is warranted IMO.

Right, this behavior is assumed by the full new routing system all over the place.

This for example have to be figured out in the generic views implementation of contextual links, which basically requires another bunch of code. This is one of the the reasons why I don't want to drop all
implementations in this patch. It just blocks itself.

Again, since that still hasn't been fixed:
- parent path -> group
- args_raw -> route_parameters
- metadata_raw -> options

In fact, why was $metadata renamed to $options? That is such a broad term, that it makes things even harder to grok.

Adapted. It seems to be for me that options are used in many more places in core, so people are more familiar with it than metadata.

Regarding 6 and 7, these have been copy and pasted lines of documentation, but nevermind, I just changed it.

Should be wrapped in t()?

Good idea.

It says "keyed by plugin ID" here, but AFAICT this is not a plugin ID, it's a "contextual link ID".

Now we are getting really nitpicky, don't we?

Accidentally ended up in patch, I think :)

Oh damn.

Overall, I still feel this makes for an unnecessarily complex system. Mitigating facts:

very few developers will ever use this API
the old API was cumbersome as well
Only by rationalizing it this way, I can convince myself that it'd be acceptable for this to go in. So: I'm fine with this if e.g. David Rothstein is fine with it. However, I continue to think that nested array structure is a major WTF.

Well, if we have converted all instances in core we should maybe switch to keyed parameters like this:

      $build[$key]['#contextual_links'][] = array(
        'module' => 'block',
        'group' => 'block',
        'route_parameters' => array('block' => $key)
      );
David_Rothstein’s picture

  1. -      $build[$key]['#contextual_links']['block'] = array('admin/structure/block/manage', array($key));
    +      $build[$key]['#contextual_links']['block'] = array('block', array('block' => $key));
    

    Yeah, I agree with Wim Leers that this is pretty confusing (the original is a bit confusing too, but at least you can read it left-to-right and understand it is forming the base of the URL).

    I think @dawehner's idea in #80 of using some kind of associative array with named parameters would help a lot. But maybe more like this?

    $build[$key]['#contextual_links']['block'] = array(
      'group' => 'block',
      'route_parameters' => array('block' => $key),
    );
    

    I left out the "module" key because it isn't actually used for anything. And yes, since the first "block" (the array key) isn't actually used for anything either, it could just be a numeric key as in #80, but that wouldn't be good for anyone who is trying to alter this. So it should be a meaningful string, and by convention it could just be the same as the group.

  2.     * @param array $route_parameters
    -   *   The incoming route parameters.
    +   *   The incoming route parameters. This route parameters need to have the
    +   *   same name on all contextual link routes, e.g. you cannot use node and
    +   *   entity in parallel.
    

    Following up on the discussion above, I wonder if that's actually true? I would expect that if you pass all possible route parameters along, multiple links within the same group could still use only a subset of them.

    So if you pass array('node' => $node, 'entity' => $entity) as the route parameters, and some contextual links within the group point to routes which require $node whereas others point to routes which require $entity, maybe it just works?

    By the way, (minor) in the above "This route parameters" should be "These route parameters".

  3. +    $this->moduleHandler->alter('contextual_links', $links, $group, $route_parameters);
    

    Doesn't work currently, since $group should be $group_name.

    I discovered that when looking into the next item....

  4. +function hook_contextual_links_alter(array &$links, $group, array $route_parameters) {
    .....
    +    $links['menu_edit']['title'] = t('Edit menu: @label', array('@label' => $menu->label()));
    +  }
    

    So I tried it out and actually it looks like we don't need to sanitize this. When I did it led to double-escaping (since ultimately this title gets passed into theme_links() which sanitizes the title). So perhaps we want !label in this example.

    I assume the translation is needed here, of course. Though I'm not sure I understand where regular (non-altered) link titles from the .yml file are getting translated, it does seem to be happening somewhere.

dawehner’s picture

I left out the "module" key because it isn't actually used for anything. And yes, since the first "block" (the array key) isn't actually used for anything either, it could just be a numeric key as in #80, but that wouldn't be good for anyone who is trying to alter this. So it should be a meaningful string, and by convention it could just be the same as the group.

As far as I remember this is used for some parts of the CSS but I agree we can drop it.

I do really like your alternative syntax.

So if you pass array('node' => $node, 'entity' => $entity) as the route parameters, and some contextual links within the group point to routes which require $node whereas others point to routes which require $entity, maybe it just works?

Sadly this is not the case. The symfony routing system has indeed some flaws like here. Every parameter which does not appear in the url pattern would be added as a query parameter, so you end up in
node/123?entity=123

Doesn't work currently, since $group should be $group_name.
I discovered that when looking into the next item....

Extended the unit test.

I assume the translation is needed here, of course. Though I'm not sure I understand where regular (non-altered) link titles from the .yml file are getting translated, it does seem to be happening somewhere.

These ones are translated in the getTitle() method of ContextualLinkDefault, see the following piece of code:

class ContextualLinkDefault extends PluginBase implements ContextualLinkInterface {

  /**
   * {@inheritdoc}
   */
  public function getTitle() {
    return $this->t($this->pluginDefinition['title']);
  }

Note: I haven't changed the syntax yet, as the examples of views and content_translation as not that easy to convert.

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-82.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
56.46 KB

There we go.

Status: Needs review » Needs work
Issue tags: -MenuSystemRevamp, -WSCCI, -VDC, -phpunit.

The last submitted patch, contextual_links-2084463-84.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +MenuSystemRevamp, +WSCCI, +VDC, +phpunit.

#84: contextual_links-2084463-84.patch queued for re-testing.

pwolanin’s picture

@David_Rothstein re: #81 - you say the array key is not used, but why don't we map that to the group name instead of making you also list the group?

so it could be as simple as:


$build[$key]['#contextual_links']['block'] = array(
  'route_parameters' => array('block' => $key),
);

or we could give the group a more distinct name:


$build[$key]['#contextual_links']['block_links'] = array(
  'route_parameters' => array('block' => $key),
);

dawehner’s picture

$build[$key]['#contextual_links']['block'] = array(
'route_parameters' => array('block' => $key),
);

I went we keeping the module name so you have 'node.node' => array('route_parameters' ...) to keep the changes as small as possible.

Additional this patch converts the content_translation as well as views_ui contextual links. There is now just a single place where the old
contextual links api is used. The remaining one is the dynamic support of configured views to provide a contextual link. I think we have to
change the UI in order to support group based contextual links.

aspilicious’s picture

While porting ds to drupal8. We had serious problems to insert a contextual links based on the 'view_mode', 'entity_type', ... and so on, on each entity. I hope this patch doesn't make it worse...

Didn't had the time yet to test it.

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-88.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
68.54 KB

Forgot to add some new files.

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-91.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
72.12 KB
9.99 KB

Fixed the test failures and whitespace.

tim.plunkett’s picture

  1. +++ b/core/includes/theme.inc
    @@ -1713,8 +1713,18 @@ function theme_links($variables) {
    +          // @todo theme_links() should *really* use the same parameters as l(),
    +          // and just take an array of '#type' => 'link' elements.
    

    Let's link this to #2102777: Allow theme_links to use routes as well as href directly, and remember to indent multiline @todos

  2. +++ b/core/modules/block/block.module
    @@ -124,6 +123,7 @@ function block_menu() {
       // that the plugin can appropriately attach to this URL structure.
    +  // that the plugin can appropriately attach to this URL structure.
    

    Weird double comment

  3. +++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.php
    @@ -240,7 +240,7 @@ public function testBlockContextualLinks() {
    -    $this->assertIdentical($json[$id], '<ul class="contextual-links"><li class="block-configure odd first"><a href="' . base_path() . 'admin/structure/block/manage/' . $block->id() . '?destination=test-page">Configure block</a></li><li class="views-ui-edit even last"><a href="' . base_path() . 'admin/structure/views/view/test_view_block/edit/block_1?destination=test-page">Edit view</a></li></ul>');
    +    $this->assertIdentical($json[$id], '<ul class="contextual-links"><li class="block-configure odd first"><a href="' . base_path() . 'admin/structure/block/manage/' . $block->id() . '?destination=test-page">Configure block</a></li><li class="views-uiedit even last"><a href="' . base_path() . 'admin/structure/views/view/test_view_block/edit/block_1?destination=test-page">Edit view</a></li></ul>');
    

    The class is changing from views-ui-edit to views-uiedit, is there any reason not to fix this?

  4. +++ b/core/modules/content_translation/content_translation.module
    @@ -152,7 +152,7 @@ function content_translation_menu() {
    -        'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,
    +        'context' => MENU_CONTEXT_PAGE,
    

    Why only kill one flag? I see this is done in other places, is it planned to also kill MENU_CONTEXT_PAGE in this issue or in a follow-up?

  5. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Plugin/Derivative/ContentTranslationContextualLinks.php
    @@ -0,0 +1,64 @@
    +class ContentTranslationContextualLinks  extends DerivativeBase implements ContainerDerivativeInterface {
    

    double space before extends

  6. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Plugin/Menu/ContextualLink/ContentTranslationContextualLinks.php
    @@ -0,0 +1,19 @@
    +class ContentTranslationContextualLinks extends ContextualLinkDefault {
    

    Why the extra class? Doesn't seem to be needed anymore.

  7. +++ b/core/modules/system/system.api.php
    @@ -840,6 +840,62 @@ function hook_menu_contextual_links_alter(&$links, $router_item, $root_path) {
    + * Alters the plugin definition of contextual links.
    

    Alter the plugin (not Alters in hook docs)

dawehner’s picture

Thank you very much for the review!

Weird double comment

The rule is simple: The more documentation the better :P

The class is changing from views-ui-edit to views-uiedit, is there any reason not to fix this?

Let's keep it for now.

Why only kill one flag? I see this is done in other places, is it planned to also kill MENU_CONTEXT_PAGE in this issue or in a follow-up?

MENU_CONTEXT_PAGE has its own dedicated issue: #2102125: Big Local Task Conversion

Why the extra class? Doesn't seem to be needed anymore.

Good catch!

Alter the plugin (not Alters in hook docs)

We are really good in being inconsistent.

tstoeckler’s picture

Status: Needs review » Needs work

Wow, this is a nice piece of work. I have a bunch of comments, but they are mostly minor. They do warrant a needs work in total, I think.

  1. +++ b/core/includes/theme.inc
    @@ -1713,8 +1713,19 @@ function theme_links($variables) {
    +            if (empty($link['route_parameters'])) {
    +              $link['route_parameters'] = array();
    +            }
    

    Super minor, but could be just

    $link += array('route_parameters' => array());
    </li>
    
    <li>
    <code>
    +++ b/core/lib/Drupal/Core/Menu/ContextualLinkInterface.php
    @@ -0,0 +1,67 @@
    +   * A contextual link group is a bunch of contextual link displayed together on
    +   * the page. For example the block group displays all links related with the
    +   * block, like the block instance edit link as well as the views edit link,
    +   * if it is a view block.
    

    I had a bunch of separated comments, but instead I'm just proposing an improved version of this comment. Feel free to take over whichever parts you like:

    A contextual link group is a set of contextual links that are displayed together on a certain page. For example, the 'block' group displays all links related to the block, such as the block instance edit link as well as the views edit link, if it is a view block.

  2. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkInterface.php
    @@ -0,0 +1,67 @@
    +   * The name of a group has to be unique.
    

    It would be cool if we could be more explicit about this. Unique among what? Per page? per route?

  3. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkInterface.php
    @@ -0,0 +1,67 @@
    +   * Returns the options based to the link generator.
    

    "based to" sounds weird. Maybe just "Returns an array of link options" as the @return already specifies what is meant. Maybe also add a @see to LinkGeneratorInterface::generate (with the FQNS, of course).

  4. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkInterface.php
    @@ -0,0 +1,67 @@
    +   * Returns the weight of the contextual link.
    

    As the weight, I assume, is relevant only within a single group, might be nice to add another sentence like "The contextual links in one group are sorted by weight for display." or something.

  5. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,179 @@
    +class ContextualLinkManager extends DefaultPluginManager {
    

    For extra credit, this would extend ContextualLinkManagerInterface which defines the added functions here, i.e. getContextualLinkPluginsByGroup() and
    getContextualLinksArrayByGroup().

    Since that would be a tad more work, and this is a rather pressing issue, I think it would be fine to do that in a follow-up.

  6. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,179 @@
    +   *   The incoming route parameters. The route parameters need to have the same
    +   *   name on all contextual link routes, e.g. you cannot use node and entity
    +   *   in parallel.
    

    Just for clarity I think it would make sense to put 'node' and 'entity' in quotes.

  7. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
    @@ -0,0 +1,179 @@
    +   *   A list of links as array, keyed by the plugin ID. Each entry is an
    +   *   associative array with the following keys:
    

    "links as array" sounds strange. Maybe: "An array of link information, keyed by the plugin ID. Each entry..."

  8. +++ b/core/modules/content_translation/content_translation.contextual_links.yml
    @@ -0,0 +1,3 @@
    +  weight: 100
    

    Is there a reason for this high weight? I think 'Translate' is one use-case for showing up between 'Edit' and 'Delete' in fact.

  9. +++ b/core/modules/contextual/contextual.module
    @@ -237,13 +238,15 @@ function contextual_pre_render_placeholder($element) {
    - *   keyed array. Each key is the name of the implementing module, and each
    - *   value is an array that forms the function arguments for
    - *   menu_contextual_links(). For example:
    + *   keyed array. Each key is the string "$implementing_module:$group".
    

    This is missing documentation for the array value. I.e. that is in array of 'route_parameters' and 'metadata'

  10. +++ b/core/modules/contextual/contextual.module
    @@ -302,15 +329,14 @@ function contextual_contextual_links_view_alter(&$element, $items) {
    + *  <module name>:<group>:<route parameters>:<options>
    
    @@ -320,18 +346,14 @@ function contextual_contextual_links_view_alter(&$element, $items) {
    +    $ids[] = "{$module}:{$group}:{$route_parameters}:{$metadata}";
    

    So should this be instead?

  11. +++ b/core/modules/system/system.api.php
    @@ -840,6 +840,62 @@ function hook_menu_contextual_links_alter(&$links, $router_item, $root_path) {
    + *   ID. Each entry contains the following keys:
    + *     - title: The displayed title of the link
    + *     - route_name: The route_name of the contextual link to be displayed
    + *     - group: The group under which the contextual links should be added to.
    + *       Possible values are e.g. 'node' or 'menu'.
    

    Also 'weight' I think and 'options'?

  12. +++ b/core/modules/system/tests/modules/menu_test/menu_test.contextual_links.yml
    @@ -0,0 +1,14 @@
    +menu_test.hidden_manage:
    ...
    +menu_test.hidden_manage:
    

    This can't be correct, I think.

dawehner’s picture

Status: Needs work » Needs review
FileSize
71.81 KB
9.79 KB

Great review!

A contextual link group is a set of contextual links that are displayed together on a certain page. For example, the 'block' group displays all links related to the block, such as the block instance edit link as well as the views edit link, if it is a view block.

Just took that bit as it is.

It would be cool if we could be more explicit about this. Unique among what? Per page? per route?

Well, i just wanted to make clear that all contextual links with the same group are rendered in the group. This though should be also somehow covered by the previous block of docs.

"based to" sounds weird. Maybe just "Returns an array of link options" as the @return already specifies what is meant. Maybe also add a @see to LinkGeneratorInterface::generate (with the FQNS, of course).

Just replaced it with "passed to"

For extra credit, this would extend ContextualLinkManagerInterface which defines the added functions here, i.e. getContextualLinkPluginsByGroup() and
getContextualLinksArrayByGroup().

Added the interface.

Is there a reason for this high weight? I think 'Translate' is one use-case for showing up between 'Edit' and 'Delete' in fact.

No there is not. Set back to 2 as it is in the hook_menu() entry.

This is missing documentation for the array value. I.e. that is in array of 'route_parameters' and 'metadata'

Fixed.

Also 'weight' I think and 'options'?

Added weight. Options is localized_options, ... we should clean that up later.

This can't be correct, I think.

Good catch.

tstoeckler’s picture

Thanks for the quick fixes.

Only one minor thing left to complain.

+++ b/core/lib/Drupal/Core/Menu/ContextualLinkManagerInterface.php
@@ -0,0 +1,49 @@
+  public function getContextualLinksArrayByGroup($group_name, array $route_parameters, array $metadata = array());
+}

Missing an empty newline.

For me this is RTBC, but I don't feel comfortable enough with the menu system and with contextual links to RTBC this myself.

dawehner’s picture

There we go, and hey someone can take the chance and RTBC it on comment 100.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed this in #94, and reviewed the interdiffs since, this is good to go.

#100 woooooo!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
+      $build[$key]['#contextual_links']['block.block'] = array(
+        'route_parameters' => array('block' => $key),
+      );

I'm going to say "block.block" is basically equivalent to the confusion that Wim Leers was worried about above...

This gets used in list($module, $group) = explode('.', $module_group) but I don't see how $module is ever used in a meaningful way? I would expect the group to always be enough to identify the group of links, so either like #81 (where 'group' is explicitly defined) or #87 (where the group is the array key) and not bothering with $module at all seem like they would be a lot simpler for module developers and with no apparent loss in functionality.

Also:

  * Examples:
- *  - node:node:1:
- *  - views_ui:admin/structure/views/view:frontpage:location=page&view_name=frontpage&view_display_id=page_1
- *  - menu:admin/structure/menu/manage:tools:|block:admin/structure/block/manage:bartik.tools:
+ *  - node:node:node=1:
+ *  - views_ui:admin/structure/views/view:view=frontpage:location=page&view_name=frontpage&view_display_id=page_1
+ *  - menu:menu:menu=tools:|block:block:block=bartik.tools:
  *
  * So, expressed in a pattern:
- *  <module name>:<parent path>:<path args>:<options>
+ *  <module>:<group>:<route parameters>:<metadata>

I'm not seeing how the changes in the examples match the changes in the pattern.

dawehner’s picture

Well, module is for example used in the code which still exists (menu_contextual_links()) which we cannot drop yet.
Battle-plan: Get this patch in, convert the last usage of hook_menu() based contextual links and then drop the $module. Would that be acceptable?

I'm not seeing how the changes in the examples match the changes in the pattern.

I adapted so that the views_ui example also has the new syntax.

xjm’s picture

Issue tags: +Prague Hard Problems
pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

I agree with both David and Daniel - we need to get this in, but also need to clean it up as a follow-up to be only based on group.

webchick’s picture

Sorry, but why can't the group thing be done in this patch? In theory we are in the phase of the release where we're trying not to actively break peoples' modules. This is already going to be a huge break, why cause another one a couple of weeks later?

webchick’s picture

Oh, ok. #102 explains the why. Can I counter-ask then why we can't leave this postponed until those things are fixed, so we can do this conversion once?

(Not moving from RTBC, this is just a question.)

dawehner’s picture

As explained in IRC the views bit is not simple to port.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Right. So I think basically the premise is, "This is holding up a critical [remove the old router system] and so we're asking to commit this as an interim step."

I think that's probably a fair request, but I think either David or Wim should probably be the ones to RTBC it, rather than a co-author of the patch.

That also by definition means this won't hit alpha4, but by holding it until alpha5, we have a chance to complete the views/contextual links follow-ups and remove the module names by then, so to module devs chasing alphas, it's just one API change, not two.

tim.plunkett’s picture

Issue tags: -phpunit. +PHPUnit

I RTBC'd this in #100.
@David_Rothstein bumped it in #101 because the $module/$group thing was confusing, but also because the example was wrong.
@dawehner fixed the example in #102 and explained why we need this in before fixing anything else

So the re-RTBC in #104 is just a hold-over from mine.

I would mark this RTBC again, but @webchick specifically asked for Wim or David to do that, so I'll sit on my hands for a bit :)

David_Rothstein’s picture

I'll take a look tomorrow.

David_Rothstein’s picture

I looked this over and I agree with @webchick in #105. It's not clear to me why we can't just change the API a single time now. Introducing $module as part of the new API only to remove it later shouldn't be necessary.

So I tried getting rid of it in the attached patch. Let's see how it goes with the testbot.

Also, the menu_contextual_links() call in #102 doesn't look like it would have ever worked? From the patch there:

-    $items += menu_contextual_links($module, $args[0], $args[1]);
....
+    $items += menu_contextual_links($module, $group, array_values($args['route_parameters']));

But menu_contextual_links() expects the second parameter to be a path, so I don't see how $group could have been the right thing.

Given that the tests still passed in #102, that makes me wonder if we can't just go ahead and remove that whole menu_contextual_links() function now too... But I left it in this patch (and fixed it) for the time being.

I made a couple other tiny fixes in the attached patch too, but the vast majority of it is just replacing $module.$group with $group.

One other thing I noticed:

-  elseif (!empty($element['#links']['views-ui-edit'])) {
+  elseif (!empty($element['#links']['views-uiedit'])) {

That looks like an unfortunate change (and there are a few others like it elsewhere). It wasn't obvious to me where this comes from (something to do with the plugin ID being used to build up the array of contextual links?).

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-111.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
21.5 KB
70.98 KB

OK, hm, so it looks like the tests in #102 only passed due to a coincidence. In the case of nodes, the $group is "node", which also happens to be the first part of the path for node URLs ("node/%/edit", etc). That is why the menu_contextual_links() call worked - but only in that specific case. Furthermore, the tests only pass because the test user doesn't have access to other node-related contextual links (besides the one provided by the view)... In real life, the Views functionality for contextual links is still broken.

I can make #111 work the same way - see attached. I think that will get the tests passing. However, I don't see the point of hacking core to make a test pass, if the functionality the test is trying to test doesn't work in practice... I think it's preferable to just remove the broken test.

For clarity, attached interdiffs are compared to #102 and to #111. Note that I restored one code comment ("A list of places where contextual links should be added") that the original patch changed but that shouldn't have been.

David_Rothstein’s picture

Here's the version that would make more sense to me - since it doesn't seem like we can make the Views stuff work at all in this issue and the only reason menu_contextual_links() was left behind was to try to make it work, just remove menu_contextual_links() instead and then leave a @todo about the broken Views functionality.

That's not great of course (it's a rather large @todo) but it's more honest about the state that this patch leaves Drupal in, plus it means we get the whole API change done at once.

Status: Needs review » Needs work

The last submitted patch, contextual_links-2084463-114.patch, failed testing.

tim.plunkett’s picture

I'm pretty sure I'd prefer "future API change" to "breaking all of Views contextual links".

EDIT: Seems I misunderstood.

OK, hm, so it looks like the tests in #102 only passed due to a coincidence.

Carry on.

dawehner’s picture

Status: Needs work » Needs review
FileSize
520 bytes
78.9 KB

The main reason why the contextual link part of views worked without issues on one of my patch was that the group "node" matched the pattern "node", now that I realized that.

The actual problem with removing the feature out of views temporarily is that we don't seem to have a clear idea how to implement
it inside the new system, because otherwise it would be part of the patch itself. I do have some potential implementations though which all require changes in the UI.

This just fixes the exceptions, ...

David_Rothstein’s picture

The actual problem with removing the feature out of views temporarily is that we don't seem to have a clear idea how to implement it inside the new system, because otherwise it would be part of the patch itself. I do have some potential implementations though which all require changes in the UI.

Yeah, exactly. It seems like the user interface would have to change to allow the administrator to choose a contextual links group?

---

By the way, since this discussion may be confusing to others, the functionality we're talking about is the one that allows you to configure page displays to appear as contextual links. For example:

  1. cp core/modules/node/tests/modules/node_test_views/test_views/views.view.test_contextual_links.yml core/modules/node/config/ (quick way to get a View that has this functionality)
  2. Install Drupal.
  3. Create a node and while logged in as an administrator look for the "Test contextual link" contextual link on it.

As far as I understand, all patches in this issue currently break that functionality.

David_Rothstein’s picture

Issue tags: -PHPUnit, -WSCCI, -VDC, -Prague Hard Problems

Also, I'm not sure how we left things with the caching discussion, but in testing this I did confirm that the following code:

+  public function getContextualLinkPluginsByGroup($group_name) {
+    if ($cache = $this->cacheBackend->get($this->cacheKey . ':' . $group_name)) {
+      $contextual_links = $cache->data;
+    }

does seem to be hitting the database cache an awful lot (for example, once for each node on the page, in the case of contextual links on nodes). So it's missing the static caching that menu_contextual_links() had to try to limit this to one database hit for all nodes on the page, and this would be a good thing to add (or at least make sure we have an issue to track adding it).

David_Rothstein’s picture

Issue tags: +PHPUnit, +WSCCI, +VDC, +Prague Hard Problems

Adding back the disappearing tags.

dawehner’s picture

dawehner’s picture

[16:39] dawehner: we can have the issue open and decide in there, but I have no problem temporarily removing that.

Here is also a follow up for the functionality in views: https://drupal.org/node/2117845

dawehner’s picture

Just another rerole.

Status: Needs review » Needs work
Issue tags: -PHPUnit, -MenuSystemRevamp, -WSCCI, -VDC, -Prague Hard Problems

The last submitted patch, contextual_links-2084463-123.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit, +MenuSystemRevamp, +WSCCI, +VDC, +Prague Hard Problems
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I think #121 addressed #119, and #2117845: Bring back contextual links support in views or drop it. is the follow-up discussed in #114.

I re-reviewed it, and it is once again RTBC.

David_Rothstein’s picture

I think so too, although #2117845: Bring back contextual links support in views or drop it. may need to be bumped in priority as a followup (currently the patch leaves the user interface in Drupal, but it doesn't work at all).

I also still find this kind of thing (from #111) confusing:

-  elseif (!empty($element['#links']['views-ui-edit'])) {
+  elseif (!empty($element['#links']['views-uiedit'])) {

It could be a followup, although that would be a (minor) API change to fix.

Gábor Hojtsy’s picture

I don't want to hold this down since this was *just* committed, but we'll need yet another followup to add title context support as it was just added to routes, tabs and actions in #2114069: Routing / tabs / actions lack way to attach context to UI strings. Contextual links need to match that.

dawehner’s picture

It could be a followup, although that would be a (minor) API change to fix.

Regarding that ... this is caused by the way how we name a single contextual link. I would prefer this change over manually working around that
by using a different plugin ID.

Let's be fair and just add the context support.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

That's a non-trivial diff, so let's get another set of eyeballs on it.

dawehner’s picture

Some problems which came indirectly to my mind.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed both interdiffs, and the changes are minimal and the unit tests are perfect.

Gábor Hojtsy’s picture

Agreed, let's get this in :) @dawehner: thanks a lot for covering context right away :)

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, contextual_links-2084463-134.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
85.19 KB
3.27 KB

Ah, needs account now.

Status: Needs review » Needs work

The last submitted patch, contextual-2084463-136.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
85.49 KB
1.77 KB

Just fixing the tests.

dawehner’s picture

Issue summary: View changes

Fix typos.

amateescu’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Related issues: +#2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items

The two interdiffs from #136 and #138 look good to me, so back to rtbc.

webchick’s picture

Issue tags: +alpha target

It'd be great to get this in for the next alpha release.

catch’s picture

Title: Convert contextual links to a plugin system similar to local tasks/actions » Change notice: Convert contextual links to a plugin system similar to local tasks/actions
Status: Reviewed & tested by the community » Active

Read through both the issue and the patch and I can't see major issues. Thanks for adding the static cache back in #121 was looking for that ;)

Committed/pushed to 8.x, thanks!

Will need a change notice.

Gábor Hojtsy’s picture

I'm trying to adapt config translation to this new API in #2130075: Config translation should work with new contextual links API. Honestly I'm having a hard time and I think the DX of the group system is not very good. So what I figured out by trial and error is basically the contextual links system is works with groups. You either establish a new group with links to be collected by altering the build #contextual_links array to include your group or you know an existing group name and add your links to that. Eg. content translation uses existing group names per entity name.

Now for config translation, we want to add contextual links to translate things at different places, eg. Views, blocks, etc. Whenever a contextual operations list may show up. So we don't really know the group names for those routes either (and I don't know if there would be a way for us to figure out, the contextual links are not defined based on their parent routes but rather their actual routes and the grouping system holds them together). We could also define new groups but that would mean we need to alter in all kinds of build arrays, which I'm not sure how is possible generically.

At this point I think I'm probably missing something since before this all we did is we included a key in our hook_menu() and the item fell into the right place being at the right parent-child point for the path. Now that is not an option at all and figuring out existing groups and/or defining new ones with build altering seems much harder.

How would a generic module like ours do this?!

Gábor Hojtsy’s picture

BTW for local tasks the grouping is based on tab root route ID, so while it is harder to do than D7, it can be looked up based on routes. Much more predictable, not a free-to-pick group name. For contextual links seems like the interaction of the build arrays and the groups is much less predictable to extend to me. :/

dawehner’s picture

I cannot respond to that at the moment (sorry), though this is the same reason why views could not be ported.

Gábor Hojtsy’s picture

@dawehner: I worked out a temporary solution for the most important ones in https://drupal.org/comment/8159639#comment-8159639, that should suffice there until we figure out how to make this work again in a general manner :)

thedavidmeister’s picture

Looks like the patch here conflicts with #2102777: Allow theme_links to use routes as well as href and makes the active class handling in theme_links on the LI element inconsistent between routes and href (whereas the patch in the other issue handles that correctly).

If we could get some extra eyes over there to finish that off and help reconcile the differences, that would be great.

Gábor Hojtsy’s picture

Documented the current process for contextual links to the best of my knowledge at https://drupal.org/node/2133283. This is NOT a change notice, it does not compare to Drupal 7, it compares to how local tasks / actions are done which are similar :)

Gábor Hojtsy’s picture

@dawehner: you suggested on #2130075: Config translation should work with new contextual links API to at least unify the contextual link group names for config entities per entity type. (The content entities are already unified like that I believe). I opened #2134841: Contextual link group names are not predictable for that. :)

xjm’s picture

xjm’s picture

Issue tags: +Needs change record
webchick’s picture

Issue tags: -alpha target

This got committed, so removing the alpha target tag.

pwolanin’s picture

Title: Change notice: Convert contextual links to a plugin system similar to local tasks/actions » Convert contextual links to a plugin system similar to local tasks/actions
Issue summary: View changes
Status: Active » Fixed
Issue tags: -Needs change record

Change notice posted: https://drupal.org/node/2165243

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.