Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chertzog’s picture

Status: Active » Needs review

Forgot to set to need review.

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community

Go!

cweagans’s picture

So, it would have been really awesome to have some context for this patch. I realize that it was detailed on the other issue, but just looking at this one, there's not really indication that it should actually be committed.

That said, I checked the function and $file isn't actually used anywhere, so this is safe to commit.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks like legit code, but if you follow git blame you see that this was actually added elsewhere. Sorry, I can't remember exactly where.

Committed and pushed to 7.x. Thanks!

pillarsdotnet’s picture

Title: update_fix_d7_install_profile() » Fix undefined variable error in update_fix_d7_install_profile()
Status: Fixed » Needs review
FileSize
840 bytes

Reviewing the original issue at #509398: Install profiles should be modules with full access to the Drupal API and all it entails(.install files, dependencies, update_x), I believe that a mistake has been made.

Compare an early patch from #22:
diff --git modules/system/system.module modules/system/system.module
index 4ed15c7..7a9d830 100644
--- modules/system/system.module
+++ modules/system/system.module

@@ -1783,6 +1792,9 @@ function _system_get_module_data() {
     drupal_alter('system_info', $modules[$key]->info, $modules[$key]);
   }
 
+  // The install profile is required.
+  $modules[$profile]->info['required'] = TRUE;
+
   return $modules;
 }
 
... with its subsequent revision in #25:
diff --git includes/update.inc includes/update.inc
index c72e085..b29c204 100644
--- includes/update.inc
+++ includes/update.inc
@@ -255,10 +255,80 @@ function update_fix_d7_requirements() {
     }
   }
 
+  update_fix_d7_install_profile();
+
   return $ret;
 }
 
 /**
+ * Register the currently installed profile in the system table.
+ *
+ * Install profiles are now treated as modules by Drupal, and have an upgrade path
+ * based on their schema version in the system table.
+ * 
+ * The install profile will be set to schema_version 0, as it has already been
+ * installed. Any other hook_update_N functions provided by the install profile
+ * will be run by update.php.
+ */
+function update_fix_d7_install_profile() {

...


+    // Read profile info file
+    $info = drupal_parse_info_file(dirname($filename) . '/' . $profile . '.info');
+
+    // Merge in defaults.
+    $info = $info + array(
+      'dependencies' => array(),
+      'dependents' => array(),
+      'description' => '',
+      'package' => 'Other',
+      'version' => NULL,
+      'php' => DRUPAL_MINIMUM_PHP,
+      'files' => array(),
+    );
+
+    // The install profile is always required.
+    $file->info['required'] = TRUE;

...

From the context, history, and comment, I believe that last line should actually read:
+    $info['required'] = TRUE;

Patch attached.

droplet’s picture

YesCT’s picture

Issue summary: View changes
Status: Needs review » Needs work

Please open an separate issue for the follow-up changes in #5.

Might be tricky though, not sure if needs to be done in 8.x.

Leaving it at Needs work until the separate issue is created (or verified it is not needed).