Updated: Comment #96

Problem/Motivation

I
With Drupal 7's vastly-improved install profile functionality, and the recently-improved drupal.org distribution packaging infrastructure, many new install profiles have been created, and they are awesome.

However, install profiles are not inheritable, so if you want to, say, take Drupal Commons, and base your site off of it, but add one or two extra modules in your own customized install profile, you have to clone the entire install profile and change it to your needs. Allowing install profiles to declare base profiles (just like themes can declare base themes) would allow more code reuse, and make install profiles even more useful.

Proposed resolution

Modify the module load system to take into account base install profiles.

Remaining tasks

  • Create plan

User interface changes

None.

API changes

Install profiles can define base profiles using the syntax base: BASE_PROFILE inside their .info.yml files.

Files: 
CommentFileSizeAuthor
#276 1356276-276-8.2.x.patch44.44 KBbalsama
#276 interdiff-273-276.txt5.81 KBbalsama
#276 1356276-276.patch44.78 KBbalsama
#274 1356276-274-8.2.x.patch44.28 KBphenaproxima
#273 interdiff-1356276-271-273.txt3.16 KBphenaproxima
#273 1356276-273.patch44.62 KBphenaproxima
#271 interdiff-1356276-269-271.txt1.23 KBphenaproxima
#271 1356276-271.patch44.06 KBphenaproxima
#269 interdiff-1356276-267-269.txt6.35 KBphenaproxima
#269 1356276-269.patch44.28 KBphenaproxima
#267 1356276-267.patch42.4 KBphenaproxima
#264 interdiff-1356276-264.patch969 bytesoriol_e9g
#263 interdiff-1356276-262-263.txt703 bytesbalsama
#263 1356276-263.patch42.33 KBbalsama
#262 interdiff-1356276-249-262.txt1.21 KBbalsama
#262 1356276-262.patch41.95 KBbalsama
#249 interdiff-239-249.txt1.69 KBmpotter
#249 1356276-249.patch40.6 KBmpotter
#239 interdiff-231-239.txt1.47 KBmpotter
#239 1356276-239.patch40.42 KBmpotter
#231 interdiff.txt5.43 KBdawehner
#231 1356276-231.patch40.29 KBdawehner
#230 interdiff.txt10.21 KBdawehner
#230 1356276-230.patch38.74 KBdawehner
#227 interdiff-1356276-222-227.txt8.55 KBmpotter
#227 1356276-227.patch36 KBmpotter
#222 interdiff.txt1.08 KBdawehner
#222 1356276-222.patch38.77 KBdawehner
#220 interdiff.txt17.4 KBdawehner
#220 1356276-220.patch38.75 KBdawehner
#217 interdiff-1356276-214-217.txt8.67 KBmpotter
#217 make_inherited_install-1356276-217.patch36.31 KBmpotter
#214 interdiff-1356276-208-214.txt9 KBmpotter
#214 make_inherited_install-1356276-214.patch31.34 KBmpotter
#209 interdiff-1356276-200-208.txt22.59 KBmpotter
#209 make_inherited_install-1356276-208.patch27.48 KBmpotter
#207 interdiff-1356276-200-207.txt24.08 KBmpotter
#207 make_inherited_install-1356276-207.patch29.05 KBmpotter
#200 interdiff-1356276-198-200.txt5.21 KBmpotter
#200 make_inherited_install-1356276-200.patch20.61 KBmpotter
#199 interdiff-1356276-196-198.txt2.98 KBmpotter
#199 make_inherited_install-1356276-198.patch21.33 KBmpotter
#196 make_inherited_install-1356276-196.patch21 KBmpotter
#193 interdiff-1356276-183-193.txt4.96 KBmpotter
#193 make_inherited_install-1356276-193.patch20.97 KBmpotter
#183 interdiff-1356276-181-183.txt13.1 KBmpotter
#183 make_inherited_install-1356276-183.patch21.25 KBmpotter
#181 interdiff-1356276-176-181.txt8.56 KBmpotter
#181 make_inherited_install-1356276-181.patch18.69 KBmpotter
#176 interdiff-1356276-170-172.txt6.82 KBmpotter
#176 make_inherited_install-1356276-176.patch19.49 KBmpotter
#172 make_inherited_install-1356276-172.patch13.13 KBmpotter
#170 make_inherited_install-1356276-170.patch11.04 KBmpotter
#158 make_inherited_install-1356276-158.patch10.78 KBdas-peter
#157 make_inherited_install-1356276-157.patch3.15 KBmqanneh
#143 1356276-143-test-only.patch2.62 KBmpotter
#133 make_inherited_install-1356276-133.patch2.64 KBmpotter
#131 make_inherited_install-1356276-131.patch2.59 KBmpotter
#129 make_inherited_install-1356276-129.patch1.97 KBmpotter
#120 frodobaggins1.png234.86 KBphenaproxima
#112 inheritable-1356276-112.patch16.43 KBtimodwhit
FAILED: [[SimpleTest]]: [MySQL] 59,593 pass(es), 1 fail(s), and 1,387 exception(s). View

Comments

e2thex’s picture

FileSize
1.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-make.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
1.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

This patch adds a recursive function that goes the profile and any of it parents and adds paths for each of them.

e2thex’s picture

FileSize
2.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-make-D7.patch. Unable to apply patch. See the log in the details link for more information. View
2.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Change the patch so that it use base instead of parents so that it is inline with

base = BASE PROFILE

or

base[] = BASE PROFILE
e2thex’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1356276.patch, failed testing.

febbraro’s picture

Title: Modules and themes From One profile being avaiable in a sub Profile. » Make install profiles inheritable

Changing to a more accurate title

DuaelFr’s picture

Looks great ! I will give it a try soon.

patcon’s picture

DuaelFr’s picture

@patcon
Profiler's base profile allow to inherit some base profile's things like installation functions but it do not add all the base profile directories to the stack when looking for modules' or themes' locations.

patcon’s picture

I can't say with 100% accuracy until I test, but I was under the impression that Profiler was designed for that same reason. Inherited *.install files were secondary, I believe. Are you certain on that?
http://drupalcode.org/project/profiler.git/blob/refs/heads/7.x-2.x:/READ...

patcon’s picture

Hm. Ok, I think this clarified for me (although I guess I can still spin up a site myself):
http://drupal.org/node/1275160

You're right. Sorry for the confusion and derailing :)

patcon’s picture

FileSize
2.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-2-recurse-parents-pressflow-7.12-git.patch. Unable to apply patch. See the log in the details link for more information. View

Same patch as #2, but rerolled against pressflow 7.12 using git (hopefully drush_make supports this now, or i'll be back :) )

Crell’s picture

cweagans’s picture

patcon’s picture

@cweagan I'm ashamed to admit how many clicks it took me to realize I was in an infinite loop of duplicate issue links...

EDIT: Guessing this was the one :)
#555212: Install profile inheritance

cweagans’s picture

Heh, whoops. Link fixed.

patcon’s picture

GOOOO TEAM! *anchorman leap*

tirdadc’s picture

FileSize
2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 39,170 pass(es). View

I rerolled http://drupal.org/files/1356276-make-D7.patch against 7.14, had to make some minor adjustments. Will test and update this comment.

mpotter’s picture

Status: Needs work » Reviewed & tested by the community

Been using this on several large production sites and works great!

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs backport to D7

This needs to go into Drupal 8 first, but it looks like the kind of change that might be acceptable for Drupal 7 backport, if it's the direction we decide to go in.

Some issues on a quick readthrough:

+ * recusive function to find listing paths from profiles and their parents

This has typos ("recusive" => "recursive") and grammar mistakes (sentence should start with a capital letter and end with a period). Also similar issues elsewhere in the documentation.

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

It seems pretty bad that we're going to manually parse the .info file every time this function runs, rather than taking advantage of information cached elsewhere. But it's a low-level function, so I'm not sure how best to solve that.

+    $searchdir = drupal_system_listing_profile($profile, $directory, $searchdir);
   }
-
+  

Extra whitespace.

mpotter’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-make-D7-20.patch. Unable to apply patch. See the log in the details link for more information. View

Here is a re-roll of the patch with the spelling, grammar, and whitespace issues fixed. The drupal_parse_info_file() isnt' really a problem because that function already has it's own cache. But I'd welcome other suggestions or patch improvements.

mpotter’s picture

FileSize
1.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-make-D7-21.patch. Unable to apply patch. See the log in the details link for more information. View

Bah, missed one more space at the end of a line. Here is the cleaner patch.

Status: Needs review » Needs work

The last submitted patch, 1356276-make-D7-21.patch, failed testing.

mpotter’s picture

Grr. Marking this as a D8 issue is causing the D7 patch to fail testing. Is there a way to mark a patch as "for D7" for the tester so this doesn't happen? Is there any special patch naming trick for that?

If you are using D7, ignore the failed test result of #21. It applies just fine to 7.14.

DuaelFr’s picture

I had seen a naming convention before but the documentation does no longer mention it.

You can just add a "-do-not-test" suffix to say to the bot to skip your patch.

cweagans’s picture

Status: Needs work » Needs review
FileSize
2 KB
PASSED: [[SimpleTest]]: [MySQL] 36,884 pass(es). View

Rerolled patch for D8.

sun’s picture

Status: Needs review » Needs work

This looks incorrect to me.

We do not want to re-evaluate the base profile at run-time.

Instead, the only change to the foreach in drupal_system_listing() would be the replacement of $profile with something along the lines of this:

$profile = drupal_get_profile();
$profiles = array();
do ($profile) {
  $profiles[] = $profile;
  $profile_info = system_get_info('profile', $profile);
  $profile = isset($profile_info['base'] ? $profile_info['base'] : FALSE);
} while ($profile);
// Search from top/parent profile first.
$profiles = array_reverse($profiles);

Ideally, of course, we'd store the already prepared base profile information with each profile's info already, so we can avoid the repetitive system_get_info() calls for every profile in the inheritance.

I'm also missing some other essential changes to the Drupal installer and actual profile installation in this patch. Merely taking the base profile information into account for path lookups does not fulfill the promise of making install profiles inheritable.

cweagans’s picture

@sun, it enables this kind of setup: http://www.agileapproach.com/blog-entry/inheriting-your-drupal-profile-e...

IMO, what is here so far could go into Drupal 7. The installer changes and profile installation seem more like D8 things.

sun’s picture

Yes, that post is how I learned about this issue. New features go into HEAD first though. I will admit that the existing patch provides some degree of profile inheritance, but it totally was not what I expected. It sorta tries to tack the new feature onto a system that's otherwise unaware of the feature, which obviously won't fly.

If we want this to land, then we need to make the system properly aware of possible install profile inheritance. I support this change, since it was mentioned in a couple of other issues and personal discussions already. I do not see a technical reason for why it couldn't be supported.

That said, while this feature addition in core seems feasible and doable, there will still have to be support for it coded into drupal.org's distro/profile packaging scripts, which inherently/actually means to support it in Drush. But that's a separate follow-up issue.

helior’s picture

The previous patch wasn't exactly what I was expecting, either. Also, pushing fake inheritance in the parts of Drupal's non-OOP code is a foreign pattern that exists nowhere else in Drupal – and feels a bit hacky imo. Certainly if install profiles were re-written as classes then that would totally make sense, as well as be trivial to implement.

Instead, this patch contains a different approach that follows the pattern of Drupal's theme system, in which there is a concept of defining a single "base". So everywhere in the system where an install profile is requested to participate – instead of invoking the single current profile, it invokes the ancestral chain of profiles starting from the top-parent down to the last sub-profile.

This allows developers to truly extend Drupal distributions with the same ease as creating a sub-theme.

This patch is for D7 – Sorry, I know new features need to go in D8 first, but I just wanted to put this out there. I'll try getting in contact with the right developers to see what direction install profiles are going in D8 (#1530756: [meta] Use a proper kernel for the installer suggests it will be a complete rewrite, so I'm not sure how the back port to D7 will look like.)

sun’s picture

Thanks! That looks a lot more like it :)

Btw, some of the changes clash/conflict with #562042: Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles, which I'd like to see getting in first. But since that is RTBC already and we don't have a patch for D8 here yet, that doesn't seem to be an issue in the first place. ;)

dixon_’s picture

Status: Needs work » Needs review

The bot should take a ride with the latest patch.

ezeedub’s picture

Slight addition to #29 to check for drupal_get_profiles(), which is defined in install.inc, and so not normally included.

sun’s picture

Status: Needs review » Needs work

Sorry, we really need a patch for D8 here. The patches for D7 are very confusing, so I'd appreciate if we could stop posting them.

#25 is obsolete by now, drupal_system_listing() does not have to be touched at all, since D8 scans for profiles everywhere already.

This also needs to be taken into account when forward-porting #32 to D8.

ezeedub’s picture

My bad. To be honest, I kind of knew that, but since local patches don't work in drush make yet, I posted here. That's an abuse of this issue queue though, so I will go stand in the corner for 20 minutes...

geoffreyr’s picture

FileSize
2.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-make-D7-17-redux.patch. Unable to apply patch. See the log in the details link for more information. View

Just a small tweak to #17: it looks like the $bases array generated in drupal_system_listing_profile was never being used. Trying to use it as is caused the base module to not be used at all, and displayed the following error:
Warning: Invalid argument supplied for foreach() in drupal_system_listing_profile() (line 5220 of includes/common.inc).

geoffreyr’s picture

FileSize
1.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-make-D7-21-redux.patch. Unable to apply patch. See the log in the details link for more information. View

Same again for #21.

helior’s picture

Status: Needs work » Needs review
FileSize
19.02 KB
PASSED: [[SimpleTest]]: [MySQL] 46,240 pass(es). View

Finally, a Drupal 8 patch :)

