As part of a larger project to fix install profiles, this simple patch introduces .info files to allow the developer to specify dependencies and other meta-information.

This patch removes 3 profile specific hooks, namely hook_profile_modules, hook_profile_info and hook_profile_tasks, and greatly simplifies development of install profiles.

This file includes the patch and also adds 2 additional .info files for the existing profiles.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

eaton’s picture

Just poking my head in here. I'd like to help test and polish this next week. Thanks for getting the ball rolling!

tstoeckler’s picture

Probably a dependency for #395480: Plugin Manager in Core: Part 4 (installation profiles), which would (IF it gets in) un-break install profiles a great deal on its own.

moshe weitzman’s picture

Wow - this is a substantial cleanup. This makes it is way way easier to write an install profile and way easier for drupal.org and others to to do automatic packaging based on .info file contents.

I can verify that the patch does not apply cleanly for default.profile file.

adrian’s picture

Status: Needs work » Needs review
FileSize
11.54 KB

here's a cleaned up patch

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

Patch applies now but I am seeing no blocks after the seemingly successful install. Looks like a $page problem. Could be my environment though.

I call attention to setUp() method at http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/simpletest/drupal_.... Thats probably going to need changes as well. I think thats what the 'failed to run tests' message means from the test bot.

adrian’s picture

Status: Needs work » Needs review
FileSize
11.84 KB

I fixed a number of small bugs , such as neglecting to actually include the profile file and added the dependencies for the image, search and toolbar modules.

I also updated the simpletest module to use the profile file to retrieve the dependencies.

adrian’s picture

FileSize
12.97 KB

cvs ignored the new files i added. so once again.

moshe weitzman’s picture

Hooray. Bot is happy and so am I. I think we need one more solid review and then RTBC. I'll ask around.

starbow’s picture

exciting

Dave Reid’s picture

One of the nice features from the current code is I could add 'optional modules' that would be included in the list if there were found in the file system. Is there a way to do this with the .info method proposed here?

adrian’s picture

@12 You can't have conditionals in them, so for now no.

That is such an edge case anyway , and the code for that is not ideal.

There is _no_way_ that any packaging scripts from drupal.org would ever be able to handle packaging those dependencies, or automated scripts like plugin manager or drush would be able to pick up your requirements with an unexpected hook implementation such as that.

We could easily also add a 'suggested' or optional keyword, or you can do the enabling of those modules yourself in your code , OR once we have the install profiles having full access to the Drupal hook system you could do a hook_info_alter.

eaton’s picture

I'd vote for 'suggests' -- Module Supports already exists in contrib to capture that information for modules.

Gábor Hojtsy’s picture

Wow, I did not review the patch in detail but the changes to .info files and removal of install profile code looks to set a very good new standard!

Damien Tournoud’s picture

I'm not sure about this. Why not simply make Installation profiles a particular case of modules? Why not adding to the .info a profile = TRUE or something like this?

adrian’s picture

Damien : install profiles have 2 special features

1) they exist as a packaging and distribution mechanism, with separation of code, by adding an additional search path.
Namely they have their own modules and themes directory, which means multiple profiles or distributions can co-exist on the same code base.
We are unable to build nice packaging scripts for d.o at the moment because we need to execute the code to get at these dependencies.
2) they have some special interaction during install time
Namely they get picked up during install, and can have optional install tasks.

Just adding a profile = TRUE to modules might partly solve (2), but it would also necessitate porting the entire install tasks system into drupal itself.

It also does not solve (1), and in fact makes it impossible.

This patch is part of an initiative to make install profiles be a lot easier to write :
#509404: Fix some conceptual problems with install profiles and make them actually usable

With a few very small and simple patches install profiles will essentially be a superset of drupal modules, instead of a subset.

Damien Tournoud’s picture

(1) is nothing more then limitations of the packaging script in d.o. The solution is to introduce proper build files (for example based on Phing). That would also solve the "cannot distribute external code with packaged modules" issue. As far as I know, no one stepped in to do that.

(2) Indeed. Making profiles a special case of modules would require refactoring some of the install code. But in your proposal here, you are also doing that.

amc’s picture

Issue tags: +installation profiles

