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.
Comment | File | Size | Author |
---|---|---|---|
#49 | drupal_profile_info7.diff | 4.04 KB | adrian |
#38 | drupal_profile_info7.diff | 4.09 KB | adrian |
#37 | drupal_profile_info6.diff | 8.72 KB | adrian |
#30 | drupal_profile_info5.diff | 17.79 KB | adrian |
#25 | Picture 24.png | 15.79 KB | adrian |
Comments
Comment #2
eaton CreditAttribution: eaton commentedJust poking my head in here. I'd like to help test and polish this next week. Thanks for getting the ball rolling!
Comment #3
tstoecklerProbably 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.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedWow - 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.
Comment #5
adrian CreditAttribution: adrian commentedhere's a cleaned up patch
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedPatch 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.
Comment #8
adrian CreditAttribution: adrian commentedI 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.
Comment #9
adrian CreditAttribution: adrian commentedcvs ignored the new files i added. so once again.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedHooray. Bot is happy and so am I. I think we need one more solid review and then RTBC. I'll ask around.
Comment #11
starbow CreditAttribution: starbow commentedexciting
Comment #12
Dave ReidOne 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?
Comment #13
adrian CreditAttribution: adrian commented@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.
Comment #14
eaton CreditAttribution: eaton commentedI'd vote for 'suggests' -- Module Supports already exists in contrib to capture that information for modules.
Comment #15
Gábor HojtsyWow, 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!
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'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?
Comment #17
adrian CreditAttribution: adrian commentedDamien : 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.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commented(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.
Comment #19
amc CreditAttribution: amc commentedTagging.
Comment #20
Gábor HojtsyDamien: 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.
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedGabor: 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.
Comment #22
eaton CreditAttribution: eaton commentedI 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.
Comment #23
Jose Reyero CreditAttribution: Jose Reyero commentedThe 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.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedthere 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.
Comment #25
adrian CreditAttribution: adrian commentedjose, it worked here (added noexist dependency , and got the resulting error)
Comment #26
Jose Reyero CreditAttribution: Jose Reyero commentedGuess who was editing the wrong info file :D
Yes, it works. Sorry about the stupid bug report.
Comment #27
adrian CreditAttribution: adrian commentedwe now have 2 positive reviews, and this is ready to be committed.
Comment #28
eaton CreditAttribution: eaton commentedIt's not just *ready* to be committed. It's *begging* to be committed.
Comment #29
webchickA 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.
"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!
Comment #30
adrian CreditAttribution: adrian commentedhere's a new patch with those issues resolved.
Also, it's hook_profile_tasks, 'default' is the install profiles namespace (from default.profile).
Comment #31
webchickExcellent. 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.
Comment #32
moshe weitzman CreditAttribution: moshe weitzman commentedPerhaps 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.
Comment #33
mikey_p CreditAttribution: mikey_p commented+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.
Comment #34
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe moved from architecture review to committed in one night? Did someone actually read and tested the patch (except webchick, who made some useful remarks)?
^ The parameters are not documented.
^ The standard form for drupal_static is
$cache = &drupal_static(__FUNCTION__, array());
.Why the $reset = TRUE?
^ Erm. sprintf? for that?
^ 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.^ 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?^ 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.Comment #35
scor CreditAttribution: scor commentedthe patch which was committed breaks the module API test:
module.test still uses drupal_get_profile_modules() which was removed with this patch.
Comment #36
moshe weitzman CreditAttribution: moshe weitzman commented@Damien - if you read the issue, you'll see that both jose , myself, webchick and adrian tested the patch. please don't imply otherwise.
Comment #37
adrian CreditAttribution: adrian commentedMoshe, jose and webchick did.
Done. see attached patch
Fixed. I was not even using the $cache array. I am caching it now.
Fixed
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.
done.
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 ?
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 :
Fixed in this patch. All module tests pass.
Did a recursive grep and there are no more instances of this function.
Comment #38
adrian CreditAttribution: adrian commentedDidn't pick up the conflict in system.api.php in the last patch, fixed now.
Comment #39
Damien Tournoud CreditAttribution: Damien Tournoud commented@Moshe, please accept my apologies. That remark was over the edge.
Comment #40
Chris Johnson CreditAttribution: Chris Johnson commentedAdrian: brilliant!
Comment #41
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks for the quick fixes, Adrian. Additional nitpicks:
^ The convention for drupal_static() is literally
$cache = &drupal_static(__FUNCTION__, array());
: there is a space before the "=", and a literal __FUNCTION__.^ 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.Comment #42
catchIt'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.
Comment #43
adrian CreditAttribution: adrian commentedA 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.
Comment #44
Gábor Hojtsy@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.
Comment #45
adrian CreditAttribution: adrian commentedIt'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.
Comment #46
Damien Tournoud CreditAttribution: Damien Tournoud commentedI don't have any installation issue either.
Comment #47
catchMust be my very crufty old HEAD install, sorry for the noise.
Comment #48
adrian CreditAttribution: adrian commentedThe 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
Comment #49
adrian CreditAttribution: adrian commentedHere's an updated version of the patch
Comment #50
philipnet CreditAttribution: philipnet commentedsubscribing.
Comment #51
moshe weitzman CreditAttribution: moshe weitzman commentedLooks 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.
Comment #52
webchickCommitted with moshe's correction in #51. Thanks!
Comment #53
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.)