Don't display install profiles in the Migrate UI

Minor fix to the IS.

#2859304: Show field type migrations correctly in Migrate Drupal UI landed, so this is now active.

A patch has arrived.

Nice work, thanks @quietone! I think it needs a test, though :)

Yep it does. How about this?

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
@@ -189,11 +191,13 @@ public function testMigrateUpgrade() {
-
+ exit();

Whoa, what? This looks like a remnant from a debugging session.

Oh my! Sorry about that.

I think the test coverage looks good enough. Let's proceed to get this merged. And I confirmed by reviewing the parent issue that we only want this to land on 8.5.x. No cherry-pick.

Can we add additional assertions to prove that Standard and Minimal are also not present?

Even better, we could compile a list of all available profiles and assert that none of them are present.

I don't see the advantage of testing for profiles that are not used in the test fixture. And how do we choose which profiles to include? Nonetheless, this patch adds a variable to list the install profiles and loops over that for the assertions. Currently, the list is limited to 'minimal' and 'standard'.

The idea is that, if we ever introduce a change that causes core profiles to be listed when they shouldn't be, we'll catch it. Future proofing! :)

Interdiff looks good, back to RTBC.

I'm not completely convinced we want to entirely disable profiles from providing an upgrade path. I was confused over in #2914974-74: Migrate UI - handle sources that do not need an upgrade, but what if instead of hiding profiles we just list them as not having an upgrade path? Or for that matter, themes?

Maybe I'm missing something?

I do agree that documentation, on d.o and possibly the upgrade page itself, to clarify that install profiles and themes are not upgraded would be helpful. How about a new issue for that and return this to RTBC?

Needed a reroll.

I'll review this.

Looks good, except for one thing:

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
@@ -48,6 +48,16 @@
+   * A list of install profiles.
+   *
+   * @var array
+   */
+  private static $install_profiles = [
+    'minimal',
+    'standard',
+  ];

The doc commment should be expanded a bit to explain what this property is used for (@see lines would be helpful), and it probably does not need to be static or private. A simple protected should do, and $install_profiles should be camelCased. (And of course, any mention of self::$install_profiles should be changed to $this->installProfiles.)

:)

Tagging novice since the feedback here seems quite easy to do.

Expanded comment added as requested in #17. This also needed a reroll and the interdiff is not included because it is larger than this small patch.

Looks good. Thanks!