I moved drupal_get_profiles() to common.inc since it makes more sense to keep it there (as @ezeedub pointed out in #32 that install.inc isn't always included). Also incorporated all the D8 changes (that I know of) including the update mentioned in #30 and #33.

helior’s picture

Also, for those using the D7 patch, here is an equivalent update.

lapith’s picture

Here is a reroll of #38 to include a fix for base profile dependencies.

lucascaro’s picture

FileSize
983 bytes

FYI an interdiff for the previous 2 patches:

pcho’s picture

#36: 1356276-make-D7-21-redux.patch queued for re-testing.

tom friedhof’s picture

FileSize
21.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-base_profiles_variable-42.patch. Unable to apply patch. See the log in the details link for more information. View

I took the patch from #39 and pulled all the logic out of runtime. It was a pretty big performance hit finding sub-profiles on every request. This patch is for D7. Is there another issue open for 7.x core to post patches to, so we don't pollute this space with 7.x issues?

Status: Needs review » Needs work

The last submitted patch, 1356276-base_profiles_variable-42.patch, failed testing.

olofjohansson’s picture

Without looking through the code, is there a reason why there is a variable called 'install_profiles' that everything depends on? The old 'install_profile' variable does still exist.

The main reason I'm asking, is because we couldn't update an existing site with this patch, since that variable is created during the installation. We solved it by simply adding the variable manually, but it would be nice if it could be solved automatically.

Aside from this, the patch at #42 has worked on several sites we've been testing.

dagomar’s picture

I haven't tried the patch yet, but I have a question, will the child profile inherit update hooks in the parent profile? Thanks!

dagomar’s picture

FileSize
21.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-cleanup-patch42-45.patch. Unable to apply patch. See the log in the details link for more information. View

I have tested and can confirm this works beautifully. I had 2 issues with existing sites, the variable "install_profiles' was missing, as previously mentioned. I just installed a blanc site to get the correct variable and imported that entry manually. The other issue I had was that the parent wasn't enabled in the system table, after I enabled it I was good to go.

I have recreated the patch because of whitespace issues. Note, it's for D7.

helior’s picture

@dagomar Yes, parent profile's update hooks are picked up as well.

geerlingguy’s picture

+++ b/includes/common.incundefined
@@ -232,6 +232,22 @@ function drupal_get_profile() {
+ * Returns a list of related install profiles in decending order of their

s/decending/descending

Could we maybe file a helper issue for D7 so those patches don't distract from getting this into D8 (if it isn't already?)? I'm a bit late to the game here, and haven't read through the entire issue+comments.

frob’s picture

I don't see why this would be added to Drupal 7 at all. Lets keep this in D8.

geerlingguy’s picture

@frob - the issue has the 'needs backport to D7' tag... which is used for issues that are possibly going to be backported/included in D7.

frob’s picture

Issue tags: -needs backport to D7

I am removing that tag. I might be out of line and I wont be offended if it gets added right back in. But to me this isn't fixing a bug or glaring UX, DX problem that needs a backport. It is a nice to have in Drupal 7, so to me it doesn't need a backport.

mparker17’s picture

Issue tags: +needs backport to D7

I would like inheritable installation profiles in Drupal 7.

Here is my attempt to explain why...

Background

Install profiles are the core of Drupal Distributions — a really quick way to set up a complex sites.

Creating a Drupal Distribution is simple: you create an installation profile; add a set of Drush makefiles to download specific versions of core and contributed modules, themes and libraries; and add your own features, custom modules and themes. If you have any patches to core or contrib, stuff to add to .htaccess or settings.php, or you write any behavioural or functional tests of the system as a whole, you can put them in sub-folders. Any changes to the database that don't belong to a particular module or theme can go in the install profile's .install file. You can build the whole site's filesystem with a single Drush make command. And, because this is an installation profile, you can easily build a fresh database for use behavioural and functional tests or to deploy on a new machine.

The result is a folder which only contains the code you've customized or written from scratch, but not cluttered with environment-specific stuff or content. This folder is ideal for checking in to version-control. And, any older version of the site can be upgraded to a new version by running update.php and reverting all features.

When do I use Distributions?

Currently, we build a Drupal distribution for any client site we work on. It lets any number of developers work on it, developers seldom need to touch staging or live directly (Jenkins does almost everything), and we don't have to constantly share copies of the database.

Why inheritable install profiles would make this better.

Inheritable install profiles would allow us to build a base install profile common to all sites we work on, containing modules, themes, etc. that we use on every project. Also, it would make it easier for us to build sites on other popular distributions. And, if a client wanted a new site similar to their old one, we could inherit from the old one.

Why I want this patch backported to D7.

We're using distributions as the base for our sites right now on D7. Since D7 will be maintained until D9 comes out, at the very least, we will be using or maintaining this methodology for a while yet.

mparker17, you seem pretty passionate about this; why aren't you helping with this issue more?

Sorry :S Somebody already wrote a patch for D7 before I knew this issue existed!

I did actually try on my own time to see if I could write a patch for D8, but I found that I didn't understand enough about D8 at the time to be able to make any progress. I know a lot more about D8 now: if I had the time, maybe I could take another crack at it. But, I don't think I'll have the time before the feature freeze.

Remove the tag or not?

@frob makes a pretty good point — this is a "nice to have" not a "need".

Is there a "would be nice to have a backport to D7" tag instead of a "needs backport to D7" tag?

cweagans’s picture

Let's not argue about the tag. Open Atrium needs it, it is backportable (since it does not change any existing behavior of install profiles unless you mark your install profile as inheriting from another), and people seem to want it. Regardless, it needs to get into D8 before anything else happens with it, so let's focus on that.

juan_g’s picture

mparker17 wrote:
> Also, it would make it easier for us to build sites on other popular distributions.

And for example well-known distributions such as Drupal Commons or Open Atrium have been only recently ported from D6 to D7 (OA for D7 still in alpha). D8 is far away for them. A backport to D7 would be indeed good for Drupal distributions in general. Of course after it's first into D8.

frob’s picture

It's not that I don't want it backported; it is that the conversation about backporting never took place. The purpose of backporting is to make sure bugs that get fixed for D8 that that are also in D7 get fixed in both code bases. Personally I want this in D7 too.

So then is the backport patch done? Someone review it please.

cweagans’s picture

We need a patch for 8.x first, and that has to be done in the next couple of days if it's going to be committed before feature freeze.

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
18.67 KB
FAILED: [[SimpleTest]]: [MySQL] 57,990 pass(es), 0 fail(s), and 47,508 exception(s). View

Rerolled patch from #37 for D8 (some files have moved, also fixed a couple of the typos mentioned earlier in the thread). Let's see how the testbot likes it.

Status: Needs review » Needs work

The last submitted patch, 1356276-base-profile-57.patch, failed testing.

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
18.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-base-profile-59.patch. Unable to apply patch. See the log in the details link for more information. View

Whoops! Put part of the patch in the wrong section of system.install. Attached patch should fix those errors.

Pancho’s picture

Priority: Normal » Critical
Issue tags: +Needs tests

Raising to major per #53.
Not sure though this would be subject to the API freeze, as it is an API addition that shouldn't affect handling of standalone profiles.

In any case this needs extensive test coverage, especially because it is not used by core.
Inheriting requirements basically seems to work but I don't believe we're already taking care of everything.
How about precedence of required modules if multiple instances are included, possibly the one patched by a distro or in a different version? There might be tricky cases to take care of.
How about inheriting config or hook_install or whatever else could possibly be included in a profile? We might omit that but need to make an informed decision.
I believe we can't do this right without a sophisticated inherited profile test case. Another option would be making standard profile inherit from minimal profile, even though that would be a very basic implementation.
Either case this would be an opportunity to vastly expand and improve our profile test cases.

Some definitive input on whether this issue is subject to the July 1st API freeze or not, would be very valuable! Depending on this we need to put much more efforts into this one - or can take our time in order to do it 100% right.

Pancho’s picture

Priority: Critical » Major
cweagans’s picture

Priority: Major » Normal

No. This is not major. It has a pressing deadline, but that doesn't make it any more important than the other features that should go into D8 either. This needs to be done before 7/1 if it's going to happen, though there's a possibility that it could go in after (since we're already considering it backportable to 7.x). Safest bet is to plan on having it done by 7/1, and if it turns out that it can be improved after that date, then great.

Pancho’s picture

It has a pressing deadline, but that doesn't make it any more important than the other features that should go into D8 either.

A pressing deadline certainly wasn't my argumentation, especially as I don't expect the deadline to be pressing for this issue.
Rather I refered to your own comment #53, also recognizing that "Major priority is also used for tasks or features which consensus has agreed are important" (Priority levels of issues)

Safest bet is to plan on having it done by 7/1, and if it turns out that it can be improved after that date, then great.

IMHO it's too late for playing safe. This issue still needs a considerable amount of manpower, which not many are able/ready to invest right now that other important tasks really are endangered by July 1st API freeze.

This needs to be done before 7/1 if it's going to happen, though there's a possibility that it could go in after (since we're already considering it backportable to 7.x).

That's a contradictio in adiecto, but with the second half-sentence I do agree. Some authoritative input by a core maintainer would be helpful.

dagomar’s picture

Hi guys,

how can I help? I'm planning to test the patch with a simple custom install profile that extends minimal. Then I could do some tests with upgrades and stuff.

Note, I have been using the patch for D7 and the only thing that does not work is inheriting libraries, which is an issue with the contrib libraries module (my workaround is to use a libraries folder in the root drupal folder).

That being said, I'll devote some hours today to test the D8 patch. If someone could tell me what I can do to do n accurate test, that would be helpful! Let's get this in (and backported ;))

frob’s picture

dagomar, I think that the best thing to work on is to make sure unit tests are in place. It will be easier on everyone if they get in now.

dagomar’s picture

Thanks for the reply frob. I have 0 experience with unit testing, just barely know what it is tbh. I'll read up on it and see if I can do anything. In the meantime I'll test the patch manually to see if anything obvious comes up...

helior’s picture

I'm glad to see this issue is gaining interest again!
@dagomar This is a corresponding patch I've been using with Libraries API so it can support libraries found in your base profiles. #1811486: Libraries API cannot find libraries in "base" profiles.

Dustin@PI’s picture

I'm guessing that groups.Drupal.org will want this so that it can inherit commons as part of the D7 upgrade. This patch is a pretty big deal from a functionality perspective so I would love to see it go in.

What's the best way to help with testing?

geerlingguy’s picture

The best way to help, it seems, would be to write tests for this functionality... it may be a bit complicated, since testing the installer/profiles is already more complex than testing systems that only run in a fully-bootstrapped Drupal environment :-/

frob’s picture

I would consider making standard a sub-theme of minimal. Then verifying that tests for both themes are being executed.

amcgowanca’s picture

Version: 8.x-dev » 7.x-dev
FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,431 pass(es). View

Re-rolled with minor updates the D7 patches.

amcgowanca’s picture

FileSize
1.98 KB
PASSED: [[SimpleTest]]: [MySQL] 40,270 pass(es). View
geerlingguy’s picture

Version: 7.x-dev » 8.x-dev

Please leave the status at D8, since the patch has to go in there first.

amcgowanca’s picture

FileSize
12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-D7-inheritable-profiles.patch. Unable to apply patch. See the log in the details link for more information. View

Hey,

What patch is everyone using above for D7? Reason I ask is I have had countless issues with many if not all of them. I have included another patch completely fresh.

Aaron

Status: Needs review » Needs work

The last submitted patch, 1356276-D7-inheritable-profiles.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
geerlingguy’s picture

#59: 1356276-base-profile-59.patch queued for re-testing.

cweagans’s picture

Right now, we need to focus on getting this into D8. Once that's done, it can go into D7. If you want to move this forward, focus on D8 first.

geerlingguy’s picture

#59: 1356276-base-profile-59.patch queued for re-testing.

geerlingguy’s picture

#59: 1356276-base-profile-59.patch queued for re-testing.

geerlingguy’s picture

Assigned: Unassigned » geerlingguy

I'm going to try to update the issue summary and possibly get some tests written for the patch in #59 for Drupal 8.

To those wanting this for Drupal 7: Please, please, please consider opening a separate issue to track the D7 patch(es) alongside this issue, instead of continuing to add them to this issue directly for now—it's already a slim-to-none chance of this getting into D8 (though I think it would be awesome!), but the signal-to-noise in this issue is getting pretty bad.

frob’s picture

I created a D7 issue here: https://drupal.org/node/2067229

geerlingguy’s picture