Tagging.

Gábor Hojtsy’s picture

Damien: well, after the three issues implicitly linked by Adrian in #17 got resolved, .profile files would not be much different to modules, except they would be (1) located elsewhere, (2) could have modules and themes in their directory tree and (3) would implement some install task hooks. The issues solve the .profile to become .module question not by adding an .info file key, but to instead keep using the .profile extension. Since (1) and (2) still separates install profiles from modules (and themes), install profiles are not modules or themes.

Damien Tournoud’s picture

Gabor: I'm more worried about the impact code-wise. I sense that we could refactor the installation system if we consider profiles just to be a special case of modules. The step by step approach proposed here could lead to duplication of most of the module management logic.

eaton’s picture

Gabor: I'm more worried about the impact code-wise. I sense that we could refactor the installation system if we consider profiles just to be a special case of modules. The step by step approach proposed here could lead to duplication of most of the module management logic.

I chatted with adrian briefly on IRC about this; in theory, profiles, modules, and themes in Drupal are all just different pieces of the same puzzle, with .info files being a common hub (at least, after this patch lands). template.php, profilename.profile, and modulename.module are all just different cases of 'the one file that is always loaded'.

I think that in the near term, getting this particular patch in cleans up the existing code, gives us additional capabilities (cleaner parsing of profile info) and paves the way for a more comprehensive 'merger' of modules, themes, and profiles in the future.

Jose Reyero’s picture

The patch installs cleanly and the installation seems to work too.

But I've tried adding one more (unexistent) module to the profile info and it installs the same without complaints (nor even notice) so it doesn't seem to properly check for the required modules. It seems to check for the modules' dependencies but not for the modules themselves.

Not sure whether this check should go into drupal_check_profile() or drupal_verify_profile().

Also I'm thinking: for a profile that just defines a list of modules, why do we need the .profile file at all? Cant we make it optional?

Anyway, I like very much this feature.

moshe weitzman’s picture

there is an issue open to let modules skip their .module file and just use .info. when that happens, i would think that .profile can follow suit. not a prerequisite IMO. it also is pretty rare.

adrian’s picture

FileSize
25.05 KB
15.79 KB

jose, it worked here (added noexist dependency , and got the resulting error)

Jose Reyero’s picture

Guess who was editing the wrong info file :D

Yes, it works. Sorry about the stupid bug report.

adrian’s picture

Status: Needs review » Reviewed & tested by the community

we now have 2 positive reviews, and this is ready to be committed.

eaton’s picture

It's not just *ready* to be committed. It's *begging* to be committed.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

A few minor clean-up things, and then I agree this is good to go. Great improvement!

1. "package" doesn't seem required in the .info files. It's not reflected in the UI at all.

2. When calculating the defaults, looks like elements such as 'dependents' and 'files' can also be removed.

3.

+ * Retrieve info about an install profile from it's .info file.

"its" ;)

4. Let's move that huge glomp of text from default_profile_tasks() moved to hook_default_profile_tasks in system.api.php.

Other than that, this looks GREAT!

adrian’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
17.79 KB

here's a new patch with those issues resolved.

Also, it's hook_profile_tasks, 'default' is the install profiles namespace (from default.profile).

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Excellent. Thanks muchly! I fixed a couple other minor comment things and then committed this to HEAD.

This patch is really exciting, because not only does it remove at least 80% of WTFs from writing installation profiles, it also makes it possible for real, live Drupal distributions to take hold. Nicely done!

Marking needs work for documentation. This needs to be noted in the 6.x => 7.x module upgrade guide.

moshe weitzman’s picture

Perhaps we should start an upgrade guide for install profiles. I don't see how this change affects module devs ... Thanks for the quick review and commit. Yes, exciting.

mikey_p’s picture

+1 for a separate guide for upgrading profiles to Drupal 7.

I'll see if I can start porting packgr to 7 soon, and see what breaks, as I think that profile makes more use of the new tasks features exposed to profiles in Drupal 6 than any other.

Damien Tournoud’s picture

Category: feature » bug
Priority: Normal » Critical

We moved from architecture review to committed in one night? Did someone actually read and tested the patch (except webchick, who made some useful remarks)?

