Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Good catch!

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Actually, can you fix this in http://api.drupal.org/api/function/drupal_get_filename/7 as well?

hefox’s picture

Patch applies, but as Jhodgdon mentioned, missing drupal_get_filename, and possibly also missing in system_schema. However, I'm not 100% sure profile data is stored in system, but it seems likely.

Patch attached includes the above along with the two mentioned areas.

hefox’s picture

Status: Needs work » Needs review

Marking as needs review

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Assuming the test bot is happy with the patch, I'm happy with it as well.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice catch. Committed to HEAD!

Status: Fixed » Closed (fixed)

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

grendzy’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Needs review
FileSize
1.67 KB

D6 backport.
I did not include the change to system_schema, because at least in D6 I can't find any evidence that there is ever 'type' = 'profile' in the system table. (for that matter I couldn't get a 'theme_engine' entry in {system} either).

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This patch still applies and looks fine to me. Sorry for the delay in reviewing, must have gotten lost in the queue...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 716348_drupal_get_path.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Fixed

Committed. Drupal 6 does not store profiles in the database (it does not load them after installation), so that's fine. Thanks!

Status: Fixed » Closed (fixed)

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

mikey_p’s picture

Version: 6.x-dev » 8.x-dev
Priority: Minor » Major
Status: Closed (fixed) » Needs review
Issue tags: +Needs backport to D7
FileSize
1.5 KB

This needs to be reverted as $type = 'profile' does not work any longer since we now treat profiles as modules.

mikey_p’s picture

Title: document drupal_get_path('profile', ...) » Revert documentation for drupal_get_path and drupal_get_filename for profiles

Changing issue title

jhodgdon’s picture

Status: Needs review » Postponed (maintainer needs more info)

I'm looking at the code for drupal_get_filename, and it looks like as a fallback it looks in directory $type . 's' to see if the theme/module/profile/whatever exists. So a profile would need to have $type = 'profile' for that to work. The function code might need to be changed...

Also, system_schema() still says in the description that 'type' can be 'profile'. So are you sure that 'profile' is not being put in there? Where's the function that puts entries in?

mikey_p’s picture

I'm not sure where this is inserted into the system table, but at least for all dozen or so Drupal 7 sites that I have locally, the profile was listed in the system table as a type='module', and there were no entries for type='profile' at all. I can roll a change to the schema docs into this as well. I wish I could pinpoint where this is getting inserted into the system table but I really have no idea (seems like it could be rather difficult to find as well).

mikey_p’s picture

It looks like the profile is added to the list of modules to be installed at: http://api.drupal.org/api/drupal/includes--install.core.inc/function/ins...

This is further corroborated by the snippet in install_finished() that sets the weight of the profile based on type='module': http://api.drupal.org/api/drupal/includes--install.core.inc/function/ins...

I'm open to fixing this function as needed if we'd like to keep that functionality, but for now I'm most interested in getting a patch in as quick as possible, so that this can be fixed in D7 since right now the docs on api.drupal.org are wrong (and I suspect this is a fairly frequently used function). Would getting this docs fix in first, backporting it to D7, and then trying to add the functionality back in with a later patch be a good approach here?

jhodgdon’s picture

Status: Postponed (maintainer needs more info) » Active

Good sleuthing!

The problem with the doc change you have proposed is that it isn't really accurate. Given the way the functions behave currently, in some cases you would need to pass $type='profile' to drupal_get_filename() in order for it to find the profile (some of the code is looking at file paths, and profiles are definitely in the 'profiles' directory and not in 'modules'). But then as you've pointed out and corroborated, if the profile you are looking for is the active one used to install the site, it will be in the system table as a 'module', so you would need to pass $type='module' to find it.

There is all kinds of wonky code already in that function to handle looking for various things in various places with various file extensions based on $type. IMO the function is currently broken for profiles (there is no one value of $type you can pass to make it work in all cases for a profile), so IMO the proper fix is to put another wonky bit of code in there so that if $type == 'profile' and it is looking in the system table, it looks for table field 'type' having value 'module'.

I think it's a bug in d7 and if we get the fix into d8 first, we should be able to get it into d7, because it's a legitimate bug that needs fixing.

Then the function doc would be correct as it currently is, but we also do need to fix the {system} table's 'type' field description so it doesn't mention 'profile' as a possible value.

mikey_p’s picture

Title: Revert documentation for drupal_get_path and drupal_get_filename for profiles » Update system_schema() description for {system}.type
FileSize
559 bytes

I decided to open #1136820: Allow drupal_get_filename() to work with the installed profile. since this isn't really a documentation issue anymore. Attached is a patch for system_schema().

mikey_p’s picture

Status: Active » Needs review
jhodgdon’s picture

Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community

That looks like the right change to system_schema(). Do we need an update function? I guess not -- I don't think we generally make update functions for field descriptions.

So this would be good to get into d8 and d7. I don't think it's "major" any more since the issue scope has decreased considerably.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

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