@frob - thanks! I've updated that issue's summary, and just a note—when linking to other issues, you can use syntax like [#NUMBER], where NUMBER is the issue number, or [#NUMBER-COMMENT] to point to a specific comment in the issue. For example:

geerlingguy’s picture

Issue summary: View changes

Updated issue summary. added link to D7 issue.

geerlingguy’s picture

Issue summary: View changes

Updated the issue summary with the issue summary template.

geerlingguy’s picture

FileSize
18.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,339 pass(es), 1 fail(s), and 32,128 exception(s). View

Re-rolled the patch from #59. No tests added yet, but some things have moved as a result of #mwds's WSCCI sprint.

Status: Needs review » Needs work

The last submitted patch, 1356276-base-profile-84.patch, failed testing.

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,224 pass(es). View

Missed a couple lines in system.install again. Testbot, go!

geerlingguy’s picture

FileSize
18.45 KB
PASSED: [[SimpleTest]]: [MySQL] 57,828 pass(es). View

Zero-byte file in last comment. This one may work.

Status: Needs review » Needs work

The last submitted patch, 1356276-base-profile-87.patch, failed testing.

mparker17’s picture

Wait what? It shows up as having passed now. Perhaps Testbot was feeling a bit under-the-weather?

geerlingguy’s picture

Status: Needs work » Needs review

That testbot had a hiccup, and so the test was manually re-run. It passed, so back to needs review!

geerlingguy’s picture

Issue summary: View changes

Changed 'parent' to 'base' to reflect latest patches.

geerlingguy’s picture

Issue summary: View changes

Updated remaining tasks.

geerlingguy’s picture

FileSize
0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Attached patch:

  • Parses .info.yml files instead of .info files (which no longer exist in D8).
  • Makes the standard profile depend on the minimal profile, and remove bits of minimal that don't belong in standard.

When installing standard profile manually (via UI) with latest HEAD, I'm getting the following error (which can be ignored, and the rest of the install still completes): The plugin (search_form_block) did not specify an instance class.. I'm guessing this is a regression in core—see #2067881: Search is missing from block admin UI after installation.

I might be able to introduce a test or two to make sure that standard inherits from minimal properly—or maybe I'll use the test profile to do a little extra inheritance testing. I'm guessing this patch will fail due to the above issue.

geerlingguy’s picture

FileSize
21.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1356276-base-profile-91_0.patch. Unable to apply patch. See the log in the details link for more information. View

Attaching an actual patch this time...

Status: Needs review » Needs work

The last submitted patch, 1356276-base-profile-91.patch, failed testing.

frob’s picture

We also need to make sure that the tests from all profiles get fired

geerlingguy’s picture

So... something is funky with the search form block—if I simply remove core/profiles/standard/config/block.block.bartik.search.yml and try the install again, the install completes successfully, with everything enabled and configured correctly. With that file, I get the error The plugin (search_form_block) did not specify an instance class..

I'm looking into why this might be happening (maybe a different bug?).

[Update: I can't figure out what's causing the search_form_block to make the installation fail. Other than that, this patch is working well...]

geerlingguy’s picture

Issue summary: View changes

Updated old .info file syntax to YAML.

geerlingguy’s picture

I updated the issue summary's remaining tasks; primarily, someone needs to figure out why enabling the search form block breaks the standard install profile.

Grayside’s picture

Worth noting that in the original patches and the Agile Approach blog, it was considered a feature that a sub-profile did not actually depend on the parent. In that way you could pick and choose which aspects of the parent you actually need to use.

I am agnostic about this--but if anyone is playing with patches since #36, dependency is presumed.

amcgowanca’s picture

@Grayside: Can you share the link to original patches and Agile Approach blog? Would love to have a good read about that approach.

Grayside’s picture

geerlingguy’s picture

Assigned: geerlingguy » Unassigned
Issue tags: -Needs tests

...since I don't have time to keep pushing this right now.

geerlingguy’s picture

Issue tags: +Needs tests

Grr... Cross post?

amcgowanca’s picture

@Grayside: Ah yes, I have read that as well. You previously mentioned that you don't believe the dependency between the base and derived should be enforced, what are the benefits of this in your opinion?

@geerlingguy: I would be happy to take over and assist in pushing this if you would like?

geerlingguy’s picture

@amcgowanca - go for it!

Grayside’s picture

@amcgowanca, I haven't actually formed a definite opinion on that. My point was really just to identify for issue visitors that there was a significant functional change in what's being done.

Happy to go into it further for the sake of clarity.

The original version of this patch limited the concept of profile inheritance to module & theme discovery. In this way you could build a codebase in which one profile could specify "get all the stuff for my parent profile and stick it in that corner, I'll add my own stuff as needed then decide what to do with it all."

The evolved version says "also, enable all that parent stuff as a default assumption, I'll turn on my additions."

The latter approach makes sense if a parent profile is treated as a black box to which you want to add more stuff. I'd say that takes more planning. The Standard profile ships with Overlay, OA ships with a Blog feature... if those are enforced on, you can't easily replace them with different solutions in your child profile without retroactively disabling those modules.

One example of how this might work is the need to separate Drupal products into minimal and standard profiles like Core. Developers can build child profiles that sit on top of the access control, plugin mechanisms, theme assumptions, and so forth of the minimal profile. Site builders can grab the complete product, make tweaks, and deploy their solution.

In a current project that forced me to go back to #36, I have a child profile that complete swaps out the media handling of the base profile. By not enabling those modules in my child profile, I am free to swap in my own media handling modules. Since those were Features modules tossing content types and fields around, it would not be pleasant to dig in and undo all that work by hand instead of skipping the module enable.

mcrittenden’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -needs backport to D7

#92: 1356276-base-profile-91.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1356276-base-profile-91.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated remaining tasks.

timodwhit’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
16.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch inheritable-1356276-107.patch. Unable to apply patch. See the log in the details link for more information. View

Re-rolling the patch to match changes over the last 3 months.

Still needs to be tested, though.

Status: Needs review » Needs work

The last submitted patch, 107: inheritable-1356276-107.patch, failed testing.

The last submitted patch, 107: inheritable-1356276-107.patch, failed testing.

timodwhit’s picture

Status: Needs work » Needs review
FileSize
16.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch inheritable-1356276-109.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 110: inheritable-1356276-109.patch, failed testing.

timodwhit’s picture

Status: Needs work » Needs review
FileSize
16.43 KB
FAILED: [[SimpleTest]]: [MySQL] 59,593 pass(es), 1 fail(s), and 1,387 exception(s). View

Re-re-re-re-...-re rolled the patch.

Status: Needs review » Needs work

The last submitted patch, 112: inheritable-1356276-112.patch, failed testing.

timodwhit’s picture

If I could get a hand with the failure of the patch that would be awesome. I basically tried to reroll the original patch, and it looks like i could have missed something somewhere a long the line.

@geerlingguy or @amcgowanca have you had any issues with the patch failing tests.

Thanks for the help and thanks for the original patch!

geerlingguy’s picture

Yes, I had trouble with the search form block (see #95). Couldn't figure it out after a few hours of debugging at the Midwest dev summit.

geerlingguy’s picture

Just an update, for those using D7 and interested in this issue; @bryanhirsch has a module (in development) that may offer an alternative solution for now. See his comment in #2067229-60: Allow install profiles to declare base profiles for Drupal 7.

DuaelFr’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

The patch in #112 no longer applies and would need more than a simple reroll. Plus, even if I'd love to see this shipped in 8.0.x, the feature freeze won't allow it so I'm postponing to 8.1.x.

frob’s picture

Status: Postponed » Needs work

postponed isn't the correct status for this issue, it still needs work even if it is being pushed to 8.1

andypost’s picture

Related issues: +#2234315: Dependency graph resolved plugins for installer tasks

only one month left for 8.1

phenaproxima’s picture

Assigned: Unassigned » phenaproxima
FileSize
234.86 KB

Frodo
I'll take this on.

alvar0hurtad0’s picture

@phenaproxima can I help you?
This is a really good improvement for people who uses profiles based workflows.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mpotter’s picture

Can somebody summarize what is needed on this? The description talks about patch #92 and a search block issue but it seems like patch #92 is really old. #117 seems to imply a large amount of work.

Can we maybe split this into two parts? (1) get the basic module searching working, (2) doing more real "inheritance" with info files, config, etc.

I've always had an issue with the inheriting of the base profile info file. I don't actually want to enable all the modules that the base distro has listed. It's a lot easier to manage those dependencies in my own child profile info.yml. In fact it's a lot easier to enable the stuff I want then to figure out how I'd possible *disable* something that is automatically inheriting from the base.

The (1) requirement is the blocker for those of us developing client profiles where we want to use something like Lightning as a base profile. I just want to tell Drupal to also look for modules in profiles/lightning.

I'm tempted to go back to just this (1) in a simple patch for D8 and then let people build on it. Otherwise I feel like it's going to be Drupal 9 before this gets addressed.

jhedstrom’s picture

+1 for splitting this work into 2 parts as suggested in #123. Perhaps we can open a child issue here to do the bare minimum for module/theme discovery in a profile designated as a 'base' profile of another and get that into 8.2?

phenaproxima’s picture

+1 to splitting it out.

DuaelFr’s picture

Agreed too.
Just a thing: given we want to allow profiles to be inheritable, we could have chains of profiles inheritance (like themes) so we need to recursively look for modules/themes in all the profile's ancestors.
My 2 cts

frob’s picture

Issue summary: View changes

Done #2692403: Make info and configuration inheritable in profiles

I have also modified this issue to refocus.

frob’s picture

Title: Make install profiles inheritable » Make inherited install profiles load base profile modules/themes in correct order
mpotter’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

OK, here is an attempt at something super simple.

The replacement for drupal_system_listing() in D8 is the ExtensionDiscovery class. As mentioned in #2503927: Convert ExtensionDiscovery to a service, this class cannot simply be converted to a service that we could override. It needs to be usable without a container and be kept very lean. Thus, we can't really load the profile info.yml file to look for a "base profile".

ExtensionDiscovery already supports something called "profileDirectories" which add additional search directories to the extension discovery process. Interestingly, the constructor even accepts a profileDirectories argument that is never used by core. Core directly instantiates new ExtensionDiscovery classes in very hardcoded ways, so it's not a simple thing to override.

There is already a function called setProfileDirectoriesFromSettings() that loads the profileDirectories with the path to the current profile. It already has some additional code to add a "parent profile" when using simpletest.

My approach is to literally implement "setProfileDirectoriesFromSettings" by taking values from $settings['profile_directories'] and adding them to the profileDirectories array. This provides a super-simple mechanism to add additional search paths via the settings.php file. The same mechanism that is used for the simpletest parent profile.

In addition to ExtensionDiscovery, DrupalKernal.php also has moduleData() which performs very similar tasks. The same simpletest changes were added here, so I also added the $settings['profile_directories'] functional to that.

Seems to work in my testing here, but feel free to point me to the proper place to add a test for this so that we get the search order correct (look in current profile first before looking in $settings['profile_directories'])

Edited Note: Profiles can use this mechanism to add support for a "base profile" the same way it currently adds the $settings['install_profile'] to the settings.php.

jhedstrom’s picture

Issue tags: +Needs tests

Could we still support the concept of base_profile in the info files, but only use it during install and actually write out the profile_directories setting in the settings.php file (via a modification of install_write_profile() perhaps)?

This will need a few tests too I think.

A few nits:

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -721,6 +721,13 @@ protected function moduleData($module) {
    +      $settings_profile_directories = Settings::get('profile_directories');
    +      if (!empty($settings_profile_directories)) {
    +        $profile_directories = array_merge($settings_profile_directories, $profile_directories);
    +      }
    

    I think this could be simplified to one or two lines using the default value parameter of Settings::get('profile_directories', []) (removes the need for the if statement).

  2. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -256,6 +263,14 @@ public function setProfileDirectoriesFromSettings() {
    +    $settings_profile_directories = Settings::get('profile_directories');
    +    if (!empty($settings_profile_directories)) {
    +      $this->profileDirectories = array_merge($settings_profile_directories, $this->profileDirectories);
    +    }
    

    Ditto here.

mpotter’s picture

Here is an updated patch to address the suggestions from #130:

  • 1 & 2 fixed.
  • Added code to install_write_profile() to update the settings.php profile_directories based on a base_profile key in the profiles.info.yml file.

For tests, I'll need some help here. Looking at the existing core/modules/system/src/Tests/Common/SystemListingTest.php, doing a real test of the added profile_directories functionality would require creating an additional test profile so we could test to see if a module similar to drupal_system_listing_compatible_test module could be found in the correct base profile directory if it didn't exist in the current profile. I'm not sure we want to create a whole new test profile just for this or if there are easier ways to test it.

Status: Needs review » Needs work

The last submitted patch, 131: make_inherited_install-1356276-131.patch, failed testing.

frob’s picture

I have not looked at these patches at all.

Just thinking about the tests, could have have the test profile set the minimal profile as a base profile.

Excluding tests, should the standard profile have the minimal profile set as its base profile?

mpotter’s picture

We'd need to be able to add a test module to the base profile, similar to how the drupal_system_listing_compatible_test module is added to the test_profile.

Edited: Oh, and we don't need to set "minimal" as the base for "standard" because this patch ONLY deals with searching for modules and neither of those profiles contain any modules. For true inheritance of stuff like config, see the bigger issue at #2692403: Make info and configuration inheritable in profiles that you created in #123.

jhedstrom’s picture

Issue tags: +Needs manual testing

Regarding tests, I was thinking we could potentially use vfsStream to mock out a profile directory structure, but the DrupalKernel fairly well hardcodes it's root path in the constructor:

    $this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));

I also spent a bit of time trying to do a unit test, but ran into other issues there (even if it was possible to get all the way at the DrupalKernel::boot() method, that wouldn't address the hard coded root). The alternative would be to use a unit test only on the protected DrupalKernel::moduleData method and use php reflection to make it callable (and also then the protected root variable could be set to use a vfsStream of our chosing).

If all that sounds a bit much, tests may need to wait on #2605654: Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testing.

Tagging for manual testing for now.

mpotter’s picture

Yeah, the more I thought about the testing last night, the more I felt it just isn't that needed in this case.

The patch in #133 is just using the existing profileDirectories functionality of ExtensionDiscovery. This functionality is already well tested in core (SystemListingTest). All we are doing in this patch is loading the profileDirectories from the $settings.

About all there is to test here is when multiple directories are specified in the $settings array to check to see that they get processed in the correct order. But that's a pretty simple manual test. I worry that we could really go overboard and make this patch super complicated with a test that isn't really needed.

Interested in what others think, but #133 just seems super straight-forward and just provides an easy way to use a pre-existing mechanism for expanding the module search path.

dawehner’s picture

Yeah, the more I thought about the testing last night, the more I felt it just isn't that needed in this case.

How about, no :) If we add a feature we should add tests, otherwise it will break at some point. Its kinda simple.

mpotter’s picture

I understand the need for tests, believe me. But all we are doing is setting profileDirectories from the settings.php. The use of profileDirectories to control the module search order is already tested (by SystemListingTest). So what are we testing here? I'm all for tests if somebody could go into more detail on what kind of test is needed and how it might be done.

dawehner’s picture

@mpotter
When I try to write tests I don't try to look at the actual tested code, but rather try to think of possible inputs and outputs. By that, later changes causes less potential required changes in the tests. For this particular case there is an additional input.

I'm happy to help out writing a test, even if its seems to be pointless. For me tests are also a way of documentation, when they are written properly

mpotter’s picture

Is the test method shown in the issue linked in #141 the right approach to take here? Seems like it would be easy.

But also seems like both issues are trying to accomplish something similar: support for a base profile. I wonder if we could somehow combine these efforts into this more general patch for adding new profileDirectories?

mpotter’s picture

mpotter’s picture

So yes, tests are good!! This test uncovered something interesting:

I tried running the test from #143 without the patch from #133 and it still passed! Learned that ExtensionDiscovery::scan() is doing this:

// Search the core directory.
$searchdirs[static::ORIGIN_CORE] = 'core';

This means that core/profiles/testing/modules is ALWAYS being searched! Regardless of the profileDirectories command. So, in fact, even the SystemListingTest is a bit wrong. It uses $listing->setProfileDirectories(array('core/profiles/testing')); but that isn't necessary since scan is always looking at all subdirectories of core.

In order to do these discovery tests properly, we would need to put a test module somewhere outside of core. Any ideas?

DuaelFr’s picture

Would it be possible that the test create a fake module in the temporary directory during its setup?

dawehner’s picture

Yeah I think when we write a unit test, we could totally setup the filesystem using vfs://

mpotter’s picture

I'll work on a test using the virtual file system to not only test this issue but also to perform a better test to ExtensionDiscover itself (improving the SystemListingTest).

jhedstrom’s picture

Issue tags: -Needs manual testing

Removing the manual testing tag since automated tests are underway.

Some feedback on the patch in #143:

  1. +++ b/core/modules/system/src/Tests/Common/ExtensionDiscoveryProfilesTest.php
    @@ -0,0 +1,65 @@
    +/**
    + * @file
    + * Contains \Drupal\system\Tests\Common\ExtensionDiscoveryProfilesTest.
    + */
    

    This can be removed since #2304909: Relax requirement for @file when using OO Class or Interface per file went in.

  2. +++ b/core/modules/system/src/Tests/Common/ExtensionDiscoveryProfilesTest.php
    @@ -0,0 +1,65 @@
    +        $this->assertTrue(file_exists(\Drupal::root() . '/' . $filename), format_string('@filename exists.', array('@filename' => $filename)));
    

    format_string is deprecated, and for both exceptions and test messages I think the standard has switched back to simple concatenation.

  3. +++ b/core/modules/system/src/Tests/Common/ExtensionDiscoveryProfilesTest.php
    @@ -0,0 +1,65 @@
    +    $settings['profile_directories'] = array (
    

    Coding standard nit array (.

zach.bimson’s picture

I've just applied the patch at #133 on both my base profile and my child profile installations, my child info file is the same as my base profile info just all the modules are stored within my base profile. I've added the directories for both in my settings file and added the base profile in my child profile info.

The base profile installs fine as expected on its own, but when the profile is places within my child profiles install /profiles directory and i try to enable the same set of modules i get un met dependencies issues...

Stepping though the install on both profiles (base on its own and child inheriting from base in its own install) i get the modules installed in a totally different order, with the base on its own getting way more modules enabled before any of my custom modules are enabled.

I was under the impression that the order in the info file was what was read but that doesnt seem like the case.

Not really sure where to start fixing this so thought id stick it up here, maybe im approaching this wrong..

brantwynn’s picture

Status: Needs review » Needs work

Not sure if the reroll goes to #2692403: Make info and configuration inheritable in profiles but the patch from #133 no longer applies to 8.2.x

Edit: Got caught up on the issue, confusion alleviated.

dawehner’s picture

Personally I believe that its fine for people to use this patch as a workaround for specific things.
On the other hand I actually believe we should tackle our base system/extension system issues like #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList first and then look into making install profiles actually inheritable. This then would allow us to not expand the brittleness, but rather give people a proper base to build upon.

DuaelFr’s picture

I totally agree. We need a more robust inheritance system if we want our distributions to rise.
Themes are a a great example of what can be done. We have a fully chained inheritance with overrides and everything we can dream of. I'd love to see something similar in profiles.

dawehner’s picture

Assigned: phenaproxima » dawehner

I'll have a look what this would actually require us to do

frob’s picture

Title: Make inherited install profiles load base profile modules/themes in correct order » Make sub install profiles load base profile modules/themes in correct order
Issue summary: View changes

These are the goals of this issue. Ideally, I would want this to work like the theme system's base theme -> sub theme relationship.

The issue has gotten off and then on the rails several times. I wonder if the word "inheritance" is loaded and throwing people off on what this is supposed to do. I have change the title to mirror the concept of the base and sub profile mirroring the base and sub theme relationship.

There is more to this than just allowing a sub profile declare a base profile, which is why this issue got split into two parts.

dawehner’s picture

Title: Make sub install profiles load base profile modules/themes in correct order » Allow profiles to provide a base/parent profile and load them in the correct order

These are the goals of this issue. Ideally, I would want this to work like the theme system's base theme -> sub theme relationship.

Can't agree more. Sadly the issue got derailed. Well, things happen, but let's discuss that in its own issue: #2743197: Load additional profile directories via settings

In order to resolve this issue I think we need a couple of those steps:

  • Allow installation profiles to define a parent/base profile
  • Ensure that installation tasks (and all the other various hooks) are fired in the installer for the base profiles as well
  • Adapt \Drupal\Core\DrupalKernel::moduleData to add the parent installation profiles as well during runtime into the list of available "modules"
  • Adapt \Drupal\Core\Extension\ExtensionDiscovery::setProfileDirectoriesFromSettings to take into account profile parents
  • In general ensure that modules from subprofiles can override moduels from their base profiles
  • Write a test case which provides a base profile and ensures that each of the previous points work properly
dawehner’s picture

Assigned: dawehner » Unassigned
das-peter’s picture

I just gave this a try.
I've created a profile which depends on the 8.x version of panopoly.
However, I hit the issue described here #2743197-8: Load additional profile directories via settings by mpotter.
It seems a bit odd that the current approach would require me to adjust the settings.php even before I can install.
So I fiddled a bit around and came up with something similar to ThemeHandler - but no ProfileHandler yet.
I kept the stuff that relies on / writes the paths in settings.php untouched.
I'm tempted to have something like ProfileHandler because core/lib/Drupal/Core/Config/ConfigInstaller.php currently needs an include to ensure the function install_profile_info() is available, which is not very nice.
The patch introduces following syntax for profile info files:

base_profile:
  name: panopoly
  excluded_dependencies:
    - panopoly_demo

The new key excluded_dependencies allows exactly what it implies - excluding certain dependencies from the parent profile.
In my tests the install hooks of the base profile are invoked and the configuration is imported (I had to suppress the error messages of duplicated configurations for all install profiles in the chain - not just the selected one.)
Patch is against 8.1.x

Saphyel’s picture

Status: Needs work » Needs review

@das-peter maybe is better do it in 8.2.x and then pick up to 8.1.x ?

Status: Needs review » Needs work

The last submitted patch, 158: make_inherited_install-1356276-158.patch, failed testing.

The last submitted patch, 133: make_inherited_install-1356276-133.patch, failed testing.

The last submitted patch, 158: make_inherited_install-1356276-158.patch, failed testing.

mpotter’s picture

mqanneh: please keep work on the simple patch that just provides the $settings profile_directory extension in issue #2743197: Load additional profile directories via settings.

This issue was split in #155 and the focus here should be on patches that solve the bigger problem of providing true profile inheritance or something similar to how themes and sub-themes are handled.

I think the patch in #158 tries to address the larger issue from the OP. However, the reason we want to split this is because the simple changes to allow profile_directories to come from $settings has a much better chance of getting tested and added to 8.2.x.

I will re-roll the patch in 2743197 so it only contains the $settings code. It will still need tests to get committed.

In this issue, let's focus on getting #158 to pass testing and to add new tests for it. It should definitely be done against 8.2.x first and then backported to 8.1.x as needed.

mpotter’s picture

OK, just cleaned up the patch in 2743197. Let's get some people testing and marking that as RTBC so we can get it into 8.2.x. Then we can remove that dup code from the patch in #158 and focus on this larger issue of supporting a base_profile.

mpotter’s picture

Tested #158 with Octane inheriting from Lightning distro and it seems to work well so far.

I'm starting to wonder if putting base_profile in the info.yml file and the whole $settings['profile_directories'] stuff is the "right way" since I think we are trying to move to a more read-only settings.php.

Perhaps the base_profile information belongs in it's own simple config yml file? When I worked on Features D8 I know I was advised not to mess with the info.yml files in modules and to instead create a modulename.features.yml file.

Or, rather than loading and parsing a yml file, perhaps we just need a new hook for hook_base_profile_info() to return this data. That would prevent the messing Include in the ConfigInstaller. But I know that's also a bit of "D7-like-thinking" and maybe it should be something different in D8?

Looking for opinions. I think this patch is on the right track but we just need to tweak how it's getting and passing around it's data for profile directories.

mpotter’s picture

Something is still not quite right with the inherited module dependency and config system.

If I create a profile (like Octane) that has a base_profile of Lightning, and then try to list "lightning_core" and "lightning_layout" in my octane.info.yml file, then when I do the site-install I get errors about Unmetdependencies:

exception 'Drupal\Core\Config\UnmetDependenciesException' with message 'Configuration objects [error]
(core.entity_form_display.node.landing_page.default, core.entity_view_display.node.landing_page.default,
core.entity_view_display.node.landing_page.full, field.field.node.landing_page.field_meta_tags, field.field.node.page.panelizer,
node.type.landing_page) provided by lightning_layout have unmet dependencies' in
/var/www/build/html/core/lib/Drupal/Core/Config/UnmetDependenciesException.php:84

Looking into the specific dependencies that are not met, it is stuff like "metatags", which is a module dependency for lightning_layout/config/install/core.entity_view_display.node.landing_page.full.yml

So when it builds the config dependency graph, I'm not sure it is taking into account the config within the base profile. I think it is trying to enable the lightning_layout module before the metatags module (alpha order) because it doesn't know there is config in lightning_layout that requires metatags. Metatags is not listed as a direct dependency in the lightning_layout.info.yml.

Will dig into this a bit more.

mpotter’s picture

Looks like the problem in #166 is with the lightning_layout module not declaring it's config dependencies in it's module info.yml file. I'll report that in the Lightning queue. After fixing that, the child Octane profile is installing properly with Lightning as a base.

Since the patch from #158 seems to be working in a real site, I'll switch to looking at why the tests are failing.

dawehner’s picture

Just some super quick feedback.

  1. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -248,8 +256,15 @@ public function setProfileDirectoriesFromSettings() {
    +
    +    // Allow additional profile directories to be added from settings.php.
    +    // This provides support for "base profiles".
    +    $this->profileDirectories = array_merge(Settings::get('profile_directories', []), $this->profileDirectories);
    +
    

    I still think this is simply a hack we should not introduce

  2. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -265,6 +280,40 @@ public function getProfileDirectories() {
       /**
    +   * Returns a list of related installation profiles.
    +   *
    +   * @param $profile
    +   *   Name of profile.
    +   *
    +   * @return array
    +   *   List of dependent installation profiles in descending order of their
    +   *   dependencies.
    +   */
    +  public function getProfileDependencies($profile) {
    

    Could we try to put that into its own service? Profile dependencies IMHO are something which doesn't belong in a discovery of extension ... the rule of thumb is basically: Extension discovery should not parse info files.

    Its tricky

  3. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -265,6 +280,40 @@ public function getProfileDirectories() {
    +    $cache = &drupal_static(__FUNCTION__, array());
    

    IMHO we should never introduce new drupal_static() examples, especially in OO code

mpotter’s picture

#2: Funny timing...I'm working on a new class for the getProfileDependencies() method as we speak! Not even sure it needs to be a real service since it has to do it's own parsing. But even if we move this out of ExtensionDiscovery directly, it will still ultimately need to parse info files since that's where the dependent profiles are listed.

But profile dependencies ARE something that needs to be considered when discovering extensions since extensions need to be discoverable in those dependent profile paths, right?

Did you have an idea of a better place to put this base_profile info if not in the info.yml file?

#3: Yes, once we put getProfileDependencies() into it's own class we won't need the drupal_static.

Any preference on the name of the new class? I was thinking of Drupal\Core\Extension\ProfileHandler

mpotter’s picture

Status: Needs work » Needs review
FileSize
11.04 KB

OK, here is a revamp of #158. Notes:

1) Removed all of the $settings['profile_directories'] stuff. No longer related to #2743197: Load additional profile directories via settings.

2) Created a ProfileHandler service. Moved all the functions related to profile inheritance into that service: getProfileDependencies, getProfileDirectories, isProfile

3) Removed the code in DrupalKernel for moduleData() function. That function is already loading all profiles as long as they are in the current module list. With this new approach, all base profiles are installed normally so they are already in the module list. Since we got rid of profile_directories in #1 we no longer need any change here.

4) Removed the base_profiles stuff in install.inc. All it needs is the base_profile info. It doesn't need to additionally store the list of profiles since it can get that from the ProfileHandler. This makes the code much cleaner and the only downside is that the profile info.yml file gets parsed twice (once in install_profile_info() and once in ProfileHandler, which is cached). Since this only happens during install I'd rather have the cleaner code.

5) Removed the complexity in ConfigInstaller needing to load the install.inc file. It just uses the new ProfileHandler

6) ExtensionDiscovery also just uses the new ProfileHandler. The logic for detecting a profile name is moved to the isProfile function in the ProfileHandler.

7) The reason the tests were failing was because during tests there isn't any install profile and the previous drupalGetBaseProfiles() wasn't handling that. This function is no longer needed and local testing looks good so we'll see what the testbot says.

8) Yes, we still need to write some tests for this. We'll need to use the vfsStream stuff again like in the related issue along with some unit tests for ProfileHandler itself.