this looks good, there are some test profiles in core too - should we be checking they're not in the list too?

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -675,9 +675,13 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +    foreach ($system_modules as $name => $info) {
   +      if ($info['type'] == 'profile') {
   +        unset($system_data['module'][$name]);
   +      }
   

   any reason not to use array_filter here?

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -675,9 +675,13 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
   +      if ($info['type'] == 'profile') {
   

   nit, we're dealing with strings, let's use ===

any reason not to use array_filter here?

I was going to point that out too, but I didn't see any real benefit to switching to array_filter() beyond the fact that it uses a closure. Ah well, it's no big deal either way.

1. No change due to response in #27
2. Fixed

Thanks!

From #26:

this looks good, there are some test profiles in core too - should we be checking they're not in the list too?

this looks good, there are some test profiles in core too - should we be checking they're not in the list too?

Good idea. Let's do it. We only need to check for one test profile ("testing" would be a good candidate).

I found a list of profiles in core/profiles and added each of those to the the list.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
@@ -45,6 +45,12 @@
+    'testing_config_import',
+    'testing_config_overrides',
+    'testing_missing_dependencies',
+    'testing_multilingual',
+    'testing_multilingual_with_english',

I think we can ditch these. Just checking for 'testing' is enough.

Okey dokey.

Bring it!

Adding review credits

Doesn't apply after #2914974: Migrate UI - handle sources that do not need an upgrade - tagging as needs re-roll

This oughta do it.

'Tis green. Sending back to RTBC since #38 is just a reroll.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
@@ -30,6 +30,24 @@
+  protected $installProfiles = [
+    'minimal',
+    'standard',
+    'testing',
+  ];

@@ -129,6 +147,13 @@ protected function translatePostValues(array $values) {
+    // Test that install profiles are not in the available or missing update
+    // path lists.
+    foreach ($this->installProfiles as $profile) {
+      $this->assertNotContains($profile, $available_paths);
+      $this->assertNotContains($profile, $missing_paths);
+    }

Rather than supplying a static list how about just checking to see if the list contains any profiles? That way we check all the things - even as we add a new profile (ie #2809635: Create experimental installation profile). Also this has the benefit of not just sticking random stuff in test class properties that are only used once.

@phenaproxima asked for more detail on #41.

For example the test could be:

// Ensure that all available/missing are modules and not install profiles.
$system_info = system_get_info('module');
foreach ($available_paths as $available) {
   $this->assertEquals('module', $system_info[$available]['type'];
}
foreach ($missing_paths as $missing) {
   $this->assertEquals('module', $system_info[$missing]['type'];
}

The issue title "Allow dynamic discovery of profiles on MigrateUpgradeForm" and the code which is removing profiles don't seem to add up.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
@@ -129,6 +129,15 @@ protected function translatePostValues(array $values) {
+      $this->assertEquals('module', $system_info[$available]['type']);
...
+      $this->assertEquals('module', $system_info[$missing]['type']);

I guess we need an if (isset($system_info[$avaiBLAHable]]) {

Add the isset as per #45.

Is this a better title?

Okay, the change looks good and makes sense to me. Let's get this in.

Wait a sec -- I just noticed that the patch in #46 has the test coverage, but is missing the actual change which was made in #43. Whoops!

This can be cherry picked to 8.5.x. :)

Oh, and this.

Rerolled patch to include the change made in #43.

Thanks, @starshaped! I'm happy with this. RTBC once Drupal CI passes it.

So in re-titling this issue to "Don't display install profiles in the Migrate UI" it made me think is there any reason why an install profile wouldn't have an migration. Yes the core one's don't but other profiles do way more. Also don't we have the same problem for modules that don't have anything at all to migrate?

Modules that do not have anything to migrate, e.g. help, overlay, has been addressed in #2914974: Migrate UI - handle sources that do not need an upgrade. There is now a list of such modules for Drupal 6 and Drupal7 and modules in that list will appear in the 'available' or 'to be upgraded' section of the review page. They are included there and not in a separate list for simplicity. Discussion on that can be found in #25 to #28 of #2914974: Migrate UI - handle sources that do not need an upgrade.

The intention is that other modules can add their modules to that list. That work, along with adding a 3rd category, incomplete, that other module can add to is being done in #2928147: Migrate UI - add 'incomplete' migration status.

I am not familiar enough with install profiles to discuss that, I'll leave that for others.

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -786,6 +786,13 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+      if ($info['type'] === 'profile') {
+        unset($system_data['module'][$name]);
+      }

Yeah, I think @alexpott is correct here, we should be able to use the extension discovery system here to check the path of the profile, and if its in core/profiles, exclude it, that way we don't exclude contrib-profiles.

Thanks!

@larowlan, thanks for explaining that. I didn't get that is what alexpott meant. I did some reading about extension discovery and I haven't learned how to do this. How does one get the path for the profile?

In the interim, you should be able to use drupal_get_path because the new extension stuff is 8.6 only.

Thanks. A bit confused by your comment since this issue is against 8.6.x. Nonetheless here is a version using drupal_get_path.

@quietone thanks for pointing me to #2914974: Migrate UI - handle sources that do not need an upgrade - looking at that my question is now why aren't we just adding standard and minimal to the lists held in MigrateUpgradeForm::$noUpdatePaths?

Can someone answer the question in #61? As I said in #56 I really don't know enough about install profiles to respond.

Nonetheless, I have an opinion. For me, adding an install profile to a list of modules is just wrong and might even cause confusion for some. If we wanted to add install profiles as an upgraded item, just like a module is an upgradeable item, that would require some more discussion in a new issue.

@quietone yeah install profiles are very icky but actually for the extension system install profiles are just a very special type of module and they can do everything a module can do and more. I agree install profiles represent an interesting challenge for migrate because it's not really possible to change an install profile on a site but putting them in a list of modules is definitely okay. That's exactly what is happening in everyone's core.extension:module configuration to install the install profile.

@alexpott, thx. I still disagree with putting an install profile in a list of modules on the Migrate UI but that is fine and that is not what this issue is about.

Looking back I see that this question has not been answered.

why aren't we just adding standard and minimal to the lists held in MigrateUpgradeForm::$noUpdatePaths?

I refrained from answering earlier but, oh, why not? Isn't it, in general, better to do this dynamically than have a static list? Might there be a new core profile in the future? And having worked on these UI patches, those lists were tedious to build and test. Anything that can be done to automate it is better.

@quietone I agree that maintaining the list is pain. For me I'd like to see the entire list go away and some solution that works for core & contrib is implemented - ie. #2932652: Migrate UI - handle extensions that can't be automatically upgraded. I think if we want to fix this quickly here hardcoding the profiles in the list is the way to go. Otherwise we should concentrate our efforts on #2932652: Migrate UI - handle extensions that can't be automatically upgraded and have a generic solution for this problem that is not core specific.

@alexpott, I think you are saying that handling profiles should be done in some generic way and should be discussed in #2932652: Migrate UI - handle extensions that can't be automatically upgraded. Is that right?

If so, do you prefer to a) keep the existing code

    // Remove core profiles from the system data.
    foreach (['standard', 'minimal'] as $profile) {
      unset($system_data['module'][$profile]);
    }

or b) hard code 'standard' and 'minimal' in the noUpgradePath list or c) or use this patch?

@quietone yep I think all of core's no upgrade path should use whatever is decided in #2932652: Migrate UI - handle extensions that can't be automatically upgraded - both modules and profiles.

But for now we should just maintain 1 list of things that don't have upgrade paths. So for this issue that is option b from #66 because then in #2932652: Migrate UI - handle extensions that can't be automatically upgraded we only have one place where we need to remove things.

@alexpott thanks for that explanation. I will make a new patch

Tagging

@alexpott, I was too quick to respond, I was so focused on working to understand what you meant that I missed the big picture. When an extension is added to noUpgradePaths it is treated as if it really does have a complete set of migrations. Then, if it is enabled on the source site it is included in the list of modules to be upgraded. Thus a core profile will appear in the list of modules to be upgraded.

Nonetheless, I worked up a patch, I don't know if it will pass all tests or not. During testing, 'standard', was still being displayed. I changed the loop to include a check if the extension was a profile that was installed on the destination and 'standard' no longer displays. But that isn't going to work either because it is looking at the profiles on the destination, not the source. If the source db has an install profile of 'minimal' and D8 is installed with standard then 'minimal' will be displayed in the list of modules to be upgraded.

It seems to me that the only way to remove the core install profiles from the display is to remove them from the array $system_data, built from the source db.

Sounds like we need more work based on the comment in #70.

I've got to admit this issue has been confusing me and I might have led it astray - sorry. @quietone wrote in #70

It seems to me that the only way to remove the core install profiles from the display is to remove them from the array $system_data, built from the source db.

And I looked at the code in HEAD and saw

    // Remove core profiles from the system data.
    foreach (['standard', 'minimal'] as $profile) {
      unset($system_data['module'][$profile]);
    }

So we are doing that already Core profiles do not appear in the list atm.

I think we need to go back to first principles and ask:
a) What can a profile do? I think the answer is in D7 and D8 is everything a module can. So therefore it is possible for them to provide an upgrade path.
b) How should a profile be listed that has no need of an upgrade path (eg. minimal and standard and probably many others.
c) How should a profile be listed if it has an upgrade path with migrations
d) How should a profile be listed than has an impossible upgrade path like Views be listed.
e) There is also a question that @quietone brings up about what to do when the source profile is different from destination profile since Drupal is already installed at this point.