+/**
+ * Retrieve info about an install profile from its .info file.
+ */
+function install_profile_info($profile, $locale = 'en') {

^ The parameters are not documented.

+  $cache =& drupal_static('install_profile_info', array(), TRUE);

^ The standard form for drupal_static is $cache = &drupal_static(__FUNCTION__, array());.

Why the $reset = TRUE?

+  $info = drupal_parse_info_file(sprintf('profiles/%s/%s.info', $profile, $profile)) + $defaults;

^ Erm. sprintf? for that?

+    ($locale != 'en' && !empty($locale) ? array('locale') : array()))

^ I know this was in the original code, but !empty($locale) && $locale != 'en would make a lot more sense here. Moving that to drupal_required_modules() would make a lot of sense too.

+    $info = install_profile_info($profile);
+    if (array_key_exists('tasks', $info)) {
+      $tasks += $info['tasks'];
     }

^ We use isset($info['tasks']) because it is way faster.

^ Task labels are supposed to be translatable (see just above $tasks['profile-install-batch'] = st('Install site');). How will that work now that those are in the .info file?

+    drupal_install_modules($profile_details['dependencies'], TRUE);

^ Everywhere in the patch, you use $profile_details['dependencies'] as the list of the modules to install. Reading the code of drupal_install_modules(), I'm not quite sure we are ordering the modules properly and if we are automatically enabling dependent modules.

scor’s picture

the patch which was committed breaks the module API test:

Fatal error: Call to undefined function drupal_get_profile_modules() in .../modules/simpletest/tests/module.test on line 26