9) Comments are welcome! There are probably still things to clean up (and maybe some new things that I didn't do correctly yet)

jhedstrom’s picture

I like this approach.

A few nitpicks, and questions:

  1. +++ b/core/includes/install.inc
    @@ -1073,6 +1080,25 @@ function install_profile_info($profile, $langcode = 'en') {
    +    if (!empty($info['base_profile']['name'])) {
    +      $base_profile = install_profile_info($info['base_profile']['name'], $langcode);
    

    Should a shortcut like

    base_profile: my_profile

    be supported? If not, for developer experience, should we throw an exception if base_theme is set but name isn't?

    Also, stylistically, should we follow base theme and remove the underscores here?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -80,6 +87,7 @@ public function __construct(ConfigFactoryInterface $config_factory, StorageInter
    +    $this->profileHandler = \Drupal::service('profile_handler');
    

    This should be injected I think.

  3. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,81 @@
    +class ProfileHandler {
    

    This should implement ProfileHandlerInterface probably?

  4. +++ b/core/lib/Drupal/Core/Extension/ProfileHandlerInterface.php
    @@ -0,0 +1,44 @@
    +   * @param $profile
    +   *   Name of profile.
    ...
    +   * @param $profile
    +   *   Name of profile.
    ...
    +   * @param $profile
    +   *   Name of profile.
    

    Missing parameter type here.

  5. +++ b/core/lib/Drupal/Core/Extension/ProfileHandlerInterface.php
    @@ -0,0 +1,44 @@
    +   * @return bool
    

    Missing return description.

mpotter’s picture

Good feedback!

And WOOT!! Tests passed on #170. Let's hope this passes too.

1) Yes, I agree is should follow the same as "base theme". Have changed it to "base profile". Also supporting the shortcut of just "base profile: name" when there aren't any excluded dependencies.

2) I knew this would be a comment! Already changed in my dev.

3) Oops, good catch! Left over from before I had an interface. Fixed.

4) Yep!

5) and Yep.

Here is a new patch.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
@@ -248,8 +249,10 @@ public function setProfileDirectoriesFromSettings() {
     if ($profile) {
-      $this->profileDirectories[] = drupal_get_path('profile', $profile);
+      $profile_handler = \Drupal::service('profile_handler');
+      $this->profileDirectories = array_merge($profile_handler->getProfileDirectories($profile), $this->profileDirectories);
     }

Could we somehow force people to set the profile directories from outside, or at least allow to set it from outside? The extension discovery ideally should be able to run without an actual working container, just for itself. This is no longer the case with this change

mpotter’s picture

dawehner: well, that is why I originally proposed using $settings['profile_directories']. The $settings were outside the container. Do you have a suggestion for how to set it from outside? Seems like it would still be a kludge involving some hooks and then passing data somehow running early in the bootstrap process.

Since extension discovery without a container doesn't make sense for profiles, can we maybe just add a conditional around this so it only runs if a container is available?

mpotter’s picture

Status: Needs review » Needs work

There are some obscure problems still with this patch. Let me summarize an IRC discussion with dawehner and some other notes:

1) extensionDiscovery needs to work in cases where there isn't any container. Meaning that services are not available. Thus, it cannot always call the new ProfileHandler service. Specifically extensionDiscovery is used by moduleData() in DrupalKernel. When called after a cache-clear it runs without a container. In this case, the base-profile is not found.

2) We need to avoid changes to extensionDiscovery. It's already complicated enough and in the process of being greatly reworked. Meta issue is #2186491: [meta] D8 Extension System: Discovery/Listing/Info. Lots of work currently being done in #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList that might effect us.

3) The only time extensionDiscovery needs to know anything about profiles is at the end when it calls filterByProfileDirectories(). Without a container, all it knows is the $settings["install_profile"]. It will filter extensions in parent profiles.

4) Issue #2156401: Write install_profile value to configuration and only to settings.php if it is writeable is almost ready to hit 8.2.x and will change how $settings is handled. The idea is to inject the profile info into the container. This patch also assumes a single profile, so we'll be patching inherited profiles over that when it gets committed. dawehner is going to try and push 2156401 forward so we can make progress. I'll be reviewing it to see what might need to change here.

5) The issue in #4 doesn't change the fact of 1,2,3 regarding extensionDiscovery not having a container, so it still won't have access to the profile(s).

6) dawehner suggests putting the profile and base-profile info into Config since there are other examples in DrupalKernel that need access to Config without having services available that might be models for extensionDiscovery. Doesn't help us directly today, but gives a direction to go as to where to store the profile stuff.

7) The patch in #172 passes normal testbot testing. However if you try to run tests via simpletest and the run-tests.sh script, they will fail with errors about not finding the parent profile. This illustrates the complexity of the issue regarding when services are available and when they aren't. It also shows that the current Core test coverage of all this is not robust enough.

THE PLAN

A) Will try to reroll #172 to fix the issue of the parent profile not being found when running run-tests.sh. This might involve putting code back into DrupalKernel to handle the situation where the service container is not available. This will give us an interim solution I can use for my current clients. I'll likely post a backport for 8.1.x as well.

B) We get 2156401 committed.

C) We reroll our patch and add the changes needed because of (B).

D) Then we wait for 2208429 and update our patch based on those changes.

E) Then we hopefully get our patch committed with clean profile inheritance! Then Profit.

Hopefully all of this in time for 8.2.0 (hey, I can dream!)

mpotter’s picture

Here is a new patch and interdiff. It fixes the issue with run-tests.sh not working. Here are some notes:

1) The problem with run-tests.sh has nothing to do with containers and moduleData. The issue is that the _system_rebuild_module_data() function specifically only adds the main profile to the list of modules. Thus causing errors when the parent profile is queried. It actually finds the *modules* within the parent profile just fine. The fix was for the InstallStorage as mentioned below in #3 and #4.

