Updated: Comment #N

Problem/Motivation

*.info.yml files support the configure property, which can contain the name of a route to which a link can be generated for use on the modules page. However, route parameters are not supported, so we can't provide links to any URL path within the system, which is a regression as compared to when the property still accepted a URL path instead of a route name.

For an example, see payment.payment_type.configure. Payments are entities and their bundles (also known as payment types) are provided by modules and cannot be deleted. If a module's main purpose is to provide a payment bundle, its configure link should point to the bundle's configuration page, which is the aforementioned route that requires a static parameter.

Proposed resolution

Add a configure_parameters property that can specify the parameters needed for a route.

Remaining tasks

None.

User interface changes

None.

API changes

None. The property is optional.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Assigned: Xano » Unassigned
FileSize
946 bytes
RoSk0’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: drupal_2147689_1.patch, failed testing.

The last submitted patch, 1: drupal_2147689_1.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
tim.plunkett’s picture

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
@@ -199,7 +199,8 @@ protected function buildRow(array $modules, $module, $distribution) {
+        $route_parameters = isset($module->info['configure_parameters']) ? $module->info['configure_parameters'] : array();

I didn't add this because it wasn't needed by core or any of the 15 top modules I checked.

If you want to add this, you'll need test coverage.

But also a justification of the use case...

The .info links are by definition static, what dynamic route could they possibly point to?

Xano’s picture

Issue summary: View changes

Updated the issue summary to address concerns raised in #7.

The .info links are by definition static, what dynamic route could they possibly point to?

Dynamic routes are impossible now and will remain impossible with this patch. This patch only introduces support for static route parameters. For a possible use case, see the issue summary.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

By "dynamic", I really meant "ones with route parameters" :)
The big worry here is about what happens if the placeholder is no longer valid. Like if you tried to link to admin/structure/types/manage/article, but the article content type was deleted.
So this should be used only when the route will always be valid, no matter what.

Xano’s picture

Xano’s picture

Title: Support route parameters for *info.yml files' "configure" route name » [regression] Support route parameters for *info.yml files' "configure" route name
Status: Needs work » Needs review
FileSize
4.49 KB
3.66 KB

The big worry here is about what happens if the placeholder is no longer valid. Like if you tried to link to admin/structure/types/manage/article, but the article content type was deleted.
So this should be used only when the route will always be valid, no matter what.

This same issue can surface in Drupal 7, so this is nothing we haven't had to deal with before, nor is it a regression.

The attached patch contains a test as well.

