Wrong export link for migration_group

Thanks for the report! That's interesting... Let's see how the code is deciding on the link URL.

So this is in config_update_ui/src/Controller/ConfigUpdateController.php . On around line 600-630, it is making the URLs. It makes the URL for export using :

      $routes = [
        'export' => 'config.export_single',
...
    ];
      foreach ($actions as $action) {
        $links[$action] = [
          'url' => Url::fromRoute($routes[$action], ['config_type' => $entity_type, 'config_name' => $id]),
          'title' => $titles[$action],
        ];
      }

So basically here, it's calling

Url::fromRoute('config.export_single', ['config_type' => $entity_type, $config_name' => $id]);

It looks like the problem is in $entity_type, which is getting the value (I'm presuming) 'migration' instead of 'migration_group'.

So that's up above around line 543:

      $entity_type = $this->configList->getTypeNameByConfigName($name);

Here, $name should be migrate_plus.migration_group.sweden, and $this->configList is an instance of the config lister service. So let's look at that class... That is the src/ConfigLister.php file, and the method looks like this:

  public function getTypeNameByConfigName($name) {
    $definitions = $this->listTypes();
    foreach ($this->typesByPrefix as $prefix => $entity_type) {
      if (strpos($name, $prefix) === 0) {
        return $entity_type;
      }
    }

OK, there's the bug. There are presumably config prefixes 'migrate_plus.migration' as well as 'migrate_plus.migration_group'. We are finding a match for the shorter prefix, and we need to hold out for a better match on the longer prefix. So instead of just returning when it finds a match, it should save that as a possibility, and keep looking in case it can find a longer match. Or, we should match on $prefix . '.', which would actually probably be better, because the prefix should be followed by a . in the name. Or maybe both -- append a . and look for the longest match possible.

Anyway, I think that's probably the cause. Thanks again for the report! I'll see about fixing the bug and also adding a test to make sure the problem doesn't crop up again.

It looks like the ConfigLister::omitKnownPrefixes() method would suffer from the same problem:

 protected function omitKnownPrefixes($list) {
    $prefixes = array_keys($this->typesByPrefix);
    $list = array_combine($list, $list);
    foreach ($list as $name) {
      foreach ($prefixes as $prefix) {
        if (strpos($name, $prefix) === 0) {
          unset($list[$name]);
        }
      }
    }

There are 2 Drush commands that use strpos matching, but interestingly, both of them match on prefix with '.' appended, so they are probably fine.

I don't see anything else in the project using strpos, except some of the tests, so that's probably it.

I think this patch will fix the problem. Can you test on your site? Also will trigger running automated tests. Then we will also need to add an automated test that detects the problem, and demonstrate it fails without patch, passes with.

You might want to wait until our automated tests pass before risking a patch on your site. :)

OK, that didn't break any of the automated tests, so that's good. It would be great to know if this fixes the reported issue.

So... we also need a test. I looked at all the existing config items in core modules, themes, and profiles in config/install and config/optional, as well as test modules. I don't think there is a single instance of a prefix that is a subset of another prefix in Core. So we'd have two options for testing:

a) Test the bug directly by creating a test module that defines two types of config with overlapping prefixes, and adding to the ConfigUpdateTest (which tests the report output) to make sure the export links are correct with those types of config items.

b) Create a unit test for ConfigLister that includes a test for the two methods that are problematic that would verify prefixes work correctly in this case. At the moment we don't have a unit test for ConfigLister at all. See #2863853: Add more unit tests.

Or both. :)

Excellent, thanks for testing!

So all this patch needs then is a test.

Well, I have a test, which tests all the public methods in ConfigLister.

I ran into a weird problem that seems to be a test artifact, but I think the test will now pass. Uploading:
- Patch with test included
- Test without patch to verify the test fails

Let's see if the bot agrees with my local results...

OK, this passed/failed as expected. Committed with a couple of coding standards fixes suggested by the bot.