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.

CommentFileSizeAuthor
#30 system-get-info-followup-561452-30.patch1.66 KBDavid_Rothstein
Passed: 14482 passes, 0 fails, 0 exceptions View
#23 drupal.module-theme-rebuild.patch14.8 KBsun
Failed: Failed to apply patch. View
#16 get_info-561452-16.patch13.66 KBDavid_Rothstein
Failed: Failed to apply patch. View
#11 get_info-561452-11.patch14.4 KBDavid_Rothstein
Failed: Failed to apply patch. View
#9 get_info-561452-9.patch12.13 KBpwolanin
Failed: Failed to apply patch. View
#8 get_info-561452-8.patch33.7 KBpwolanin
Failed: Failed to apply patch. View
#6 get_info-561452-6.patch11.88 KBpwolanin
Failed: Failed to apply patch. View
#5 get_info-561452-5.patch11.88 KBpwolanin
Failed: Failed to apply patch. View
#1 get_info-561452-1.patch1017 bytespwolanin
Failed: Failed to install HEAD. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
1017 bytes
Failed: Failed to install HEAD. View
ksenzee’s picture

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.

David_Rothstein’s picture

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...

pwolanin’s picture

We probably could use it in core - for example on the by-module page. There's no reason we have to build fresh there.

pwolanin’s picture

FileSize
11.88 KB
Failed: Failed to apply patch. View

updated patch per suggestions above with conversion of the by-modules page to use the new API function.

pwolanin’s picture

FileSize
11.88 KB
Failed: Failed to apply patch. View

minor whitespace fix.

ksenzee’s picture

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.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
33.7 KB
Failed: Failed to apply patch. View

ok, wrapped doxygen

pwolanin’s picture

FileSize
12.13 KB
Failed: Failed to apply patch. View

oops - ignore that patch - diffed w/out a recent core commit merged in

ksenzee’s picture

Looks good.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.4 KB
Failed: Failed to apply patch. View

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?

pwolanin’s picture

Ah, I see what you mean about the static not being reset that way. Without it is fine too.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

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.

pwolanin’s picture

Issue tags: +API change, +API addition

tagging

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
13.66 KB
Failed: Failed to apply patch. View

Here's a version that is updated to chase HEAD and also remove the $rebuild parameter.

Should be RTBC if the tests pass.

ksenzee’s picture

Status: Needs review » Reviewed & tested by the community

Well, the point of the $rebuild parameter was to remove the API inconsistency between system_get_info('module') and system_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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

ksenzee’s picture

Issue tags: +API clean-up

No time for a reroll atm, but tagging.

sun’s picture

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.

catch’s picture

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.

sun’s picture

Category: feature » task
Priority: Normal » Critical
Issue tags: +D7 API clean-up

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.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.8 KB
Failed: Failed to apply patch. View

I now understand why the proposal I gave doesn't make sense - because those two rebuilding functions are totally different.

So back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs Documentation

Seems sensible. Committed to HEAD.

Needs documenting in the upgrade guide.

sun’s picture

Issue tags: -D7 API clean-up

.

alexanderpas’s picture

Issue tags: +D7 API clean-up

Seems like i'm unable to get information about the dependencies trough this new function.

andypost’s picture

@alexanderpas try my.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, )

alexanderpas’s picture

yes, that way it worked, however, the other way, it didn't work.

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.

andypost’s picture

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

David_Rothstein’s picture

Status: Needs work » Needs review
Issue tags: -Needs Documentation
FileSize
1.66 KB
Passed: 14482 passes, 0 fails, 0 exceptions View

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?

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Tested, suppose it brings a little performance to permissions page

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD! Thanks!

sun’s picture

+++ 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?

Status: Fixed » Closed (fixed)

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

marcvangend’s picture

Maybe the people who participated here can shed some light on the patch and questions in #808030-13: New themes are not detected when visiting themes setting page without first clearing the cache manually?