One of the major problems with install profiles, is that they do not have access to the Drupal API, and are _only_ loaded during install.
This results in almost every install profile having to write their own custom contrib module, to do the actual work the install profile aims to do.
I propose
a) always loading the install profile code
b) adding the install profile namespace to the bottom of module_listing
This will give install profiles access to the full Drupal API, and work towards removing the differences between install profiles and modules.
Install profiles thus become a superset of the Drupal API and not a subset, and you have to jump through fewer hoops to write install profiles.
Comment | File | Size | Author |
---|---|---|---|
#32 | profile_registry.patch | 31.02 KB | adrian |
#29 | profile_registry.patch | 31.02 KB | adrian |
#26 | Picture 32.png | 12.25 KB | adrian |
#25 | profile_registry.patch | 30.9 KB | adrian |
#22 | profile_registry.patch | 27.4 KB | adrian |
Comments
Comment #1
rickvug CreditAttribution: rickvug commentedtagging
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedIs it too much to make an install profile an actual module that implements distribution=TRUE in its .info file? Not sure what that implies for the UI of the modules page.
Comment #3
joshmillerYes!
Comment #4
robeano CreditAttribution: robeano commentedI agree with moshe. When you consider the adrian's other proposed changes to install profiles (adding .info and .install support, see http://drupal.org/node/509404), it sure seems like install profiles are becoming modules.
Comment #5
adrian CreditAttribution: adrian commentedI've looked at this problem a bit, and so far I can determine a few that need to be done.
1) the install profile needs to be registered in the system table
2) install profiles need to be loaded as part of bootstrap
3) the hook registry needs to look for hooks and includes in the profiles dir too.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commented3) for that matter, i think themes should be able to implement hooks. a different issue, for sure.
Comment #7
webchickMoshe's comment at #2 is intriguing to me. *Is* there any reason not to do away with the confusing concept of "installation profiles" altogether in favour of a simple .info file flag for modules?
I think the only thing install profiles can do that modules can't is ship with themes (since both install profiles and modules can include sub-modules). I wonder if it's less work to allow modules to ship with their own themes (which might provide other additional interesting opportunities) than it is to shoe-horn installation profiles into the module system. And I guess finding the possible install profiles would take longer, since you'd be doing a file_scan_directory() through all of the modules folders, gathering up .info files, and filtering the list. But this is something that happens exactly once in the entire lifetime of your Drupal site, so this performance hit seems negligible compared to what feels like might be a substantiative DX improvement.
I can't really think of other cons. Adrian, what say you?
Comment #8
adrian CreditAttribution: adrian commented@moshe : theoretically, i can perhaps see the point , except that in no uncertain terms that would completely break the separation between the display and business logic.
@webchick : no. not now, not ever. modules and installation profiles are _completely_ _totally_ and _utterly_ different things.
You should read what dries had to say about install profiles : http://buytaert.net/drupal-distributions
At a very high level, modules are interchangeable and configurable bits of functionality that (should) make no direct assumptions on who is using them. Installation profiles are the mechanism by which assumptions are made about how these modules are going to be used in a cohesive system.
The primary reason install profiles have a bad reputation is because the only real mechanisms for making these assumptions is the Drupal API, but unfortunately the Drupal API is not available to these profiles. We have been using the install_profile_api as a band-aid for this problem, but it has still been very painful.
In the most basic sense install profiles are glue code and this issue is not about shoehorning the profiles into the module api, it is about generalizing the module API to cover more of Drupal, to allow us to finally collaborate in a sensible way to build Drupal for intranets, etc. etc.
On a much lower level, the most important part of an install profile that differentiates it from a module is that it provides it's own search path.
The relevant code is here : http://api.drupal.org/api/function/drupal_system_listing/7
The simplest concrete example that I can give regarding this (that also illustrates why we need versioned dependencies) the following :
Install profile A : provides a set of default panels for the Panels 2.x module
Install profile B : provides a set of default panels for the Panels 3.x module
In your approach, the module would need to be installed in sites/all/modules, but you can not install 2 versions of the same module in the same location. So you would need to know what install profiles are running on which sites and place the relevant version of the module for each site in the correct sites/default/module. Now multiply that by 10 000 sites running various install profiles.
So, if you want to go deeper and want to do nested packaged versions of the right modules for each 'install profile module' to be placed in sites/all/module, you would no longer be able to do the simple system_listing to get the overrides, you would need to find the active install
profile, get all the results for it, do the recursive search on the rest of the tree and replace the results from the last search with the results of the first search. And this would need to be done every time the system table is updated for every 'thing' that drupal manages.
Plus you need to throw in the fact that sites installed with a certain install profile _should_not_ be able to see the modules in the other install profile. This makes finding the right module, the right tpl.php , the right whatever , orders of magnitude more complex.
And the only benefit you would get from this is that you can drop your packaged 'install profile module' into the modules directory instead of the profiles directory.
The end result of this is essentially that multiple install profiles would not be able to easily exist in the same drupal directory and most people would end up having a separate drupal directory for each profile they run. This is just a nice way of saying we have semi-forked the code base by forcing the end user to do the forking themselves whenever they run the code, while needlessly and endlessly complicating their maintenance and their ability to keep their systems up to date.
Keep in mind this is the very simplest example I could have chosen to illustrate this point, and the simple fact is that the main idea behind install profiles are sound, but the current implementation isn't. What install profiles are right at this moment is not what they should actually be, but we can actually fix this.
I feel that there is such a large amount of cognitive dissonance around the nature of install profiles and distributions that many people do not understand fundamental considerations such as why you aren't able to switch install profiles on a site that has already been installed and why you can't have 'multiple' install profiles running on a site.
As part of my holy crusade to make install profiles work I will also be working towards educating people about all these issues, but this issue queue is not the place to do that.
Until I get these concepts communicated in a more adequate venue, please accept that:
install profiles are not modules. To make them useful they need to be more like modules, but we can not do away with install profiles altogether.
Comment #9
acsubscribing
Comment #10
chx CreditAttribution: chx commentedWith all that said, install profiles still should be modules in the profiles directory. It makes things a lot and a LOT easier. Instead of a lot of special casing, we have everything granted. The only thing is that we do not add profiles as a normal search path but only deal with one, hardwired module from there. Edit: this last sentence makes no sense, see following comments.
Comment #11
chx CreditAttribution: chx commentedMoshe points out that we already have the profile as a search path and yeah,
$searchdir[] = "profiles/$profile/$directory";
indrupal_system_listing
. If this is the case, then why on earth not make 'em a module? #509404-10: Fix some conceptual problems with install profiles and make them actually usable says thataye and if it would be a module then that would be granted immediately.
Comment #12
chx CreditAttribution: chx commentedTo clarify, Adrian's chain of arguments is built on "In your approach, the module would need to be installed in sites/all/modules" which is not true, it sits happily in profiles/foo and all the modules that it requires would ALSO sit in profiles/foo which is already supported.
Comment #13
eaton CreditAttribution: eaton commentedChiming in to support this as well; although I suspect it won't make ti for D7, it's an important step towards maintainable Drupal distros.
Comment #14
adrian CreditAttribution: adrian commentedThis patch adds the install profile to the results returned from system_get_module_data, and therefore adds the profiles to the system table.
The existing default_profile_setup was no longer necessary, and is now default_install in the new default.install file.
This patch also resolves:
#80272: Make install profiles updatable
I did not touch the existing wizard code, and it will likely still work but there is no core install profile i can test it with.
The patch I want to write for the following issue will almost completely replace this system, but it should still be working :
#525594: Installation should consist of 2 phases instead of one
There are some small inconsistencies, specifically in the user interface where the install profile is referred to as a module, but the code is incredibly simple, and works during install, update and normal operation.
I'm not sure, but i think this also allows modules to depend on profiles.
The testing system is down, but I think it should work. will know as soon as something changes.
Comment #15
adrian CreditAttribution: adrian commentedbetter title
Comment #16
adrian CreditAttribution: adrian commentedI really think that this patch will make this even simpler #545452: Store install profile in the generated settings.php file
so we can remove all the weird install_state stuff
Comment #17
adrian CreditAttribution: adrian commentedpp[s
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedWow. Once again, this simplifies life for the profile author. Authoring a profile is looking a *lot* like module authoring which is just great. I did a code review and all looks swell. Lets see what test bot says.
Comment #20
adrian CreditAttribution: adrian commentedHere's an updated patch that passes more of the tests (my vm keeps on crashing during testing, so i haven't finished testing, but no issues so far).
I just updated to head and modified the test case slightly so the profile gets installed last.
Comment #22
adrian CreditAttribution: adrian commentedFixed the module API test, i needed to include the profile as one of the modules that would be loaded.
Comment #23
eaton CreditAttribution: eaton commentedI just took this for a spin, and I can't say enough good about it. Getting install profiles into the mix,. giving them access to the full API, and allowing them to participate after the initial events on install.php allows much, MUCH richer configuration and ongoing maintenance of highly customized Drupal based web-apps.
It means more people will be able to write profiles, because they understand how to use and maintain modules. It means profiles will be better supported, because their customizations can be maintained via update hooks.
This patch has the highest 'awesome-per-line' quotient I've seen in a long, long, LONG time.
It's a very small amount of actual code (mostly just moving existing functions to different files), it's passing tests, and a reading of the code itself makes perfect sense.
RTBC, with great anticipation.
Comment #24
webchickYou know what else it has is the highest "moving WTF to non-WTF" quotient as well.
The only real downside (and it's not even a downside really, because modules work the same way) is that many profiles will just be a gigantic .install file and a totally blank .profile file, just to get the profile to register. But whatever, we can deal with that (or not) in another issue IMO.
Also, it appears this patch is missing an upgrade path for people who've already got Drupal installed. I'm not really sure how to handle this.
Sorry, what? Could we document this line a little better to explain why we're adding a profile to the list of present modules?
drupal_required_modules()
(about the earlier note) Yeah! Like this! :)
Why weight = 1000? Could we get a snippet of documentation here?
Just curious.
One of the traditional complaints about install profiles is that you can only ever install one of them. By only allowing a single value into this variable and not an array, we're continuing on with this tradition. Is this seen as acceptable, or are you planning on fixing it in another patch, or..?
Something else that occurs to me is we're going to open ourselves up here to some wonderful naming collisions between install profiles and modules, but it won't be clear anywhere to someone what profile they're using without hacking the DB. We hide required modules from the modules screen.
Could we please expose this data so it's visible on the status page at admin/reports/status?
(about earlier note) Yes, like that. ;)
"Implement hook_install()."
This file is also missing a line for // $Id$ and a /** @file */ definition. Please fix.
Same deal as the default.install hunk. (though the $Id$ tag is here already)
This review is powered by Dreditor.
Comment #25
adrian CreditAttribution: adrian commentedI have added an update path. The install profiles get registered as schema 0 from this point on. so any update_x hooks will run by update.php.
Which means we are free to evolve default.install too.
It's not really possible to switch installation profiles on an already running site without completely breaking the site.
I am however working on #555212: Install profile inheritance
That patch will build on this patch and turn the value into an array, but that's beyond the scope of this issue.
We already have that problem, but nobody wants to talk about it. Modules and themes should not share the same namespace, and the same was already true for themes and profiles.
This is only a problem for stuff not hosted on drupal.org, as we are currently enforcing that here.
I have added install profile display to the requirements, but i hide it when the default profile is used because it's superfluous (check pic)
Comment #26
adrian CreditAttribution: adrian commentedpic
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commentedBack to RTBC. Thanks adrian.
Comment #28
webchickGuys. ;) I know we're all excited about this functionality, but can you please make sure patches are actually RTBC before you RTBC them? If I can't trust RTBC on these patches, I need to wait until I have brainspace to do full reviews myself, and with everything else grabbing at my attention, that drastically reduces the chances of it happening.
That is not translatable. Other strings in that function use $t().
Additionally, the last two hunks of #24 aren't implemented.
And could I get confirmation that someone tested the upgrade path?
This review is powered by Dreditor.
Comment #29
adrian CreditAttribution: adrian commentedWeird, that last hunk was fixed in my local code.
Fixed now.
Comment #30
adrian CreditAttribution: adrian commentedThe upgrade path can be tested the following way :
1) install drupal without the patch
2) apply the patch
3) go to update.php
4) check the system table to ensure that the default profile is loaded as status = 1 , schema_version = 0
You can test upgrades by adding the following to default.install either before or after you go to update.php
update.php will now let you run the update on the install profile.
Comment #32
adrian CreditAttribution: adrian commentedTypo in last patch
and rebuilt against latest head
Comment #33
webchickAwesome! :)
I went through Adrian's testing instructions myself and things seem to be working properly. The one annoyance I have lingering is that the row in the system table that points to the profile still says 'type' = 'module'. For the purposes of this patch that was nice, because it kept the changes to the minimum and made it easy to review. But in a follow-up patch it'd be nice to resolve this discrepancy, and/or just rename .profile to .module and be done with it (though the .profile extension is kind of nice for knowing at a glance what you're dealing with, and Adrian pointed out a couple of ways that profiles are actually different than modules [tasks, wizard, etc.]).
Committed to HEAD. On to the next improvement!
Comment #34
yched CreditAttribution: yched commentedCould #554620: precedence order of module paths in drupal_system_listing() be a consequence of that patch ?