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.
Comment | File | Size | Author |
---|---|---|---|
#30 | system-get-info-followup-561452-30.patch | 1.66 KB | David_Rothstein |
#23 | drupal.module-theme-rebuild.patch | 14.8 KB | sun |
#16 | get_info-561452-16.patch | 13.66 KB | David_Rothstein |
#11 | get_info-561452-11.patch | 14.4 KB | David_Rothstein |
#9 | get_info-561452-9.patch | 12.13 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedComment #2
ksenzeeI 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.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedYes, 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...
Comment #4
pwolanin CreditAttribution: pwolanin commentedWe probably could use it in core - for example on the by-module page. There's no reason we have to build fresh there.
Comment #5
pwolanin CreditAttribution: pwolanin commentedupdated patch per suggestions above with conversion of the by-modules page to use the new API function.
Comment #6
pwolanin CreditAttribution: pwolanin commentedminor whitespace fix.
Comment #7
ksenzeeLine 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.
Comment #8
pwolanin CreditAttribution: pwolanin commentedok, wrapped doxygen
Comment #9
pwolanin CreditAttribution: pwolanin commentedoops - ignore that patch - diffed w/out a recent core commit merged in
Comment #10
ksenzeeLooks good.
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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?
Comment #12
pwolanin CreditAttribution: pwolanin commentedAh, I see what you mean about the static not being reset that way. Without it is fine too.
Comment #13
pwolanin CreditAttribution: pwolanin commentedI 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.
Comment #14
pwolanin CreditAttribution: pwolanin commentedtagging
Comment #15
Dries CreditAttribution: Dries commentedI 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.
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a version that is updated to chase HEAD and also remove the $rebuild parameter.
Should be RTBC if the tests pass.
Comment #17
ksenzeeWell, 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.Comment #19
ksenzeeNo time for a reroll atm, but tagging.
Comment #20
sunVery nice patch! This might kill some performance issues in the queue.
Why not
system_get_info($type)
and
system_rebuild_info($type)
?
This review is powered by Dreditor.
Comment #21
catchSun 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.
Comment #22
sunTagging 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.
Comment #23
sunI now understand why the proposal I gave doesn't make sense - because those two rebuilding functions are totally different.
So back to RTBC.
Comment #24
webchickSeems sensible. Committed to HEAD.
Needs documenting in the upgrade guide.
Comment #25
sun.
Comment #26
alexanderpas CreditAttribution: alexanderpas commentedSeems like i'm unable to get information about the dependencies trough this new function.
Comment #27
andypost@alexanderpas try my.php
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, )
Comment #28
alexanderpas CreditAttribution: alexanderpas commentedyes, that way it worked, however, the other way, it didn't work.
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.
Comment #29
andypostHm, 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
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commentedFollowing 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?
Comment #31
andypostTested, suppose it brings a little performance to permissions page
Comment #32
webchickCommitted to HEAD! Thanks!
Comment #33
sunWhat 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?
Comment #35
marcvangendMaybe 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?