module.test still uses drupal_get_profile_modules() which was removed with this patch.

  /**
   * The basic functionality of module_list().
   */
  function testModuleList() {
    // Build a list of modules, sorted alphabetically.
    $module_list = drupal_get_profile_modules('default', 'en');
    ...
moshe weitzman’s picture

@Damien - if you read the issue, you'll see that both jose , myself, webchick and adrian tested the patch. please don't imply otherwise.

adrian’s picture

Status: Needs work » Needs review
FileSize
8.72 KB

We moved from architecture review to committed in one night? Did someone actually read and tested the patch (except webchick, who made some useful remarks)?

Moshe, jose and webchick did.

+/**
+ * Retrieve info about an install profile from its .info file.
+ */
+function install_profile_info($profile, $locale = 'en') {

^ The parameters are not documented.

Done. see attached patch

+  $cache =& drupal_static('install_profile_info', array(), TRUE);

^ The standard form for drupal_static is $cache = &drupal_static(__FUNCTION__, array());.

Why the $reset = TRUE?

Fixed. I was not even using the $cache array. I am caching it now.

+  $info = drupal_parse_info_file(sprintf('profiles/%s/%s.info', $profile, $profile)) + $defaults;

^ Erm. sprintf? for that?

Fixed

+    ($locale != 'en' && !empty($locale) ? array('locale') : array()))

^ I know this was in the original code, but !empty($locale) && $locale != 'en would make a lot more sense here. Moving that to drupal_required_modules() would make a lot of sense too.

Could do that, but we'd need to parameterize drupal_required_modules, to do it right.
Will handle that in a follow up patch if that's what we decide.

+    $info = install_profile_info($profile);
+    if (array_key_exists('tasks', $info)) {
+      $tasks += $info['tasks'];
     }

^ We use isset($info['tasks']) because it is way faster.

done.

^ Task labels are supposed to be translatable (see just above $tasks['profile-install-batch'] = st('Install site');). How will that work now that those are in the .info file?

The same way module descriptions are translateable.
I changed it so it runs the titles through st() before printing, we might need to modify the string extraction stuff too, but do
they even cover install profiles to begin with ?

+    drupal_install_modules($profile_details['dependencies'], TRUE);

^ Everywhere in the patch, you use $profile_details['dependencies'] as the list of the modules to install. Reading the code of drupal_install_modules(), I'm not quite sure we are ordering the modules properly and if we are automatically enabling dependent modules.

The original code wasn't doing that either, although I agree we should do it. I'm surprised there isn't an existing issue for this.

from scor :

module.test still uses drupal_get_profile_modules() which was removed with this patch.

  /**
   * The basic functionality of module_list().
   */
  function testModuleList() {
    // Build a list of modules, sorted alphabetically.
    $module_list = drupal_get_profile_modules('default', 'en');
    ...

Fixed in this patch. All module tests pass.
Did a recursive grep and there are no more instances of this function.

adrian’s picture

FileSize
4.09 KB

Didn't pick up the conflict in system.api.php in the last patch, fixed now.

Damien Tournoud’s picture

@Moshe, please accept my apologies. That remark was over the edge.

Chris Johnson’s picture

Adrian: brilliant!

Damien Tournoud’s picture

Status: Needs review » Needs work

Thanks for the quick fixes, Adrian. Additional nitpicks:

+  $cache =& drupal_static('install_profile_info', array());

^ The convention for drupal_static() is literally $cache = &drupal_static(__FUNCTION__, array());: there is a space before the "=", and a literal __FUNCTION__.

+    $info = drupal_parse_info_file('profiles/'.$profile.'/'.$profile.'.info') + $defaults;

^ Elsewhere, we use "profiles/$profile/$profile.info". I think it makes more sense. Additionally, Drupal coding standards mandate to use a space before and after the concatenation operation.

catch’s picture

It's currently impossible to install HEAD, either with or without the latest patch, after modules are installed, you get 'Drupal already installed' instead of entering admin user information etc.

adrian’s picture

A note regarding dependencies :

The install profile uses drupal_install_modules , which installs the modules in the correct order, afaict : http://api.drupal.org/api/function/drupal_install_modules/7

What it doesn't do, is confirm that all the dependencies' dependencies are present.

This is because Drupal's API's don't do this either. The only 'dependency checking' I was able to find is baked very very deeply into the system_modules form, by disabling checkboxes that don't have dependencies present.

I think fixing this is kind of out of scope for this patch, as refactoring the module system is something that really shouldn't be done through an install profile patch like this.

Gábor Hojtsy’s picture

@catch: interesting, I was able to install from the d7ux repo, updated to latest HEAD after this patch and others were committed. So I am not sure this patch is the culprit.

adrian’s picture

It's installing fine here with the latest HEAD.

This code only kicks in after it's done the drupal already installed check, and also the automated tests would not be able to run.

Damien Tournoud’s picture

I don't have any installation issue either.

catch’s picture

Must be my very crufty old HEAD install, sorry for the noise.

adrian’s picture

The cvs commit scripts need to be modified, they don't allow .info files in contrib

** Access denied: this file is not allowed:
** contributions/profiles/hostmaster/hostmaster.info
** The only files allowed in contributions/profiles are .txt and .profile
** Packaged files (.gz, .tar, .tgz, and .zip) are not allowed anywhere.
** License files (LICENSE.txt) are not allowed, since everything committed
** to the Drupal CVS repository must be licensed under the GPL.
** You should never commit .svn/* or .bzr/* files to the CVS repository.
cvs commit: Pre-commit check failed
cvs [commit aborted]: correct above errors first!

Infrastructure issue : http://drupal.org/node/522088

adrian’s picture

Status: Needs work » Needs review
FileSize
4.04 KB

Here's an updated version of the patch

philipnet’s picture

subscribing.

moshe weitzman’s picture

Looks good. I would remove

+ * version = VERSION

Only core is suppose to use version keyword. Packaging script adds it for all of contrib. No sense in documenting it IMO.

webchick’s picture

Status: Needs review » Fixed

Committed with moshe's correction in #51. Thanks!

David_Rothstein’s picture

This is a pretty cool patch. I'm wondering, though - what was the rationale for moving tasks to the .info file along with everything else? It seems to me that they are much more of a code thing and therefore do not belong in an .info file.

The reason I ask is that I just posted a patch at #524728: Refactor install.php to allow Drupal to be installed from the command line which (necessarily, at the moment) moves task definitions back into the profile file, and I was wondering if there was some reason why that might be considered a step backwards? (As a side effect, the patch also hopefully makes custom installation tasks easier to write, but to do so we need to have them define a structured array of information, which is why I moved them back into the .profile file.)

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation, -installation profiles

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