Install profiles not working with CCK
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
drupal_get_filename returns a different set of results (or rather returns results or doesn't return results) depending on the case that it falls into.
Before the db is active, if a call to drupal_get_path exists, elements falling into the else clause return results _iff_ it is a module and only in situations more apt for Drupal 4 than Drupal 6:
sites/{sitename}/modules/{modulename}.module
sites/{sitename}/modules/{modulename}/{modulename}.module
modules/{modulename}.module
modules/{modulename}/{modulename}.module
Since most modules now go into sites/all/modules rather than sites/{sitename} or /modules, (and since modules are placed in folders inside of the modules directory instead of placed in the modules directory parent) it seems to make sense to modify this logic.
The primary fault (and the way to reproduce this error) is when using a profile to install modules that have a call to drupal_get_path in the install files. CCK in particular has a call to drupal_get_path at the beginning of most of the .install files and therefore cannot install correctly via a profile.
Since both modules and themes support .info files at this point, I chose to search for {filename}.info so that it would correctly find both modules & themes (mirroring what you would find in the system table that would be hit if the db were active). This modifies the return of the else case in many situations, but I believe it is more accurate not less accurate.
Thoughts?
| Attachment | Size |
|---|---|
| bootstap.inc-get_filename-patch.txt | 1.02 KB |
#1
The patch looks good, and removes some code duplication in drupal_get_filename() by calling drupal_system_listing() instead of doing the check for the possible locations directly. I haven't tested this fix in a situation where drupal_get_filename() is called before the database is set up, but in all other cases the patched function behaves just like the original.
I didn't find any existing tests for this function, having some would be very helpful in this case.
Moving to 7.x, and we can backport this to 6.x later.
#2
By using drupal_system_listing which is in common.inc, you've made this function dependant on that include, and therefore if we don't have a database active, unusable until
DRUPAL_BOOTSTRAP_FULL.I'm going to set as code needs work because either you need to run a function_exists to make sure you can call drupal_system_listing or you need to provide a further fallback in drupal_get_filename as well, if we're before
DRUPAL_BOOTSTRAP_FULL.#3
I thought the same thing, but I can't find a situation where this function is called without common.inc being included. During the install process, common.inc is included, and once things are up and running, you're doing a full bootstrap, so common.inc is included. If you can give me an example of a case that I should protect against, that'd be supremely helpful. I, of course, can check for the function, but if the case never fails then it's sort of unwarranted overhead.
Also, the current system fails completely when it has to drop to this case for drupal_get_path, and this at least provides some functionality for that case that wasn't covered before. I want to cover the full scope of cases, but if we look at this as an incremental step, it seems like movement in the correct direction. That being said, if there are use-cases where this function gets called without common.inc being included, I would love to cover those cases.
#4
Changing title to reflect the real problem. From the original issue:
If install profiles cannot use CCK, that's a big limitation on their use cases, so I'm also bumping this to critical.
#5
Tested on a 7.x-dev, fully updated, and no existing database. I have fresh head checkouts of Views, CCK, and Admin_menu in my sites/all/contrib/ directory because they're required by the install profile I'm testing with.
Without the patch, I went to use an install profile, and got this error, proving that the installation bug in http://drupal.org/node/210752 hasn't been fixed yet.
user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY fit DESC LIMIT 0, 1' at line 1 query: SELECT * FROM menu_router WHERE path IN () ORDER BY fit DESC LIMIT 0, 1 in /www/drupalhead/includes/menu.inc on line 315.I can't proceed with this test until I get that thing sorted out, and when I do, I'll be sure to post some more details on the afore mentioned thread 210752.
#6
I accidentally changed the status. Setting back to critical.
#7
#8
@csevb10 Okay, well we're fixing a serious bug here, and are worrying about some serious edge cases. Can you reroll a patch so that drupal_get_filename will first check for the database, then if that fails, do a function_exists for drupal_system_listing, and use that but, but if that fails, then it falls back to the old way, but add some more paths too.
That should cover most of the edge cases no? And still provide the updated functionality. The function is already built using failover, so it's more of an extension to the current functionality anyway.
#9
Something like this.
#10
That's a fair point. 'tis core code and all. :-P
Here's a slightly tighter version, I think, a little less code duplication.
#11
Patch still applies.
#12
The last submitted patch failed testing.
#13
Using 6.12, this problem is not fixed yet, waiting for a patch please.
#14
Chasing HEAD. Patch looks good.
This patch could use a test or two, but for what?
A) Check to see if the $files[$type][$name] array is populated with recognizable strings?
B) See if function_exists('drupal_system_listing') is coming back TRUE so we don't accidentally add things that break core later? (since there's no calls that fail right now)
C) ...
#15
Lets see if the TestBot likes it...
#16
The last submitted patch failed testing.
#17
Odd, HEAD installs perfectly for me with this patch applied. Don't know what the problem is.
#18
The last submitted patch failed testing.
#19
Ahh, the install.php is failing due to a
Warning: preg_match() [/www/manual_php/function.preg-match.html]: Delimiter must not be alphanumeric or backslash in /www/drupalhead/includes/file.inc on line 1652error.