2) In the case of moduleData() calling extensionDiscovery without any container, it already calls setProfileDirectories(array()) which causes the scan() to return ALL profiles, not just the current profile. It then filters in moduleData() based on the enabled modules in the module list. In this case, the code in setProfileDirectoriesFromSettings that calls the ProfileHandler is never called (because profile_directories is already set). So no changes were needed to moduleData.

3) There are several places in Drupal that call scan('profiles') to get the profile list and then call
drupal_get_filename('profile', $profile, $profile_list[$profile]->getPathname());
to set the filename cache directly. I think this kind of behavior is going away in some of the issues mentioned above (Good! because it's horrible!). This kind of code assumes a single profile path. So several places (InstallStorage, ExtensionInstallStorage) had to be updated to call the ProfileHandler to get a list of profiles to loop through and set.

4) In add cases of using drupal_get_filename(..,..,value) to set the cache, I have just let it set the path for *all* profiles instead of just the current profile. It doesn't hurt to have all the profile path info cached. You cannot call the ProfileHandler->getProfileDependencies(profile_name) unless this filename cache is already set.

jhedstrom’s picture

+++ b/core/modules/system/system.module
@@ -963,22 +963,31 @@ function _system_rebuild_module_data() {
+  // Include the installation profile in modules that are loaded.
+  $weight = 1000;
+  foreach ($profile_names as $profile_name) {
+    $modules[$profile_name] = $profiles[$profile_name];
     // Installation profile hooks are always executed last.

Should this array be reversed in order so that the actual profile has the heaviest weight, so it can always override hooks provided by base profiles?

As I understand it now, if there were, for instance, 3 profiles (foo, foo_base_1, and foo_base_2), foo_base_2 would be the heaviest, rather than foo...

+++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
@@ -248,8 +249,15 @@ public function setProfileDirectoriesFromSettings() {
     // In case both profile directories contain the same extension, the actual
     // profile always has precedence.
     if ($profile) {
-      $this->profileDirectories[] = drupal_get_path('profile', $profile);
+      if (\Drupal::hasService('profile_handler')) {
+        $profile_handler = \Drupal::service('profile_handler');
+        $this->profileDirectories = array_merge($profile_handler->getProfileDirectories($profile), $this->profileDirectories);
+      }
+      else {
+        $this->profileDirectories[] = drupal_get_path('profile', $profile);
+      }

There should probably be a code comment here explaining why the profile_handler service may be unavailable.

Status: Needs review » Needs work

The last submitted patch, 176: make_inherited_install-1356276-176.patch, failed testing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Extension/ProfileHandlerInterface.php
@@ -0,0 +1,45 @@
+  /**
+   * Returns a list of related installation profiles.
+   *
+   * @param string $profile
+   *   Name of profile.
+   *
+   * @return array
+   *   List of dependent installation profiles in descending order of their
+   *   dependencies.
+   */
+  public function getProfileDependencies($profile);
+
+  /**
+   * Returns an array of install profile directories
+   *
+   * @param string $profile
+   *   Name of profile.
+   *
+   * @return array
+   *   List of profile paths
+   */
+  public function getProfileDirectories($profile);

IMHO this is a bad interface design, but rather a copy of the potential public methods one could have onto an interface. IMHO we should just have a getProfileDependencyExtensions which returns a list of extension objects. This allows you to both use them as names and for directories.

mpotter’s picture

dawehner: I could merge getProfileDependencies() and getProfileDirectories() into the single getProfileDirectories() (and use array_keys when you just want the profile names). But returning a full extension object is probably a bad idea since this function is called at a low level when the extension objects themselves are not available yet. For example, when getProfileDependencies() is called from InstallStorage::getAllFolders() it has already fetched the list of profiles via the ExtensionDiscovery scan() and that is not available within the ProfileHandler class. We'd need to add calls to ExtensionDiscovery::scan within the ProfileHandler itself which then prevents the caller from using the same listing object to scan for modules. I think it would make the code a lot more complex just for the sake of a "better interface".

Can we handle stuff like that in later revisions/re-rolls? This patch is obviously going to change a lot as the 2156401 and 2208429 issues get committed. So I'd rather focus on getting something that works for now and then worry about making it nicer after those other two issues drop.

jhedstrom: Regarding the weights, the current order is correct. The getProfileDependencies() returns the profiles in dependent order with the parent profiles first and the main profile last. So I believe this already assigns the highest weight to the profile you want to run hooks for last (the main profile. parent hooks run first). I'll improve the comment for getProfileDependencies() to make this order more obvious.

mpotter’s picture

Here is a reroll that incorporates the changes requested above (simplified the interface as much as I could without fully loading extensions). Also fixes the failed test from #176 where _system_rebuild_module_data() was returning all profiles when no profile was being used.

mpotter’s picture

Re #179: I just looked at what the Extension class actually entails. It's not as much as I thought, so returning Extension objects for the profiles might be easier than I thought. Looking into it.

mpotter’s picture

OK, dawehner you are brilliant! Your suggestion to return Extension objects worked really well. In fact, it allowed me to greatly simplify the system module _system_rebuild_module_data() function. I was able to move all of the profile-related stuff into the ProfileHandler, such as the weights starting at 1000, having profiles be hidden and required, etc. It also no longer parses the info file twice.

This patch is starting to accomplish what I originally hoped in encapsulating the gory details of Profiles into implementation of a clean service!

Cross fingers for tests.

Status: Needs review » Needs work

The last submitted patch, 183: make_inherited_install-1356276-183.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review

@mpotter Awesome! This really is starting to look pretty nice. Hope I can give it a test run soon :)

das-peter’s picture

Status: Needs review » Needs work

Oops, test-bot just came back with "Needs work". Fixing status.

mpotter’s picture

Yep, I think we are really close. I wasn't surprised that the last reroll had some test failures given the extensive changes made in _system_rebuild_module_data() but hopefully I'll be able to fix those.

mpotter’s picture

Hmm, so one reason tests are failing is that ProfileHandler is setting the profiles as required *before* _system_rebuild_module_data_ensure_required is executed. In the past the _system_rebuild_module_data_ensure_required call is done before the profiles were set to required.

This is dirty since _system_rebuild_module_data would still need to know something about the profile instead of ProfileHandler handling it all. But it's causing dependent modules of the profile to be added to the array.

Hmm, looking at the _system_rebuild_module_data_ensure_required() code it doesn't seem to check first if the extension is actually in the array. Extensions get added as Extension objects, but if you have a dependency that isn't yet in the list a stdClass object is created indirectly in _system_rebuild_module_data_ensure_required. I think I can fix that directly in the _system_rebuild_module_data_ensure_required function, so let me play with that.

mpotter’s picture

I'll have to look at this more tomorrow. If somebody wants to help in the meantime, the quick test to run locally via run-tests.sh is "Drupal\Tests\system\Kernel\Extension\ModuleHandlerTest". It completes in about 30 secs on my dev system, making it a quick turnaround.

In _system_rebuild_module_data() if I remove the "if (empty($module->info))..." conditional around the info_parser then the test passes. If I put this conditional back in, the test fails (also restored the other bits in _system_rebuild_module_data() originally computing Hidden and Required).

I don't understand why using the cached info_parser result is different from calling the info_parser again fresh. When I var_dump() the non-empty array and the newly parsed array there are not any significant differences in the dependencies that should be causing these errors. So there is some other bizzare side-effect going on here.

The last submitted patch, 183: make_inherited_install-1356276-183.patch, failed testing.

RajabNatshah’s picture

Hi Drupal Profiles Developers,
I have tested all list of patches.
We do need to have a full Start to End solution. This is not working right now.

- Profiles must install and start working without showing the "Base Profile".
- Profiles must start work without the need to any changes in the settings.php.

It should be something like "Profile Manager" .. and have a menu link called "Profiles"
Users must be able manage profiles as they manage Themes. Like the "Theme manager" and the appearance link.

mpotter’s picture

RajabNatshah: The patch in #183 doesn't require any changes to settings.php and is a full solution. However if you read the issues you'll see that this patch is still in "Needs work" state and is not passing tests. This issue isn't about anything like "Profile Manager". Profiles cannot be managed like Themes because you can change Themes on a running site, but you cannot change Profiles. Profiles are only for Install time. If you have requests for other ways of doing this, please create a new issue.

Back to the patch...

I think I might have found a core issue, or else I don't know exactly what is going on with the caching of the Extension system. I downloaded a fresh copy of Drupal 8.2.x and then in the System::_system_rebuild_module_data() function I changed line 996 from
$module->info = \Drupal::service('info_parser')->parse($module->getPathname());
to this

    if (empty($module->info)) {
      $module->info = \Drupal::service('info_parser')->parse($module->getPathname());
    }

Basically, I'm just trying to prevent un-needed info file parsing. If the $module->info array already exists, then it shouldn't need to re-read it again, right?

Well, after making this one change, tests break. Specifically the Drupal\Tests\system\Kernel\Extension\ModuleHandlerTest breaks:

Drupal\Tests\system\Kernel\Extension\ModuleHandlerTest 5 passes 3 fails
FATAL Drupal\Tests\system\Kernel\Extension\ModuleHandlerTest: test runner returned a non-zero error code (2).
Drupal\Tests\system\Kernel\Extension\ModuleHandlerTest 0 passes 1 fails

Looks like an issue with dependent modules, but I'm not sure. Anyone know what Drupal is doing with the $module->info data that is requiring it to be regenerated all the time?

Now that I know what was causing the tests to fail, I'll go back to the patch and see if I can come up with a way to set the profiles info array within ProfileHandler and yet still allow _system_rebuild_module_data to regenerate it. It will likely be messy though. Maybe one of the other issues for reworking _system_rebuild_module_data() will help with this eventually.

mpotter’s picture

OK, let's fire up the testbot. I found that you cannot set the "required" property for the profile before calling _system_rebuild_module_data_ensure_required(). So I removed that from ProfileHandler and moved it back into System::_system_rebuild_module_data(). Let's see if that fixes the issues.

mpotter’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 193: make_inherited_install-1356276-193.patch, failed testing.

mpotter’s picture

Nasty little test! That one failing test is because it's looking for $module->origin even though "origin" is not an official property of the Extension object. Dirty OOP here. It's a special value added by the scan() function not documented for Extension objects. Just set extension->origin = '' for now to see if that's good enough. If we really need this origin setting then we'll need to do a scan('profiles') within ProfileHandler and then also document in Extension that Origin is a required property. But in this case I think it's more of an issue with the specific test.

jhedstrom’s picture

This is looking really close!

Just a few tiny nitpicks here:

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -54,6 +55,13 @@ class ConfigInstaller implements ConfigInstallerInterface {
    +   * ProfileHandler
    

    Tiny nit, should be "Profile handler.".

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -462,7 +473,8 @@ public function checkConfigurationToInstall($type, $name) {
           // modules because those may depend on the current module being installed.
    
    +++ b/core/lib/Drupal/Core/Config/InstallStorage.php
    @@ -155,16 +155,18 @@ protected function getAllFolders() {
    +      // Now that we can fetch the path, get dependent profiles and add the extensions
    

    This line exceeds 80 characters, so needs to wrap. It also needs a '.' at the end.

  3. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,102 @@
    +        // See @1356276-192.  Issue in System::_system_rebuild_module_data,
    

    This should probably be a proper link? Alternatively, a clear explanation directly in the code comment might be preferable, unless there's an associated @todo task in the linked issue.

  4. +++ b/core/modules/system/system.module
    @@ -994,6 +991,12 @@ function _system_rebuild_module_data() {
    +    else {
    +    }
    

    The empty else can be removed here since it does nothing.

phenaproxima’s picture

This is looking real good. I'm super glad we're finally going to have a ProfileHandler service (long has this been needed!) and that the system will start treating profiles like first-class extensions. I don't see much that is functionally wrong this patch; the overwhelming majority of my comments are basically nitpicks.

  1. +++ b/core/includes/install.core.inc
    @@ -21,6 +21,7 @@
    +use Drupal\Core\Extension\ProfileHandler;
    

    It doesn't look like this class is actually used in this file...?

  2. +++ b/core/includes/install.core.inc
    @@ -439,6 +440,13 @@ function install_begin_request($class_loader, &$install_state) {
    +    $profile_handler = \Drupal::service('profile_handler');
    +    $profiles = $profile_handler->getProfiles($profile);
    

    Nit: For brevity's sake, can this be $profiles = \Drupal::service('profile_handler')->getProfiles($profile)?

  3. +++ b/core/includes/install.core.inc
    @@ -1550,7 +1558,11 @@ function install_profile_themes(&$install_state) {
    +  $profile_handler = \Drupal::service('profile_handler');
    +  $profiles = $profile_handler->getProfiles();
    

    Same here.

  4. +++ b/core/includes/install.inc
    @@ -1073,6 +1082,27 @@ function install_profile_info($profile, $langcode = 'en') {
    +    $base_profile_name = !empty($info['base profile']['name']) ? $info['base profile']['name'] :
    +      (!empty($info['base profile']) ? $info['base profile'] : '');
    

    I find this a bit on the convoluted side. Seeing as how it's repeated elsewhere in the patch, can it maybe be made into a method of ProfileHandler?

  5. +++ b/core/includes/install.inc
    @@ -1073,6 +1082,27 @@ function install_profile_info($profile, $langcode = 'en') {
    +      $info['dependencies'] = array_merge($info['dependencies'], $base_profile['dependencies']);
    +      // If there are dependency excludes from the base apply them now.
    +      if (!empty($info['base profile']['excluded_dependencies'])) {
    +        $info['dependencies'] = array_diff($info['dependencies'], $info['base profile']['excluded_dependencies']);
    +      }
    

    What if the base profile is, itself, depending on another base profile? Shouldn't the dependencies be resolved recursively?

  6. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -54,6 +55,13 @@ class ConfigInstaller implements ConfigInstallerInterface {
    +   * @var \Drupal\Core\Extension\ProfileHandler
    

    Should be ProfileHandlerInterface.

  7. +++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
    @@ -118,11 +118,9 @@ protected function getAllFolders() {
    +          $profiles = \Drupal::service('profile_handler')->getProfiles($profile);
    

    Why isn't this using $this->profileHandler?

  8. +++ b/core/lib/Drupal/Core/Config/InstallStorage.php
    @@ -155,16 +155,18 @@ protected function getAllFolders() {
    +      $profiles = \Drupal::service('profile_handler')->getProfiles();
    

    Is there any way to inject the profile handler?

  9. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Extension\ProfileHandler;
    

    It doesn't look like this is being used.

  10. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -248,8 +249,21 @@ public function setProfileDirectoriesFromSettings() {
    +        $profile_handler = \Drupal::service('profile_handler');
    +        $profiles = $profile_handler->getProfiles($profile);
    

    Nitpick, but can this be collapsed into a single line?

  11. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,102 @@
    +  /**
    +   * Static state cache.
    +   *
    +   * @var array
    +   */
    +  protected static $cache = array();
    

    Not a big deal, but why is this static? The ProfileHandler is stateful service maintained by the service container, so this can be a normal class member.

  12. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,102 @@
    +   * The info parser to parse the profile.info.yml files.
    

    Should be a space between "profile" and ".info.yml", otherwise it makes it sound as though we're looking for files actually named profile.info.yml :)

  13. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,102 @@
    +      if (!empty($profile)) {
    

    This if check doesn't make sense. If $profile is empty, it is set to the value of drupal_get_profile() at the very beginning of the method. So it should never, ever be empty.

  14. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,102 @@
    +        $base_profile_name = !empty($profile_info['base profile']['name']) ? $profile_info['base profile']['name'] :
    +          (!empty($profile_info['base profile']) ? $profile_info['base profile'] : '');
    

    As mentioned before, can this maybe be a method of ProfileHandler? Perhaps getBaseProfile($child_profile) or something like that?

  15. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,102 @@
    +        $filename = file_exists($profile_path . "/$profile.$type") ? "$profile.$type" : NULL;
    

    Can the $type = $profile['type'] line be moved to just before this line, for clarity's sake? Also, it's not clear what the type key does, so I think this needs a comment to explain.

  16. +++ b/core/lib/Drupal/Core/Extension/ProfileHandlerInterface.php
    @@ -0,0 +1,23 @@
    +/**
    + * Interface for classes that manage profiles.
    + */
    

    This doc comment is kinda obvious. :) How about "Defines an interface for listing and managing installation profiles."?

mpotter’s picture

Fixes from #197. Except the comment in ConfigInstaller was an existing comment and already exactly 80 chars with a period. Added @todos and tried to be more verbose with some of the comments, but probably won't iterate those any further because I expect this to all change once the other #2156401: Write install_profile value to configuration and only to settings.php if it is writeable and #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList issues are committed. Hoping that those issues clean up the code to make this cleaner.

mpotter’s picture

Lol, phenaproxima we posted at about the same time! But thanks for the detailed comments. Here is an updated patch and interdiff for your comments. And for now we get a nice round #200 comment number for it :)

1. done
2. done.
3. done.
4. well, this is allowing the profile info.yml to use a shortcut of just "base profile" as suggested in #171. This info data is not directly available within ProfileHandler and I'd rather not make the interface more complex for it at this time. I'll revisit this in the future. I don't want to do getBaseProfile($child_profile) which would then need to load and parse the info array again for this.
5. I think the dependencies already get handled recursively. All we are doing here is removing any dependencies explicitly excluded in the info.yml file. Perhaps when I actually write tests for all this I'll try doing a triple profile dependency to ensure it works properly.
6. done.
7. I didn't inject the ProfileHandler service into ExtensionInstallStorage because the constructor has the $include_profile = TRUE argument at the end. Thus, adding another dependency would change the signature and potentially break somewhere else. Once the related issues are committed if this is still required then I'll add it.
8. Similar to #7, there are places in Core that call "new InstallStorage()" so wasn't sure exactly how to inject it since it's not just coming from the services.yml.
9. done.
10. done.
11. done.
12. done.
13. The empty($profile) still makes sense because drupal_get_profile() will return an empty string if there isn't any current profile.
14. see #4.
15. moved line done. This code came from elsewhere copy/paste and handles the general case of extensions having their $type defined in the info.yml file. I didn't want to change this to hardcode it as "profile" although we certainly could if people wanted.
16. done.

OK, let's see if it still passes tests!

dawehner’s picture

I really like the simplicity of the approach! It is a bit sad, that this patch has no sign of test coverage for the new features.

  1. +++ b/core/includes/install.inc
    @@ -1029,6 +1030,14 @@ function drupal_check_module($module) {
    + * - base profile: Existence of this key denotes that the installation profile
    

    I would have expected no space in here ... any opinions about it?

  2. +++ b/core/includes/install.inc
    @@ -1029,6 +1030,14 @@ function drupal_check_module($module) {
    + *   - name: The shortname of the base installation profile.
    + *   - excluded_dependencies: An array of shortnames of other modules that have
    + *     to be excluded from the base profile requirements. This allows e.g. to
    + *     disable a demo module that would be installed by the base profile.
    

    I'm a bit confused why we add this feature as part of this patch. This seems quite some scope creep, at least for me.

  3. +++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
    @@ -86,13 +86,13 @@ protected function getAllFolders() {
    +        $profiles = $listing->scan('profile');
    +        foreach ($profiles as $profile_name => $profile_object) {
    

    What about name it $profile_extension here?

  4. +++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
    @@ -118,11 +118,9 @@ protected function getAllFolders() {
    +          $profiles = \Drupal::service('profile_handler')->getProfiles($profile);
    +          foreach ($profiles as $extension) {
    +            $profile_folders = $this->getComponentNames(array($extension));
    

    we should document why we cannot use the profile from $profiles above, as it seems to contain similar information

  5. +++ b/core/lib/Drupal/Core/Config/InstallStorage.php
    @@ -155,16 +155,19 @@ protected function getAllFolders() {
    +      $profiles = $listing->scan('profile');
    +      foreach ($profiles as $profile_name => $extension) {
    +        // Prime the drupal_get_filename() static cache with the profile info
    +        // file location so we can use drupal_get_path() on the active profile
    +        // during the module scan.
    +        // @todo Remove as part of https://www.drupal.org/node/2186491
    +        drupal_get_filename('profile', $profile_name, $extension->getPathname());
    +      }
    +      // Now that we can fetch the path, get dependent profiles and add
    +      // the extensions.
    +      $profiles = \Drupal::service('profile_handler')->getProfiles();
    +      foreach ($profiles as $extension) {
    +        $this->folders += $this->getComponentNames(array($extension));
    

    Also here we should document why we need to use both services ...

  6. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -248,8 +248,20 @@ public function setProfileDirectoriesFromSettings() {
    +      // ExtensionDiscovery can be called without a service container.
    +      // (@drupalKernel::moduleData) so check if profile_handler is available.
    +      if (\Drupal::hasService('profile_handler')) {
    +        $profiles = \Drupal::service('profile_handler')->getProfiles($profile);
    

    Just a random idea. What about allow to pass along the profile handler as constructor argument, and just otherwise fallback to \Drupal::service()?

  7. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,104 @@
    +
    +/**
    + * Class that manages profiles in a Drupal installation.
    + */
    +class ProfileHandler implements ProfileHandlerInterface {
    +
    
    +++ b/core/lib/Drupal/Core/Extension/ProfileHandlerInterface.php
    @@ -0,0 +1,23 @@
    +/**
    + * Defines an interface for listing and managing installation profiles.
    + */
    +interface ProfileHandlerInterface {
    +
    

    Can we document for example when this should be used over the extension list ? (I guess most of the time, as it provides caching and some additional abstractions?)

  8. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,104 @@
    +    static $weight;
    

    Do we really need a static variable here? I cannot really think of a reason.

  9. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,104 @@
    +
    +        $extension->info = $profile_info + $info_defaults;
    +        $extension->weight = $weight;
    +        $extension->origin = '';
    

    I'm wondering whether instead of shuffling it onto the existing extension object and knowing that PHP just likes you I'm wondering whether we could have a ProfileExtension extends Extension or something similar here.

  10. +++ b/core/lib/Drupal/Core/Extension/ProfileHandlerInterface.php
    @@ -0,0 +1,23 @@
    +  /**
    +   * Returns a list of related installation profiles.
    +   *
    

    Let's define that "related" means.

  11. +++ b/core/lib/Drupal/Core/Extension/ProfileHandlerInterface.php
    @@ -0,0 +1,23 @@
    +   * @return array
    +   *   List of installation profile extensions keyed by profile name
    +   *   in descending order of their dependencies.
    +   *   (parent profiles first, main profile last)
    

    I love that the order is documented

mpotter’s picture

Yep, tests are coming. Hopefully today.

1. "base profile" is modeled from "base theme", so I think the space is appropriate.

2. Not being able to exclude dependencies was a reason much much earlier in this thread for *not* implementing true inheritance. In many client profiles you don't want to inherit the full list of enabled modules from the base profile. This feature removes this limitation in a clean way making inherited profiles much more useable in the real world. So I applauded das-peter for adding this. I'll probably move some of the logic for this into ProfileHandler to also make it easier to test.

3. Yep. Actually changing it to $extension to mirror the loop naming later in that same function.

4. Will improve the comment. scan('profile') returns a list of all profiles found on the file system in no particular order. ProfileHandler::getProfiles returns the ordered list of just the current profile and its parent(s).

4a. The whole bit done here (and elsewhere) to "prime the static cache" for drupal_get_filename() I find messy and I'm thinking of ways to move that into another method of ProfileHandler.

5. Same as 4.

6. I think that is a good idea as it would allow moduleData to pass the profile info in similar to what it's doing with the config in the pseudo container. Similar to figuring out how to inject ProfileHandler as mentioned in #198 so it's not a hard-coded service. I'll take a stab at this.

7. Related to 4 and 5. Yes, I'd rather put the documentation into ProfileHandler itself rather than needing to comment on it every time it is used.

8. So getProfiles() is a recursive function. This static was a way to persist the weight as it gets incremented as the profile tree is traversed. Couldn't see a cleaner way to do it without breaking the function into more pieces, but I'm welcome to suggestions.

9. This is a bigger question. It's not just "ProfileExtension". Even Module extensions need the origin and weight. I think this is more of an issue with Extension itself being abused and these properties need to be added to the parent class if they are being used like they are. But see the code in system_rebuild_module_data() for example. Drupal Core is doing this kind of "ad hoc property" stuff with Extensions all over the place.

10. Will do.

11. Yep, since that's the whole main point of this ;)

Thanks for the feedback. Will be posting new patches today with some changes and start of testing.

dawehner’s picture

Drupal Core is doing this kind of "ad hoc property" stuff with Extensions all over the place.

Sure not question, but this is not a reason to not add more sadness :)

mpotter’s picture

8. Hmm, the $weight stuff was broken...I was stupid. Reworking.

9. It's not really adding "more sadness", it's just moving some sadness from _system_rebuild_module_data into profile_handler. I'm working on a new iteration that encapsulates this $info processing stuff into another protected function that should make it easier to clean up in the future as needed. Also moving the handling of dependencies and excluded_dependencies out of the install.inc and into profile_handler.

Edited: Also, the actual messy bit was the need for "info_defaults" since the way _system_rebuild_module_data() is currently written we cannot set the "required" property when the info.yml is first parsed. It can only be set *after* the _system_rebuild_module_data_ensure_required() is called. As I mentioned in #192, this is all messy until the bigger problems with _system_rebuild_module_data() can be solved. For now just doing what needs to be done to get it working without involving all those related issues.

dawehner’s picture

9. It's not really adding "more sadness", it's just moving some sadness from _system_rebuild_module_data into profile_handler. I'm working on a new iteration that encapsulates this $info processing stuff into another protected function that should make it easier to clean up in the future as needed. Also moving the handling of dependencies and excluded_dependencies out of the install.inc and into profile_handler.

That's fair :)

RajabNatshah’s picture

Mike: You do have a better solution for this.
I have tested all patches. I will still wait to get to the best fix solution for this.

About the Profile Management.

1. Install Drupal, Install Parent profile, and then Install the Child profile.
- The [Profile manager] will only work in the installation process. around the selection of profiles.
- We could have the [Profile Manager Generator] only to generate new profiles.
- Profile configuration and conflicts with drupal or parent profile.
2. Update Drupal, Update Parent profile, and then Update the Child Profile.

Rewarded work :)

mpotter’s picture

RajabNatshah: I think you should create a new issue for discussion of a Profile Management. You don't "Install Drupal, Install Parent profile, then Install Child profile". You just install the Child profile directly. There isn't any such thing as "Install Drupal". You are *always* using some sort of profile, such as "Standard" or "Minimal". Please keep the discussion in this issue directly related to this patch. This issue has already gone off track too many times in it's 5 year history. This is not the place for other discussions.

Back to the patch. Before embarking on tests, I wanted to upload a new version of the patch to run the testbot on some refactorings mentioned in #202. I moved a lot more profile code into ProfileHandler to simplify other functions. Also dealt with the messy drupal_get_filename() caching issue for profiles. This work should make it easier to refactor as other related issues get committed.

Added an interdiff to make it easier to comment, but would love another review by people on these changes while I go work on tests. Thanks!

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -2,21 +2,33 @@
     /**
      * Class that manages profiles in a Drupal installation.
      */
     class ProfileHandler implements ProfileHandlerInterface {
     
       /**
    

    I would like to have all that stuff to be marked as internal to allow for further improvements in the future. This is needed maybe to get into a better shape with #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList again.

  2. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -46,59 +65,195 @@
    +    $modules_cache = &drupal_static('system_rebuild_module_data');
    

    Urgs. Is there no way to get around that?

  3. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -46,59 +65,195 @@
    +  static function getProfileBaseName($info) {
    

    Is this method meant to be public?

mpotter’s picture

Bleh, sorry about that. Try again. Removed the debug stuff and the info_defaults that I forgot to remove.

(Also, the patch itself might be easier to review than the interdiff at this point ;)

mpotter’s picture

dawehner:

1. Not sure what you mean by "all that stuff to be marked as internal". At this point we should consider everything in ProfileHandler to be subject to change based on the related issues we have already linked previously. I think there are already several @todo comments about reworking stuff based on those issues.

2. Sadly not that I could find. I sort of "raged" when I discovered that the drupal_get_filename() function calls trigger_error() when there is a cache-miss rather than firing any sort of exception that can be caught. So this was the best I could come up with. This whole cache priming thing is a complete mess, but at least this moves it out of several other places for work later.

3. Nope, I meant it to be static for now. It allows modules to use ProfileHandler::getProfileBaseName without instantiating the service to just determine the base profile name. This was more important before I ended up consolidating the calls and moving them all into ProfileHandler itself. But for a while other modules were calling this, so I left it. We might eventually just make it protected if we decide nobody else needs this information.

btw, that was a fast review! But you didn't notice the debug_backtrace() call ;)

The last submitted patch, 207: make_inherited_install-1356276-207.patch, failed testing.

dawehner’s picture

1. Not sure what you mean by "all that stuff to be marked as internal". At this point we should consider everything in ProfileHandler to be subject to change based on the related issues we have already linked previously. I think there are already several @todo comments about reworking stuff based on those issues.

Right, but in order to be able to do that, we need those classes/interfaces as @internal ...

3. Nope, I meant it to be static for now.

Well yeah, either make it public or protected, but don't leave it hanging in the middle of it and let PHP fallback to public :)

btw, that was a fast review! But you didn't notice the debug_backtrace() call ;)

I just expected you to fix it anyway :P

Status: Needs review » Needs work

The last submitted patch, 209: make_inherited_install-1356276-208.patch, failed testing.

mpotter’s picture

1. OK, let me know where to put the "@internal"...I've never done that. Although I'm not sure I agree it's @internal since I think it handles what it is doing in a really clean way and we'll always need a service to return a list of profiles in proper order.

3. Doh, yep.

OK, here is a new patch that has the first pass at tests. The ProfileHandler testing is covered. All we need to do next is actually test a real install. I'll poke around the existing tests to see how that is done. But I've added the testing_inherited profile (inherits from minimal) so testing that it installs and that the proper modules get enabled (or not) should be an easy job for tomorrow. I'm done for today.

Edited: Uh-oh, tests failed above, so this won't pass either. More work for tomorrow.

RajabNatshah’s picture

mpotter: I totally understand what you are talking about.

Agree with you I will create a new issue or new project about the "Profile manager" : Profile installation handler, Profile update management, and malty nested profiles.

If you read my first comment #191

full Start to End solution.

- Profiles must install and start working without showing the "Base Profile".
- Profiles must start work without the need to any changes in the settings.php.

I will keep following and testing patches, until we do have a stable and tested one.

Rewarded work you have :)

mpotter’s picture

Status: Needs work » Needs review
FileSize
33.99 KB
5.18 KB

I think I found the source of at least one of the failing tests. And it's related to the request by RajabNatshah in #215. The last refactor was setting the default distribution name to "Drupal" in the ProfileHandler. But we don't want this because then *every* profile has a distribution name and the installer will always pick the first one it finds. This resulted in the Minimal profile always getting installed.

Just picking the first matching distribution seemed a bit arbitrary, so in this patch I have add a new function to ProfileHandler to select a distribution from a list of profiles. Any base-profiles are removed from the list. Then if there are more than one it will select the first one.

So now if the child profile adds the distribution name to it's info.yml file, it should get selected as the profile to install even if it's parent profile also has the distribution name selected. If the child profile doesn't add a distribution name, then rather than just installing the parent profile, the user gets a choice of which profile to install. This puts control over the installer back into the hands of the main child profile rather than letting the parent profile control it.

I also added a new test that actually tests installing an inherited profile and ensures the proper modules are enabled or excluded.

The last submitted patch, 216: make_inherited_install-1356276-216.patch, failed testing.

mpotter’s picture

Issue tags: -Needs tests

Woot, tests pass! I'm done with my refactoring now, so any last feedback would be welcome.

dawehner’s picture

This patch in general is great, here are some points I would have pointed out in a review.

I still bet with you that every committer will say: excluded extensions is a scope creep, but well, if you think you actually save time by not creating simply a small follow up issue.
Given that we are in beta phase for 8.2.x now, this is a 8.3.x feature.

Status: Needs review » Needs work

The last submitted patch, 220: 1356276-220.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.77 KB
1.08 KB

This should be it, ideally.

mpotter’s picture

Thanks for updating the patch with your changes. I'm not up on the latest coding standard stuff and didn't realize we were using [] over array() now, so that was good to see. Also, thanks for updating the new test to be a Browser test instead of the older WebTest. I looked at both the Minimal and Standard profile tests as examples, but they each do it differently and wasn't sure which to follow (I followed Minimal which I guess is outdated).

My only concern is the duplicate code for the tests for selectDistribution and selectDistributionExtension. I can understand that my attempt to write a more general purpose function that accepted either strings or objects might not follow strict guidelines. But having both functions seems to make the class/interface messier especially given that this is only needed by the installer.

In fact, it looks like in install.core.inc the $install_state['profiles'] array is already keyed by the profile name, so I suggest we just keep selectDistribution and pass it array_keys($install_state['profiles']) since _install_select_profile() returns a profile string name anyway. Let me know what you think.

dawehner’s picture

My only concern is the duplicate code for the tests for selectDistribution and selectDistributionExtension. I can understand that my attempt to write a more general purpose function that accepted either strings or objects might not follow strict guidelines. But having both functions seems to make the class/interface messier especially given that this is only needed by the installer.

Well, as you just realized in your comment is that we actually just need one of the two methods. Having one method which has two behaviours is always a code smell and a sign that actually it should work in a different way.

mpotter’s picture

Oh also looks like we also need a test for the shortcut "base profile: name". The new code added to getProfileInfo()

        $info['base profile'] += ['excluded_dependencies' => []];

won't work in that case. So I'll add code to normalize the $info to handle both syntaxes.

The last submitted patch, 220: 1356276-220.patch, failed testing.

phenaproxima’s picture

This looks pretty good to me, honestly. I have nitpicks, but nothing major leaps out at me. I don't think I'm experienced enough with the low-level extension handling systems to to RTBC this patch.

  1. +++ b/core/includes/install.core.inc
    @@ -1213,11 +1221,8 @@ function _install_select_profile(&$install_state) {
    +  if ($distribution = \Drupal::service('profile_handler')->selectDistribution(array_keys($install_state['profiles']))) {
    +    return $distribution;
    

    Ye gods. Can we do the call to selectDistribution() on its own line, purely for readability?

  2. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -99,6 +99,15 @@ class ExtensionDiscovery {
    +   * It is used to determine the directories we want to scan modules for.
    

    Should be "in which we want to scan for modules"?

  3. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -248,8 +267,18 @@ public function setProfileDirectoriesFromSettings() {
    +        $profile_directories = array_map(function($extension) {
    +          return $extension->getPath();
    +        }, $profiles);
    

    I've seen this repeated elsewhere in the patch. Maybe it could be added as a convenience method -- ProfileHandlerInterface::getProfileDirectories()?

  4. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,302 @@
    +   * @var \Drupal\Core\Extension\Extension[][]
    

    Why [][]?

  5. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,302 @@
    +    if (!$this->scanCache && !isset($modules_cache)) {
    

    $this->scanCache is not defined on the object. Can it be?

  6. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,302 @@
    +    if (!isset($this->infoCache[$profile])) {
    +
    +      // Set defaults for profile info.
    

    There's an extra blank line here.

  7. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,302 @@
    +      if ($base_profile_name = $info['base profile']['name']) {
    

    Shouldn't we do isset($info['base profile']) first?

  8. +++ b/core/lib/Drupal/Core/Extension/ProfileHandlerInterface.php
    @@ -0,0 +1,75 @@
    +  public function setProfileInfo($profile, $info);
    

    $info should be type hinted as an array.

  9. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ProfileHandlerTest.php
    @@ -0,0 +1,107 @@
    +  /**
    +   * @covers ::selectDistribution
    +   * @covers ::setProfileInfo
    +   */
    +  public function testSelectDistribution() {
    

    No description in the doc comment.

dawehner’s picture

This addresses feedback from @phenaproxima and myself.

I've seen this repeated elsewhere in the patch. Maybe it could be added as a convenience method -- ProfileHandlerInterface::getProfileDirectories()?

To be honest I really like the general idea to NOT have arbitrary helper methods, but actually always deal with the objects.

Why [][]?

I renamed the variable to $profilesWithParentsCache to make its purpose a bit clearer.

$this->scanCache is not defined on the object. Can it be?

Its for me :)

Shouldn't we do isset($info['base profile']) first?

IMHO no, because we += ed above already.

dawehner’s picture

A couple of more small improvements.

DuaelFr’s picture

I did some manual testing.
It works quite well, congratulations!

I have some issues though:

  1. the base profiles themes are not inherited, you have to define your own theme section in your info.yml file
  2. all the base profile config/install files seems to be loaded so, if you exclude a dependency that's needed for one of these files (or if you forget to define the same themes as the base profile), you'll get an error upon install

Let's try with this very simple profile:

name: testinherit
type: profile
core: 8.x
base profile:
  name: standard
  excluded_dependencies:
    - block_content

The result is :

Starting Drupal installation. This takes a while. Consider using the --notify global option.
exception 'Drupal\Core\Config\UnmetDependenciesException' with message 'Configuration objects
(block.block.bartik_account_menu, block.block.bartik_branding, block.block.bartik_breadcrumbs,
block.block.bartik_content, block.block.bartik_footer, block.block.bartik_help,
block.block.bartik_local_actions, block.block.bartik_local_tasks, block.block.bartik_main_menu,
block.block.bartik_messages, block.block.bartik_page_title, block.block.bartik_powered,
block.block.bartik_search, block.block.bartik_tools, block.block.seven_breadcrumbs,
block.block.seven_content, block.block.seven_help, block.block.seven_local_actions,
block.block.seven_login, block.block.seven_messages, block.block.seven_page_title,
block.block.seven_primary_local_tasks, block.block.seven_secondary_local_tasks, block_content.type.basic,
field.field.block_content.basic.body) provided by standard have unmet dependencies'

Themes not being inherited, none of the blocks can be installed, plus, block_content being excluded, its configuration cannot be installed either.

mpotter’s picture

Since the theme is one of the first things usually changed on a site I don't think not inheriting the theme is necessarily a problem. Profile themes are often tied to its functionality and like the case of the config you mentioned it's very possible a theme could be broken if you turned off some of the modules.

My guess is that the theme issue will be solved or made easier by those other core issues that are meant to better treat modules, themes, and profiles uniformly so I'd wait till they get finished before trying to mess with this patch anymore.

On the config issue, I don't see any way to really fix that. This is why I discourage profiles from putting config directly within the profile rather than within some modules. But I think your example of excluding "block_content" is a bit contrived and in reality you wouldn't be doing that. All this means is that certain profiles are better suited to being a "base profile" than others.

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community

I was just thinking that it would work more like themes where everything that's not explicitely excluded or overriden is inherited from the base theme.

If my concerns in #232 are not relevant, then this patch is RTBC for me.
I'm going to use it in production for my next project. Let's live dangerously ;)

catch’s picture

Component: install system » extension system
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +Needs change record

This issue really needs an issue summary update and draft change notice.

Also moving it to extension system since that's where most of the changes are.

The last submitted patch, 214: make_inherited_install-1356276-214.patch, failed testing.

dawehner’s picture

Issue summary: View changes

I'm honestly believe that we should think about #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList as well, given its really related and ensure that this issue will fit into the long term bigger picture.

timcosgrove’s picture

Noting that this change in Features, committed to 8.x-3.0-rc1:
https://www.drupal.org/node/2625310

Breaks the work done in patch 227
https://www.drupal.org/files/issues/1356276-227.patch

because that core patch for profiles modifies ConfigInstaller, and the Features changes include FeaturesConfigInstaller now extending ConfigInstaller rather than just implementing the ConfigInstallerInterface.

You can avoid the issue by pinning Features at 8.x-3.0-beta9 for the time being.

Not sure where to put this comment, but posting here.

mpotter’s picture

Status: Needs work » Needs review
FileSize
40.42 KB
1.47 KB

Here is an updated patch to fix the issue from #238. We forgot to make the new profile_handler argument optional in the ConfigInstaller. But also from #238, you should not pin Features to beta9 since it has important bug fixes since then.

Also, reminder that this patch probably shouldn't be RTBC'd until the related issues mentioned above in the Extension system are committed first. This patch will need to be updated depending on those changes. We don't want to commit this patch first and then have to work around it in other core issues.

kreynen’s picture

Glad to see #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList is no longer postponed. Since we are hoping to use a base profile and sub profiles for each University of Colorado campus, we'd really like to help move this forward in any way that would be useful. Is it worth building proof of concept base/sub profiles using #239 and the dev branch of Features? The latest Extension system patches are failing to apply. @mpotter are you saying that getting this into 8.3 will depend on that? Should we only be testing if we can get that to apply?

mpotter’s picture

kreynen: The above patch #239 should work fine with Drupal core 8.2.x and the latest Features rc1. We use it on several projects already that have profiles using lightning as the base profile. (I think it also works with Drupal core 8.1.x but haven't tried in a while)

What I am saying is that for 8.3.x we want to work on the other core Extension issues first, get those committed, and then rework this patch as needed to get it working with the new extension system. So stay tuned to this issue thread for updated 8.3.x patches as they are needed. But the intent is to keep the functionality of this patch and just adjust it as needed for api changes. The idea of using "base profile" in your info.yml etc shouldn't change.

I don't think you need the other extension issues for this, unless you are doing something else that requires those other patches. Certainly #239 is not going to work with those other patches yet. I want to wait until they are more rtbc'd before spending time reworking the patch here since I have limited time on this.

So you don't even need to build a proof of concept. The concept has already been proven on several projects. Just go ahead and use #239.

DuaelFr’s picture

@mpotter For the record, we use this patch with Features rc1 and it works well except that Features contained in the parent profile are not shown in the Features UI and they are never used to calculate dependencies. Should I open an issue in the Features queue?

mpotter’s picture

DuaelFr: I cannot reproduce that. I build the Octane project which has Lightning as it's base profile and I can see the Lightning features in the Features UI just fine. Now, there are maybe some other issues with Features in 8.2 that could be related to this inherited profile patch that I am investigating, but let's keep the discussion over in the Features queue since this patch thread is plenty long enough without side-tracking. If it turns out you find an issue with this patch, upload a new patch here.

josebc’s picture

Patch in #239 fixes features issue but breaks drush for me
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container. in [error]
/core/lib/Drupal.php:129

Anybody else facing the same issue?

mpotter’s picture

josebc: what version of drush and what drush command are you trying to call? I've used both drush 7 and 8 without a problem, but maybe it is a specific version issue? Probably also need to mention if the error is happening during your drush site-install what your profile setup looks like. I've only been testing when using Lightning as a base profile.

Edited: Also, is this just a problem with #239? Did #231 have the same problem? The more details on reproducing this you can give the better the chance of getting it fixed.

josebc’s picture

mpotter thank you for the reply, I tried the patch on a clean lightning and its working good, I'm using head drush so that is not the issue.
I'm guessing it happens when used with another patch or module, ill keep digging and post back here in case anyone else is facing the same problem

josebc’s picture

I tracked down the problem to libraries module in LibrariesServiceProvider alter method, still need to figure out if the issue is with the service or the alter method
error happens after calling $config_factory = $container->get('config.factory');

brantwynn’s picture

Can we get an issue summary update? Its unclear to me what is happening now as I have been out of the loop and we've been using the patch from #157 like bad kids over in the Demo Framework project to "inherit" Lightning for too long...

Perhaps @mpotter (or anyone else) can confirm the fllowing:

1. Declare base_profile, in my info file -- ie: octane.info.yml declares base_profile: lightning
2. Do a site-install using my profile as the argument (drush si octane)
- First,the base profile (lightning) is installed, any config/install hook_install or otherwise are run.
- Second, the sub profile (octane) is installed, any config/install or hook_install should not duplicate, but possibly override base profile

Is this a correct description of the expected functionality to test?

mpotter’s picture

@brantwynn:
1) The key is "base profile" (with a space, not a _. Same as the convention for base theme)
Otherwise yes, you got it. You site-install your profile.

While doing some unit testing, I found a problem with this patch since the InstallStorage tries to grab the profile_handler service when there might not be a container. Here is an updated patch and interdiff.

josebc’s picture

Last patch seems to fix issue with libraries

kreynen’s picture

The updated syntax of base[SPACE]profile vs. base_profile confused me at first because the Octane example on GitHub was still using base_profile. I submitted a PR to fix that https://github.com/phase2/octane/pull/4/files

I'm also seeing notices if excluded_dependencies isn't included in the base profile definition or it is included with no items with #249. It only works as expected when at least one project from the base profile is excluded. I didn't add excluded_dependenciesto the octane PR, so these notices should be easy to reproduce.

I also have some concerns about steps in a base profile's install and update hooks that will assume a dependency defined in the base profile is enable, but I guess the solution for that is anyone who wants a profile to be used as a base needs to write install and update hooks that check what's enabled in case a sub profile is excluding dependencies.

DuaelFr’s picture

I found two little minor issues related to this patch.

  1. If the base profile has a dependency that you excluded in your own profile. Then, for any reason if you enabled that dependency manually after the installation. You cannot uninstall it anymore as the parent profile is enabled a depends on it.
  2. The parent profile is available in the admin/modules/uninstall UI and nothing prevents this to happen.
dwkitchen’s picture

I had the same problem with excluded_dependencies, I simply added the key with a single blank dependency as a tempory solution.

base profile:
  excluded_dependencies:
    -

I also found that theme dependencies are not correctly loaded. My base profile uses Stark and there is install config for blocks. The child profile uses Seven so there where unmet dependency error until I added Stark as a dependency of the child profile.

EDIT: I also found that when I added tasks from the base_profile install they didn't run as the functions were not found. The base_profile.profile is not being loaded during install. I added this to the child_profile.profile file:

module_load_include('profile', 'base_profile');
phenaproxima’s picture

Status: Needs review » Needs work

We discovered a problem with this patch over in #2831438: dblog and UI modules should not be profile dependencies. In short, if you have a profile with dependencies, and you install a sub-profile of that profile, you can no longer uninstall dependencies of the base profile.

I traced this a little bit and discovered that the trouble comes down to line 146 of system module's ModulesUninstallForm:

if ($dependent != $profile && drupal_get_installed_schema_version($dependent) != SCHEMA_UNINSTALLED) {

$profile is the result of drupal_get_profile().

When this line is reached, the module being checked will have both profiles -- the installed profile, and its base profile -- listed as dependendents. But it's only checking that the installed profile is a dependent. Therefore, the form will think that the module is depended upon by the base profile, and refuse to uninstall it.

To me, the real fix here is to enforce that modules are never, ever dependent upon install profiles. An install profile is essentially meant to be a one-and-done thing. It runs during install time, sets things up, then should fade into the ether. Having modules depend on profiles is not only deeply unkosher, but it leads to these kinds of problems. I'm not sure exactly how to fix this, but it seems like system_rebuild_module_data() may need some love. This problem has always existed, apparently, but it's being exacerbated and manifested by this patch (and the whole concept of sub-profiles). So I suggest we handle this here, now.

kreynen’s picture

To me, the real fix here is to enforce that modules are never, ever dependent upon install profiles. An install profile is essentially meant to be a one-and-done thing. It runs during install time, sets things up, then should fade into the ether.

"Enforcing" this would really limit how install profiles could be used. In D7, we wrote Profile Module Manager in part to keep make sure that the dependencies declared in the install profile used by ~1,000 sites we run as a service for the University of Colorado remained. Currently, if a module that was added as a dependency to the profile's .info failed to get enabled (for whatever reason), the profile itself would get disabled on the next module enable and any alters or future update hooks in the profile would never run.

The idea that an install profile should be nothing but the distribution of code that *could* be used together is not new, but I don't see what's stopping anyone from developing a profile that is just a code distribution while still supporting other use cases like a single application with an install profile. Most large/mature profiles have already adopted the concept of a "core module" that is enabled by the install profile and manages the dependencies. With Express, we install slightly different cores for internal hosting, hosting on Pantheon, and testing on Travis CI. Projects required by the profile itself are common to all cores, but as we move towards D8 we are planning on doing even less in the profile and moving most of what was done in the cores to subprofiles.

I don't think the problem is with the patch, but with the profile you are using as the base trying to do too much and getting in the way of what you want to do at the subprofile level.

Aren't these really the same issues you'd have using a theme as a base that wasn't designed to be a base and overreaches in what it is trying to do?

balsama’s picture

The problem I see is that this patch effectively changes the way core treats dependencies of profiles. Without the patch, dependencies of profiles can be uninstalled. I don't think this issue aimed to change that behavior - which it does.

If/when this finally lands, I can see users using Standard as a base profile and then extending Standard with a subprofile. Currently, that setup would disallow users from uninstalling views_ui, for example, because Views UI is a dependency of Standard.

geerlingguy’s picture

FYI, we should probably update the Issue Summary at this point—it was last updated as of comment #96, and is now ~150 comments and ~3 years out of date...

brantwynn’s picture

Late to dinner here as usual but let me clarify something? Are we saying in:

The problem I see is that this patch effectively changes the way core treats dependencies of profiles. Without the patch, dependencies of profiles can be uninstalled. I don't think this issue aimed to change that behavior - which it does.

So like, if this patch goes into core as-is, then someone installs the Standard profile, they would not be able to uninstall views_ui or any other module declared in standard.info.yml - correct?

To me, this actually makes a lot of sense, and perhaps Drupal should have worked this way all along. However, it has not worked this way for years and people expect to be able to override the basic setup. This would discourage any users who expect to be able to click their way to solutions knowing little, if not nothing at all about how the install profile system works.

Could Drupal provide a 'spartan' install profile that is recommended for actual builds? It would basically need to be empty. Perhaps it could be so bold as to delcare node as a dependency but I'm not going to go crazy.

At the same time, what @balsama says in #256 makes sense. We could rebuild the Standard profile to work as-is, but as a sub-profile. A Sub-Standard profile, if you will?

We would need to provide some type of community consensus for doing this. One idea that comes to mind is combining this with the existing demo profile (snowman?) efforts.

Proposal:

  • In addition to the work from patch #249
  • Add 'spartan' profile and extend it (call it whatever you want, the bikeshed is over here?: {future issue placeholder})
  • Refactor standard profile and stark profiles to extend 'spartan'
  • Possible Followup: convert standard profile over to a more fleshed out "demo" that contains example content
brantwynn’s picture

I got some further clarification from chatting online with @balsama that I had missed, which may affect some decisions.

For the record, only dependencies of parent profiles are affected. so if you were _just_ using standard, not extending it, you could still turn [views_ui] off

Given that likely a high percentage of users that install the Standard profile are intending to click around and maybe turn things on or off, it could also be left as-is with this new system merged in. These users would never be affected by the sub-profile problem unless a vendor delivered them a sub-profile driven site without letting them in on this nuance. E.g. someone downloads and installs Lightning, then cannot disable some things and is confused or dissatisfied with that experience.

In an ideal world, only people that are willingly opting in to being locked down by their parent profile dependencies would sign up for using sub-profiles.

Existing base profiles can also change to accommodate this feature - likely by using an install hook to enable a *_core module that finishes the actual install process we've come to know and love from distributions. E.g. Lightning has no dependencies in it's *.info.yml but still kicks off the module installer in lightning.install for lightning_core or whatever else it reads from lightning.extend.yml

mpotter’s picture

From #254, couldn't we also change the ModulesUninstallForm so that instead of just using drupal_get_profile() to get the current installed profile that instead it calls the ProfileHandler service to get a list of all profiles? If the module is part of any of the profiles then we let it be uninstalled.

That would seem to maintain the existing core behavior. This patch has been sidetracked *so* many times that I think the decision on whether to change the way core handles module dependencies of profiles mentioned in the other comments should be handled in a separate issue.

Trying to get this back on track...seems like the remaining work on patch from #249 is:

  1. Handle missing excluded_dependencies (from #251 and #253)
  2. Handle theme dependencies (from #253)
  3. Allow modules from parent profile to be uninstalled (from #254)
  4. Remove parent profiles from module listing (from #252)

Items listed in #258 to go to a new issue.

sylus’s picture

I just wanted to give a huge thank you to @mpotter for getting this moving and functionality mostly complete. I am successfully now extended off of lightning by just the following:

distribution:
  name: WxT

# Base profile
base profile: lightning

It properly inherits all of the enabled modules and I only need to extend with our additions. Definitely a great DX workflow :) Again greatly appreciated.

Next I will be trying one more + final level of inheritance. @mikepotter do you foresee any issues with:

install profile (opendata) -> install profile (wxt) -> install profile (lightning)

Edit: multiple inheritance (see above) does work as well!

balsama’s picture

Small update to allow module dependencies of parent profile to be uninstalled:

  1. Handle missing excluded_dependencies (from #251 and #253)
  2. Handle theme dependencies (from #253)
  3. Allow modules from parent profile to be uninstalled (from #254)
  4. Remove parent profiles from module listing (from #252)
balsama’s picture

Status: Needs work » Needs review
FileSize
42.33 KB
703 bytes

And another tiny update to remove the parent profile from the list.

  1. Handle missing excluded_dependencies (from #251 and #253)
  2. Handle theme dependencies (from #253)
  3. Allow modules from parent profile to be uninstalled (from #254)
  4. Remove parent profiles from module listing (from #252)

Setting to NR to trigger tests.

oriol_e9g’s picture

FileSize
969 bytes

We are planning to use this feature, we can help testing. I have go away the excluded_dependencies notices with a simple isset.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -73,13 +81,21 @@ class ConfigInstaller implements ConfigInstallerInterface {
    +    if (!isset($profile_handler) && \Drupal::hasService('profile_handler')) {
    +      $this->profileHandler = \Drupal::service('profile_handler');
    +    }
    +    else {
    +      $this->profileHandler = $profile_handler;
    +    }
    

    This can be $this->profileHandler = $profile_handler ?: \Drupal::service('profile_handler') for brevity.

  2. +++ b/core/lib/Drupal/Core/Config/InstallStorage.php
    @@ -56,9 +64,17 @@ class InstallStorage extends FileStorage {
    +    if (!isset($profile_handler) && \Drupal::hasService('profile_handler')) {
    +      $this->profileHandler = \Drupal::service('profile_handler');
    +    }
    +    else {
    +      $this->profileHandler = $profile_handler;
    +    }
    

    Ditto.

  3. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -102,12 +111,23 @@ class ExtensionDiscovery {
    +    if (!isset($profile_handler) && \Drupal::hasService('profile_handler')) {
    +      $profile_handler = \Drupal::service('profile_handler');
    +    }
    +    elseif (!isset($profile_handler)) {
    +      $profile_handler = new FallbackProfileHandler($root);
    +    }
    

    I think it would be less confusing to change this to a try-catch around ServiceNotFoundException:

    try {
      $profile_handler = $profile_handler ?: \Drupal::service('profile_handler');
    }
    catch (ServiceNotFoundException $e) {
      $profile_handler = new FallbackProfileHandler($root);
    }
    
  4. +++ b/core/lib/Drupal/Core/Extension/FallbackProfileHandler.php
    @@ -0,0 +1,73 @@
    +  public function getProfileInfo($profile) {
    +    if (isset($this->profileInfo[$profile])) {
    +      return $this->profileInfo[$profile];
    +    }
    +  }
    

    The interface doc comment says this should return an array, but it will be implicitly returning NULL if $profile does not exist.

  5. +++ b/core/lib/Drupal/Core/Extension/FallbackProfileHandler.php
    @@ -0,0 +1,73 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function selectDistribution($profile_list) {
    +  }
    

    This should return an explicit NULL for interface conformance.

  6. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,301 @@
    +
    +  /**
    +   * Returns an extension discovery object.
    +   *
    +   * @return \Drupal\Core\Extension\ExtensionDiscovery
    +   *   The extension discovery object.
    +   */
    +  protected function getExtensionDiscovery() {
    +    if (!isset($this->extensionDiscovery)) {
    +      $this->extensionDiscovery = new ExtensionDiscovery($this->root);
    +    }
    +    return $this->extensionDiscovery;
    +  }
    

    Why is this method needed? Couldn't we just do this in the constructor:

    $this->extensionDiscovery = $extension_discovery ?: new ExtensionDiscovery($root)

  7. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,301 @@
    +      // module scan as the module scan requires knowing what the active profile is.
    

    Nit: Exceeds 80 characters.

  8. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,301 @@
    +        // Prime the drupal_get_filename() static cache with the profile info file
    

    Ditto.

  9. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,301 @@
    +      $info = $this->infoParser->parse($profile_file);
    +      $info += $defaults;
    

    I think this could be combined into one line:

    $info = $this->infoParser->parse($info_file) + $defaults

  10. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,301 @@
    +      // Installation profiles are hidden by default, unless explicitly specified
    

    Nit: Exceeds 80 characters.

  11. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,301 @@
    +      if (!empty($profile)) {
    

    This check makes no sense to me. The first thing this method does is set $profile if it was empty -- so why would it be empty after that? The rest of the function cheerfully assumes that $profile has a value.

  12. +++ b/core/lib/Drupal/Core/Extension/ProfileHandler.php
    @@ -0,0 +1,301 @@
    +  public function selectDistribution($profile_list) {
    

    $profile_list should be type hinted.

  13. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
    @@ -143,7 +146,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        if (!in_array($dependent, array_keys($profiles)) && drupal_get_installed_schema_version($dependent) != SCHEMA_UNINSTALLED) {
    

    Micro-optimzation -- we're calling array_keys($profiles) repeatedly in this loop. Can we instead call it once before the loop begins?

phenaproxima’s picture

Rerolled against 8.4.x and addressed my own feedback except for #266.11.

Status: Needs review » Needs work

The last submitted patch, 267: 1356276-267.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
44.28 KB
6.35 KB

Fixed a few things I missed that will break the tests, and I addressed #263.2, at least to some extent. Parent profile themes will now be installed along with themes from the child profile, and the child profile can exclude parent themes the same way it can exclude parent dependencies. (I added an excluded_themes key for this purpose.) And I adjusted the tests to cover these changes. Bam!

Status: Needs review » Needs work

The last submitted patch, 269: 1356276-269.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
44.06 KB
1.23 KB

Sorry about that. Arrogant idiocy on my part.

Status: Needs review » Needs work

The last submitted patch, 271: 1356276-271.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
44.62 KB
3.16 KB

Fixing that failing test and confirmed that this patch applies cleanly against 8.3.x.

phenaproxima’s picture

Here's a backwards reroll of the patch against 8.2.x, for distributions like Lightning that want to leverage this functionality now.

mpotter’s picture

Sorry for the delay. Answers to #266:

1 & 2) It seems like there were cases where the service wasn't available in some situations like when ModuleData is called without any service container. But maybe that only applies to the ExtensionDiscovery class and these others were just from copy/paste.

3) I don't think that is the correct Exception. I think when ExtensionDiscovery is called without a container you get the ContainerNotInitializedException exception. But the current code checks ::hasService() because that is faster than going through exception handling and handles any possible case of the service not being found (no container, no service, etc).

4) Yep, should return [] if not found, although this raises a bunch of other issues where callers to getProfileInfo() are not handling that error case and are referencing keys in the returned array that won't exist whether it is null or [].

5) yes.

6) I think the idea here was to not instantiate the ExtensionDiscovery object until/unless it is needed. But that really shouldn't be a big performance issue or anything, so this can probably be done in the constructor.

7 & 8) sure.

9) Yep

10) sure

11) Yep, that check was probably left over from a refactor. Not needed.

12) Yes

13) Even better...it's checking to see if $dependent is in the array_keys($profiles)...why doesn't it just check for isset($profiles[$dependent]) which would be even faster (getting rid of the slow in_array also).

balsama’s picture

I think @phenaproxima addressed his own feedback in the previous patch. This patch just adds an isset before trying to diff the excluded_dependencies and excluded_themes arrays so the install screen doesn't get plastered with warnings.

Status: Needs review » Needs work

The last submitted patch, 276: 1356276-276-8.2.x.patch, failed testing.

balsama’s picture

Status: Needs work » Needs review

Not sure why testbot set this to NW. Trying again.

The last submitted patch, 262: 1356276-262.patch, failed testing.

frob’s picture

Status: Needs review » Needs work

@balsama because the patch failed testing.