Updated: Comment #0

Problem/Motivation

Routing system is a new big thing so it is time to convert info file configure link to route name.

Proposed resolution

Convert info file configure link to route name.

Remaining tasks

Discussion and patch
Related code in \Drupal\system\Form\ModulesListForm

    // Generate link for module's configuration page, if it has one.
    $row['links']['configure'] = array();
    if ($module->status && isset($module->info['configure'])) {
      if (($configure = menu_get_item($module->info['configure'])) && $configure['access']) {
        $row['links']['configure'] = array(
          '#type' => 'link',
          '#title' => $this->t('Configure'),
          '#href' => $configure['href'],
          '#options' => array('attributes' => array('class' => array('module-link', 'module-link-configure'), 'title' => $configure['description'])),
        );
      }
    }

User interface changes

None

API changes

I don't think it is an api change.

https://drupal.org/project/issues/search/drupal?issue_tags=WSCCI-conversion

CommentFileSizeAuthor
#15 configure-route-2089511-15.patch14.11 KBtim.plunkett
#15 interdiff.txt1.14 KBtim.plunkett
#15 Screen Shot 2013-10-03 at 3.58.49 PM.png17.43 KBtim.plunkett
#12 drupal8.routing-system.2089511-12.patch14.04 KBRoSk0
#12 interdiff-2089511-8-12.txt323 bytesRoSk0
#9 drupal8.routing-system.2089511-8.patch14.04 KBRoSk0
#9 interdiff-2089511-2-6.txt1.5 KBRoSk0
#6 drupal8.routing-system.2089511-6.patch14.04 KBRoSk0
#6 interdiff-2089511-2-6.txt1.5 KBRoSk0
#2 drupal8.routing-system.2089511-2.patch14.83 KBRoSk0
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 commented
Assigned: Unassigned » RoSk0

Lets rock!

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 commented
Status: Active » Needs review
FileSize
drupal8.routing-system.2089511-2.patch14.83 KB

Initial patch.

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 commented

Forget to mention - I removed 'configure' link for toolbar module, there is no route for it and looks like this was added accidentally.

Status: Needs review » Needs work

The last submitted patch, drupal8.routing-system.2089511-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/includes/common.inc
@@ -3521,7 +3521,7 @@ function drupal_pre_render_link($element) {
-    $element['#markup'] = \Drupal::linkGenerator()->generate($element['#title'], $element['#route_name'], $element['#route_parameters'], $element['#options']);
+    $element['#markup'] = \Drupal::linkGenerator()->generate(t('Configure'), $element['#route_name'], $element['#route_parameters'], $element['#options']);

We should not change the title

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 commented
Status: Needs work » Needs review
FileSize
interdiff-2089511-2-6.txt1.5 KB
drupal8.routing-system.2089511-6.patch14.04 KB

Fixed review comments.

Status: Needs review » Needs work

The last submitted patch, drupal8.routing-system.2089511-6.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review 
  1. +++ b/core/modules/toolbar/toolbar.info.yml
@@ -7,4 +7,3 @@ version: VERSION
   - menu_link
-configure: admin/structure/toolbar

    What happened here? Oh I see, there is no configuration available for toolbar. 

  2. +++ b/core/modules/user/user.info.yml
@@ -5,4 +5,4 @@ package: Core
-configure: admin/config/people
+configure: user.account_settings

    user.admin_index:
  path: '/admin/config/people'

    You seem to have changed the used route name...

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 commented
FileSize
interdiff-2089511-2-6.txt1.5 KB
drupal8.routing-system.2089511-8.patch14.04 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, drupal8.routing-system.2089511-8.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Have a look at my review...

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 commented
Status: Needs work » Needs review
FileSize
interdiff-2089511-8-12.txt323 bytes
drupal8.routing-system.2089511-12.patch14.04 KB

Fixed review comments and rerolled.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

I manually reviewed each path/route_name replacement, looks good!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
@@ -193,12 +198,17 @@ protected function buildRow(array $modules, $module, $distribution) {
+          '#route_name' => $module->info['configure'],
+          '#options' => array(
+            'attributes' => array(
+              'class' => array('module-link', 'module-link-configure'),
...
+            ),
+          ),

Are $configure['description'] and $module->info['description'] really equivalent? I don't think so.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
Screen Shot 2013-10-03 at 3.58.49 PM.png17.43 KB
interdiff.txt1.14 KB
configure-route-2089511-15.patch14.11 KB

You're 100% right. HEAD uses the description of the menu link it points to, this patch uses the module description (which is already visible on the page).

I think we could chose to skip this completely, since its just a title text on a link in a collapsed details element, but if we want to keep it this is how.

Screen Shot 2013-10-03 at 3.58.49 PM.png

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

Awesome catch by @alexpott and nice fix by @tim.plunkett back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Convert info file configure link to route name » Change notice: Convert info file configure link to route name
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

This seems consistent with our use of routes over paths elsewhere.

Committed and pushed to 8.x. Thanks!

This'll need a change notice.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Title: Change notice: Convert info file configure link to route name » Convert info file configure link to route name
Assigned: RoSk0 » Unassigned
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Created https://drupal.org/node/2107171.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

I moved @jibran's change notice into the main one:
https://drupal.org/node/1935708#configure

Status: Fixed » Closed (fixed)

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

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

#2147689: [regression] Support route parameters for *info.yml files' "configure" route name tries to fix a small regression that was introduced by this issue.