Also just thinking out loud - is this really a major task? I think the majority of UI based migrations with be D7 standard to D8 standard and that already works correctly.

I agree that returning to first principles is a good idea here. In your points you

a) OK. Is there a list of modules that a profile installs somewhere useful?
b) If a profile installs a bunch of modules then it has an upgrade path. One that 'will be upgraded' if all the modules it installs have a complete set of migrations, otherwise it 'will not be upgraded'. A profile that does not install any modules would not have an upgrade path. I presume profiles can install themes, so what about those? They would not have an upgrade path because of the theme.
c) A good question. To answer that we need to think as someone who wants an easy couple of clicks upgrade. What is most important? I suggest it is a simple, 'will it work and if not, what do I do so I can upgrade'.
d) List such profiles in the will not be upgraded list.
e) Too difficult and too tired to process this case now.

Yes, not major.

Doesn't need to be assigned to me.

Need as summary update for the conversation between myself and alexpott in the last few comments.

IS updated.

When a site uses a profile how do you determine which modules are provided by the profile?

There is a patch here that, I suggest, is rerolled, once #2936365: Migrate UI - allow modules to declare the state of their migrations, and reviewed with the intention of committing. That way all profiles are not shown in the UI which provides consistency. But the larger questions in the IS still need to be addressed.