Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

See #2313651: [policy, no patch] Clarify policy on the inclusion of Migrate in Core for 8.0.0 for background.

In order to provide transparency to site builders and developers, the decision was made in that issue to explicitly denote the Migrate modules as "Experimental." This reflects their current state, which is that we are not applying the same level of rigour in terms of maintaining backwards-compatibility as other areas of Drupal 8, nor have they been tested as throughly since there is not (yet) a UI for site builders to test.

The idea is that we can remove this "Experimental" distinction once Migrate is considered complete and release-ready. This will hopefully happen in 8.1.0.

CommentFileSizeAuthor
#20 migrate-experimental-FOR-REAL-2560597-20.patch844 byteswebchick
#17 2015-09-06_003014.png2.53 KBhass
#13 migrate-experimental-FOR-REAL-2560597-13.patch887 byteswebchick
#6 Screen Shot 2015-08-31 at 2.43.10 PM.png23.51 KBwebchick
#6 migrate-experimental-2560597-6.patch1.89 KBwebchick
#2 Screen Shot 2015-08-31 at 12.42.35 PM.png49.43 KBwebchick
#2 migrate-experimental-2560597-2.patch870 byteswebchick

Comments

webchick created an issue. See original summary.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Active » Needs review
StatusFileSize
new migrate-experimental-2560597-2.patch870 bytes
new Screen Shot 2015-08-31 at 12.42.35 PM.png49.43 KB

Here's a patch, and a screenshot.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 8.1.x-dev » 8.0.x-dev
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Pre-emptive RTBC from me.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

I think we could do with a hook_requirements() warning for if there are any experimental modules enabled - for example you might have to uninstall before each core update then re-install again etc.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
StatusFileSize
new migrate-experimental-2560597-6.patch1.89 KB
new Screen Shot 2015-08-31 at 2.43.10 PM.png23.51 KB

Let's try this.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Note the screenshot is slightly off, since the patch says "for testing purposes only." (adds the word only)

The last submitted patch, 2: migrate-experimental-2560597-2.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Two thumbs up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 496f553 and pushed to 8.0.x. Thanks!

+++ b/core/modules/system/system.install
@@ -50,6 +50,22 @@ function system_requirements($phase) {
+        'value' => t('Experimental modules found: %module_list. Experimental modules are provided for testing purposes only. Use at your own risk.', array('%module_list' => implode(', ', $experimental))),

I thought about asking for the comma separated item list for accessibility here - but it does not handle complete inline-ness that well atm so not doing that.

  • alexpott committed 496f553 on 8.0.x 
    Issue #2560597 by webchick: Mark Migrate* modules as Experimental
hass’s picture

hass commented
Status: Fixed » Active

This issue introduced a bug. On my status page I now get told Experimental modules enabled, but these are NOT enabled. They are on the system, but not enabled!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new migrate-experimental-FOR-REAL-2560597-13.patch887 bytes

Whoops! Right you are! This is why no one should ever trust my code. ;)

hass’s picture

hass commented 
+++ b/core/modules/system/system.install
@@ -53,10 +53,11 @@ function system_requirements($phase) {
+      if ($module_data[$module]->info['package'] === 'Core (Experimental)') {

Isn't Core (Experimental) not a translatable string?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Good question, I have no idea if module packages are translatable or not.

hass’s picture

hass commented

In D7 it is...

hass’s picture

hass commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2015-09-06_003014.png2.53 KB

Yes, it is... but here in code it is still English and does not break. I hope that is still true, if someone may not use English on a site.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/system.install
@@ -53,10 +53,11 @@ function system_requirements($phase) {
+    $module_data = system_rebuild_module_data();

Looking at this again (oops), I don't think there is any need to rebuild the module data here. We can just call system_get_info('module', $module); inside the loop.

hass’s picture

hass commented

Aside... With marking this experimental we need to provide D6 security support until after a migration is stable + 3-6 months. Otherwise people cannot upgrade as they lose data what is really no option.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Needs review
StatusFileSize
new migrate-experimental-FOR-REAL-2560597-20.patch844 bytes

I knew that was probably not right. Thanks, that feels much nicer. :) Also believe I verified that the t() around the 'Core - Experimental' is not needed; $this->t() is added dynamically in core/modules/system/src/Form/ModulesListForm.php:

    // Add a wrapper around every package.
    foreach (Element::children($form['modules']) as $package) {
      $form['modules'][$package] += array(
        '#type' => 'details',
        '#title' => $this->t($package),
        '#open' => TRUE,
        '#theme' => 'system_modules_details',

#19 would be something to take up with security team, but AFAIK it's pretty much set in stone at 3 months past 8.0.0's ship date https://www.drupal.org/d6-lts-support FWIW though there are at least a few people who are running serious migrations against D6 and largely succeeding. The D6 -> D8 migration path has been in HEAD for a year or so; it's D7 => D8 that's still very much in progress.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

I'm not sure if I'm the most qualified person to RTBC this, but it looks OK to me.

nerdcore’s picture

nerdcore commented

I have tested #20 and it works as advertised. The erroneous message indicating I have experimental modules enabled when I do not is now gone.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 72ed272 and pushed to 8.0.x. Thanks!

  • alexpott committed 72ed272 on 8.0.x 
    Issue #2560597 followup by webchick, hass: Mark Migrate* modules as...

Status: Fixed » Closed (fixed)

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

cilefen’s picture

cilefen commented

The committed patch does not test properly if the module package key is set, which leads to #2839844: Notice: Undefined index: package in system_requirements() (line 60 of core\modules\system\system.install). .