Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
drupal_get_path('profile', ...) works, but you wouldn't know it from the docs.
Comment | File | Size | Author |
---|---|---|---|
#19 | 716348-system-schema-comment-19.patch | 559 bytes | mikey_p |
#13 | 0001-Issue-716348-by-mikey_p-Revert-documentation-on-drup.patch | 1.5 KB | mikey_p |
#8 | 716348_drupal_get_path.patch | 1.67 KB | grendzy |
#3 | 716348_profile_documentation.patch | 1.71 KB | hefox |
drupal_get_path_profile.patch | 674 bytes | grendzy | |
Comments
Comment #1
jhodgdonGood catch!
Comment #2
jhodgdonActually, can you fix this in http://api.drupal.org/api/function/drupal_get_filename/7 as well?
Comment #3
hefox CreditAttribution: hefox commentedPatch 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.
Comment #4
hefox CreditAttribution: hefox commentedMarking as needs review
Comment #5
jhodgdonAssuming the test bot is happy with the patch, I'm happy with it as well.
Comment #6
webchickNice catch. Committed to HEAD!
Comment #8
grendzy CreditAttribution: grendzy commentedD6 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).
Comment #9
jhodgdonThis patch still applies and looks fine to me. Sorry for the delay in reviewing, must have gotten lost in the queue...
Comment #11
Gábor HojtsyCommitted. Drupal 6 does not store profiles in the database (it does not load them after installation), so that's fine. Thanks!
Comment #13
mikey_p CreditAttribution: mikey_p commentedThis needs to be reverted as $type = 'profile' does not work any longer since we now treat profiles as modules.
Comment #14
mikey_p CreditAttribution: mikey_p commentedChanging issue title
Comment #15
jhodgdonI'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?
Comment #16
mikey_p CreditAttribution: mikey_p commentedI'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).
Comment #17
mikey_p CreditAttribution: mikey_p commentedIt 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?
Comment #18
jhodgdonGood 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.
Comment #19
mikey_p CreditAttribution: mikey_p commentedI 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().
Comment #20
mikey_p CreditAttribution: mikey_p commentedComment #21
jhodgdonThat 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.
Comment #22
Dries CreditAttribution: Dries commentedCommitted to 7.x and 8.x. Thanks!