tim.plunkett’s picture

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/ModuleListFormWebTest.php
    @@ -0,0 +1,44 @@
    + * @file Contains \Drupal\system\Form\ModulesListFormWebTest.
    

    Two lines

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/ModuleListFormWebTest.php
    @@ -0,0 +1,44 @@
    +      'description' => '',
    

    ...

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/ModuleListFormWebTest.php
    @@ -0,0 +1,44 @@
    +    $user = $this->drupalCreateUser(array('administer modules'));
    +    $this->drupalLogin($user);
    

    One line

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/ModuleListFormWebTest.php
    @@ -0,0 +1,44 @@
    +   * Tests the form.
    +   */
    +  function testForm() {
    

    Which form? Also, public function

Xano’s picture

Issue tags: -Needs tests
FileSize
4.36 KB
1015 bytes

Done, except for the empty description. There is no need to repeat what the title already says. See #1941552: [Policy, no patch] Never require descriptions if a title/label is already required.

tim.plunkett’s picture

#1941552: [Policy, no patch] Never require descriptions if a title/label is already required has no consensus, and hasn't seen any activity in 9 months. Please add a description.

Xano’s picture

FileSize
1.06 KB
3.11 KB
tim.plunkett’s picture

Issue tags: +Needs tests

I'm not sure that was the best way to do that.

Xano’s picture

Issue tags: -Needs tests
FileSize
4.54 KB
2.82 KB

Test file wasn't strapped in. It fell off along the way.

Xano’s picture

FileSize
1.12 KB

Apologies. The previous interdiff was faulty. The patch is the right one, though.

The last submitted patch, 16: drupal_2147689_16.patch, failed testing.

Xano’s picture

16: drupal_2147689_16.patch queued for re-testing.

Xano’s picture

16: drupal_2147689_16.patch queued for re-testing.

Crell’s picture

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
@@ -198,12 +198,14 @@ protected function buildRow(array $modules, $module, $distribution) {
+      if ($this->accessManager->checkNamedRoute($module->info['configure'], $route_parameters, \Drupal::currentUser())) {

Please don't use \Drupal in an object. Get the user injected into the form.

Otherwise this looks good to me.

Xano’s picture

FileSize
6.18 KB
4.65 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, Xano.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/tests/modules/system_test/system_test.module
@@ -69,6 +69,9 @@ function system_test_system_info_alter(&$info, $file, $type) {
+  if ($file->name == 'system_test') {
+    $info['hidden'] = FALSE;
+  }

This looks dodgy to me because it means that the moment system_test is enabled by any test it will not be hidden. So this has the potential to bleed unexpected change into other tests since the only test that requires it not to be hidden is the new test added here.

Xano’s picture

What do you suggest? Also, if another module's tests depend on the visiblity of modules, doesn't that mean those tests are broken?

Xano’s picture

Bump.

Xano’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
1.29 KB

Status: Needs review » Needs work

The last submitted patch, 27: drupal_8414923_27.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
6.42 KB
1.44 KB

Status: Needs review » Needs work

The last submitted patch, 29: drupal_2147689_29.patch, failed testing.

Xano’s picture

29: drupal_2147689_29.patch queued for re-testing.

Xano’s picture

Status: Needs work » Needs review

The tests pass, and @alexpott's concern from #24 was addressed.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Xano’s picture

FileSize
6.75 KB

Re-roll.

Reject:

diff a/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.php	(rejected hunks)
@@ -45,7 +53,8 @@ public static function create(ContainerInterface $container) {
     return new static(
       $container->get('module_handler'),
       $container->get('keyvalue.expirable')->get('module_list'),
-      $container->get('access_manager')
+      $container->get('access_manager'),
+      $container->get('current_user')
     );
   }
 
@@ -59,10 +68,11 @@ public static function create(ContainerInterface $container) {
    * @param \Drupal\Core\Access\AccessManager $access_manager
    *   Access manager.
    */
-  public function __construct(ModuleHandlerInterface $module_handler, KeyValueStoreExpirableInterface $key_value_expirable, AccessManager $access_manager) {
+  public function __construct(ModuleHandlerInterface $module_handler, KeyValueStoreExpirableInterface $key_value_expirable, AccessManager $access_manager, AccountInterface $current_user) {
     $this->moduleHandler = $module_handler;
     $this->keyValueExpirable = $key_value_expirable;
     $this->accessManager = $access_manager;
+    $this->currentUser = $current_user;
   }
 
   /**
@@ -198,12 +208,14 @@ protected function buildRow(array $modules, $module, $distribution) {
     // Generate link for module's configuration page, if it has one.
     $row['links']['configure'] = array();
     if ($module->status && isset($module->info['configure'])) {
-      if ($this->accessManager->checkNamedRoute($module->info['configure'], array(), \Drupal::currentUser())) {
-        $item = menu_get_item(trim($this->url($module->info['configure']), '/'));
+      $route_parameters = isset($module->info['configure_parameters']) ? $module->info['configure_parameters'] : array();
+      if ($this->accessManager->checkNamedRoute($module->info['configure'], $route_parameters, $this->currentUser)) {
+        $item = menu_get_item(trim($this->url($module->info['configure'], $route_parameters), '/'));
         $row['links']['configure'] = array(
           '#type' => 'link',
           '#title' => $this->t('Configure'),
           '#route_name' => $module->info['configure'],
+          '#route_parameters' => $route_parameters,
           '#options' => array(
             'attributes' => array(
               'class' => array('module-link', 'module-link-configure'),

Xano’s picture

RTBC per #33, under the condition that the tests pass.

catch’s picture

Status: Reviewed & tested by the community » Needs work

There's absolutely no documentation added here about 'configure parameters' - ought to be possible to add that somewhere.

Patch also needs a re-roll.

Xano’s picture

Is the original configure parameter even documented anywhere in the code itself? As far as I know this has always only been documented in the online handbook, such as Writing module .info files (Drupal 7.x).

Xano’s picture

Status: Needs work » Reviewed & tested by the community

Catch and I agreed that this is okay for now, but that the handbook page about writing *info.yml files will need to be updated, and that we will open a follow-up to rename the configure and configure_parameters properties to something more seld-descriptive.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: drupal_2147689_34.patch, failed testing.

Xano’s picture

The follow-up is at #2228357: Rename *.info.yml file "configure" property to "configure_route_name" and it includes a change notice draft.

Xano’s picture

Status: Needs work » Needs review
FileSize
6.79 KB

Re-roll. Reject:

diff a/core/modules/system/tests/modules/system_test/system_test.module b/core/modules/system/tests/modules/system_test/system_test.module	(rejected hunks)
@@ -69,6 +69,9 @@ function system_test_system_info_alter(&$info, $file, $type) {
   if ($file->name == 'requirements1_test' || $file->name == 'requirements2_test') {
     $info['hidden'] = FALSE;
   }
+  if ($file->name == 'system_test') {
+    $info['hidden'] = \Drupal::state()->get('system_test.module_hidden', TRUE);
+  }
 }
 
 /**
Xano’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, under the condition that the tests pass.

webchick’s picture

Hm. :\ TBH I much prefer to see this issue go the other way, where we simply restore the ability to just add paths to the configure parameter. .info files are the easiest thing we have in all of Drupal, and this just further complicates them. Not moving down from RTBC though, since it sounds like catch / alexpott were on board.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: drupal_2147689_41.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
6.77 KB

Re-roll.

Hm. :\ TBH I much prefer to see this issue go the other way, where we simply restore the ability to just add paths to the configure parameter. .info files are the easiest thing we have in all of Drupal, and this just further complicates them. Not moving down from RTBC though, since it sounds like catch / alexpott were on board.

Inconsistency complicates things. This property is used for link generation and that is updated from using paths to route names all over Drupal. Unless we revert that, there is no reason not to commit this patch because of complexity. Besides that, writing *.info.yml files might be easy, they are useless without other code that is much more complex anyway.

Xano’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, under the condition that the tests pass.

catch’s picture

Status: Reviewed & tested by the community » Fixed

The advantage of using routes is that if you change the route definition to use a different path, all other code everywhere else works. Wherever we fall back to paths, that's somewhere that modules altering need to be concerned about, or modules updating their own paths have to copy/replace. So it is more clunky but it's not introduced by this issue.

Committed/pushed to 8.x, thanks!

  • Commit ef650e7 on 8.x by catch:
    Issue #2147689 by Xano: [regression] Support route parameters for *info....

Status: Fixed » Closed (fixed)

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