Add API function to get module/theme info without rebuilding it.
pwolanin - August 27, 2009 - 19:29
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | system.module |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | API addition, API change, API clean-up, D7 API clean-up |
Description
In D6/D7 it's impossible to ask system module to just give you a nice copy of the data from the {system} table - for example for building a form with a list of module names or descriptions.
This function http://api.drupal.org/api/function/system_get_module_data/7 always rebuilds evertyhing from scratch - which is appropriate for the system_modules page, but not everywhere.
#1
#2
I wonder if it makes sense to add a $rebuild parameter, which if it were TRUE would call system_get_[module|theme]_data for you. That would make this a nice clean entry point for either cached or rebuilt data. Either way, it's a bit of a WTF to have no API for running the db query, so this seems like a good addition.
#3
Yes, this looks like it would be useful - although it's interesting that nowhere in core would actually use it right now :) Although I can immediately think of one followup patch that would make use of it.
I think maybe the PHP docblock could use a little work - it's a little unclear from reading it what "get the .info data" and "module or theme information" actually mean. I kind of want to see a bit more explanation of what it is this function actually returns. But otherwise this seems like it should go in.
A further useful followup, besides what Katherine mentioned, might be to rename the existing system_get_module_data() function to something like system_rebuild_module_data() - and similar for themes...
#4
We probably could use it in core - for example on the by-module page. There's no reason we have to build fresh there.
#5
updated patch per suggestions above with conversion of the by-modules page to use the new API function.
#6
minor whitespace fix.
#7
Line 202 of the patch is a docblock line in hook_system_info_alter() that goes past 80 characters (to be fair, it went past 80 chars before, but now it's really sticking its neck out there). Otherwise RTBC.
#8
ok, wrapped doxygen
#9
oops - ignore that patch - diffed w/out a recent core commit merged in
#10
Looks good.
#11
The latest patch used a static variable, but the API does not automatically reset it - for example, if system_rebuild_module_data() is called, that should reset the static variable in system_get_info().
I actually prefer the original version (without the static). It doesn't seem like this is a function that will get called that often, and even then, usually on pages where micro-optimization is not that important. It's also only one database query, so not that big of a deal.
The attached patch removes the static, and also replaces call_user_func() with $function() syntax, which is usually cleaner. I also added to the code comments for system_get_info() a bit.
Everything else looks fine to me... ready to go in now?
#12
Ah, I see what you mean about the static not being reset that way. Without it is fine too.
#13
I used call_user_func() since there were no parameters and it made the code more compact. Either that or the variable function name are the same to me.
#14
tagging
#15
I think system_get_info() would be cleaner if we dropped the $rebuild parameter. We don't really need it because there is a separate API for it.
#16
Here's a version that is updated to chase HEAD and also remove the $rebuild parameter.
Should be RTBC if the tests pass.
#17
Well, the point of the $rebuild parameter was to remove the API inconsistency between
system_get_info('module')andsystem_rebuild_module_data(). Ideally they would both take a parameter, or neither one would. But that is a minor nitpicky detail, and this is RTBC anyway.#18
The last submitted patch failed testing.
#19
No time for a reroll atm, but tagging.
#20
Very nice patch! This might kill some performance issues in the queue.
+++ modules/system/system.module 1 Sep 2009 04:10:26 -0000@@ -1852,12 +1852,35 @@ function system_update_files_database(&$
+ * @see system_rebuild_module_data()
+ * @see system_rebuild_theme_data()
+ */
+function system_get_info($type) {
Why not
system_get_info($type)
and
system_rebuild_info($type)
?
This review is powered by Dreditor.
#21
Sun just pointed me here from #591734: Reduce system table queries in the critical path. Due to the lack of an API function, we currently query the system table 3-4 times per request to get very similar information. I'm not sure this patch would do what that one needs in terms of the array structure (see comments over there), since it's trying to solve a different problem, but subscribing here anyway.
#22
Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.
#23
I now understand why the proposal I gave doesn't make sense - because those two rebuilding functions are totally different.
So back to RTBC.
#24
Seems sensible. Committed to HEAD.
Needs documenting in the upgrade guide.
#25
.
#26
Seems like i'm unable to get information about the dependencies trough this new function.
#27
@alexanderpas try my.php
<?php
define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
$info = system_get_info('module');
var_export($info['dashboard']);
?>
array ( 'name' => 'Dashboard', 'description' => 'A module that provides a dashboard interface for organizing and tracking various information within your site.', 'core' => '7.x', 'package' => 'Core', 'version' => '7.0-dev', 'files' => array ( 0 => 'dashboard.module', ), 'dependencies' => array ( 0 => 'block', ), 'dependents' => array ( ), 'php' => '5.2.0', 'bootstrap' => 0, )#28
yes, that way it worked, however, the other way, it didn't work.
<?php
define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
$info = system_get_info('module');
var_export($info['dashboard']);
var_export($info['block']);
?>
array ( 'name' => 'Dashboard', 'description' => 'A module that provides a dashboard interface for organizing and tracking various information within your site.', 'core' => '7.x', 'package' => 'Core', 'version' => '7.0-dev', 'files' => array ( 0 => 'dashboard.module', ), 'dependencies' => array ( 0 => 'block', ), 'dependents' => array ( ), 'php' => '5.2.0', 'bootstrap' => 0, )array ( 'name' => 'Block', 'description' => 'Controls the visual building blocks a page is constructed with. Blocks are boxes of content rendered into an area, or region, of a web page.', 'package' => 'Core', 'version' => '7.0-dev', 'core' => '7.x', 'files' => array ( 0 => 'block.module', 1 => 'block.admin.inc', 2 => 'block.install', 3 => 'block.test', ),'dependencies' => array ( ), 'dependents' => array ( ),'php' => '5.2.0', 'bootstrap' => 0, )block doesn't show dashboard depends on it.
#29
Hm, I found no info in DB, suppose this information is not stored in DB but could be a nice feature.
This needs one _for loop_ before saving or after reading DB because stored info possibly could be different from real state
#30
Following up on this, I've updated the existing documentation at http://drupal.org/update/modules/6/7#system_rebuild_module_data (since these functions were already renamed once before during the D7 cycle).
Also, here's a followup patch that gets one stray reference to _system_get_theme_data() that I found in the codebase, and also converts another case (on the permission page) where we were previously scanning the filesystem unnecessarily but now can use the new API function instead -- that one had been annoying me for a while :)
Regarding #26-#29, that sounds like a separate issue -- the function introduced here just takes whatever it finds in the database, so if the dependency information isn't correctly making it into the database, sounds like you should file that as a separate bug report?
#31
Tested, suppose it brings a little performance to permissions page
#32
Committed to HEAD! Thanks!
#33
+++ modules/system/system.module 13 Oct 2009 04:26:37 -0000@@ -1847,12 +1847,35 @@ function system_update_files_database(&$
+function system_get_info($type) {
+ $info = array();
+ $result = db_query('SELECT name, info FROM {system} WHERE type = :type AND status = 1', array(':type' => $type));
What bugs me a bit is that I still need to invoke the rebuilding functions in case I want to do something with all available modules/themes (not only enabled). I have several use-cases for that and at least two contrib modules (of mine) that would highly appreciate a way (perhaps via an optional second argument $include_disabled = FALSE?) to retrieve that info without rebuilding it from scratch.
I'm on crack. Are you, too?
#34
Automatically closed -- issue fixed for 2 weeks with no activity.