This patch is being submitted as a result of http://drupal.org/node/116488#comment-192167 where Dries asked me to take a look. I've broken the original API into four helper functions and changed the API to be a wrapper to the helper functions. This change leaves the original API intact; however a review of the use of the API is needed as is pointed out in the original comment in order to consider the preservation of the static list and sorted_list with regard to the use of the boolean refresh argument. My design helps reduce the need to refresh the static $list and $sorted_list from the DB because the helper functions create a ``type of list'' and prevents the need to refresh the list because the required ``type of list'' changes. My design also self determines an empty list and will refresh from the DB if the list is empty. This means that the refresh argument should usually be false and should only be specified if the DB changes.

CommentFileSizeAuthor
#24 module_list_resolution.patch_5.txt15.9 KBAnonymous (not verified)
#19 module_list_resolution.patch_4.txt18.84 KBAnonymous (not verified)
#18 module_list_resolution.patch_3.txt19.45 KBAnonymous (not verified)
#17 module_list_resolution.patch_2.txt20.5 KBAnonymous (not verified)
#14 module_list_resolution.patch_1.txt18.28 KBAnonymous (not verified)
#10 module_list_resolution.patch_0.txt18.65 KBAnonymous (not verified)
#8 module_list_resolution.patch.txt19.32 KBAnonymous (not verified)
#5 module_list.patch_1.txt3.9 KBAnonymous (not verified)
#1 module_list.patch_0.txt3.89 KBAnonymous (not verified)
module_list.patch.txt3.88 KBAnonymous (not verified)

Comments

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new module_list.patch_0.txt3.89 KB

Typo fix. Resubmitting patch file.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Code-style issues: we don't put a space before the function name and the (

This seems like a good change, though, in my .03 second review of the code. ;) Will try to look more later on.

Anonymous’s picture

Anonymous (not verified) commented

That is a habit difficult to break. Perhaps someone has a Drupal Lint filter or Code Pretty filter I could use; I'll search for one.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

There's code-style.pl in the script directory, and also the "Coder" module will also tell you (but core still has code standards violations in places so it's difficult to tell yours apart from theirs, and also it's not compatible with HEAD yet.)

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new module_list.patch_1.txt3.9 KB

I took care of the space issue and found some tabs to convert.

dries’s picture

dries commented

Great start! The next logical step would be to:

1. Remove the wrapping function.
2. To directly call the proper module_list_xxx() function from the code.
3. To add PHPDoc to each of the module_list_xxX() functions.
4. To double-check why we wouldn't always want to sort the module. If we always sort the modules, we can remove the parameter from the function definition.

I think.

Anonymous’s picture

Anonymous (not verified) commented

In looking further into what Dries has suggested it is obvious that every module uses module_list. So to protect the innocent and to force the module developer to use the new methods I am renaming my _module_list to module_list_all. This brings me to install.php which is the only source I've found to use the fixed list feature. Can someone explain why the fixed list call at line 45 of install.php is needed? What benefit does it have? I'm not grasping that it has any benefit and can be removed. If I'm correct then the module_list_fixed can be eliminated.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new module_list_resolution.patch.txt19.32 KB

Quiz: How many modules listed in the system table have the bootstrap flag set?

The way module_list is currently coded no one knew that the selection where bootstrap = 1 was returning zero rows.

I renamed module_list to module_list_all to force every module user to need to take a look because of the way the API was coded. If we leave it as module_list, nasty surprises could be in store.

New patch attached encompassing the changes for the core modules and files.

kkaefer’s picture

kkaefer commented
Status: Needs review » Needs work

Please don't include your personal database access information.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new module_list_resolution.patch_0.txt18.65 KB

Removed.

douggreen’s picture

douggreen commented

Hmm, I thought coder was compatible with HEAD. A few weeks ago, I added support for the new menu system, and fixed some E_ALL problems. If it's not, I'd be happy to fix if someone (webchick) let's me know where it's not working.

Anonymous’s picture

Anonymous (not verified) commented

@douggreen: Cool module but what does your comment have to do with my patch?

dries’s picture

dries commented

This patch contains some erroneous changes -- it changes the MySQL version, for example.

With this patch, we always load all modules? I don't see where we choose to load the bootstrap modules only (i.e. when serving a cached page). Am I missing something, or is this no longer necessary?

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work
StatusFileSize
new module_list_resolution.patch_1.txt18.28 KB

Sorry about the MySQL version bit. I had dreams about it last night and had removed it from the patch before reading your post.

When testing the implementation of the module_list_bootstrap doing a fresh install many times while testing I discovered that the bootstrap value is 0 in the DB because during the install process the menu system build didn't complete. I had assumed the bootstrap column to mean a small set of modules needed to bootstrap the system and not a value that is changed based on caching needs. Karoly has pointed me to some code to look through that sets bootstrap during caching and I will need to take a look at that.

The current module_list implementation hides the fact that the bootstrap select returns zero rows. The default value for the bootstrap parameter is TRUE but that means little since the refresh value default is FALSE. So when a module did something like $module_list = module_list(); it received what ever the static $list contained at the time which was most likely the last full list pull.

With my module_list_all implementation the refresh of modules should only happen when the status column changes or a new row is added. I will look at the code that Karoly pointed me to and determine if it should still be setting this flag. The caching code will be faster if it has fewer rows to update. If bootstrap value should be set at caching it should be set for no-caching as well so I could be adding module_list_bootstrap back in but with other changes to set the value during the system.install.

The attached file contains the change to remove the erroneous update of MySQL Version.

Anonymous’s picture

Anonymous (not verified) commented

On a fresh install, bootstrap column for statistics.module and throttle.module should be set but isn't. The reason is because the module_rebuild_cache, called from drupal_install_profile, retrieves a list of files from the system table calling system_get_files_database but the system table hasn't been fully populated at the time the module_rebuild_cache is called. There are multiple occurrences of db_query("INSERT INTO {system} ... and IMO that needs to be consolidated to one function that DTRT with bootstrap from the start. I will recode my patch for module_list to do the bootstrap thing but I don't have time to spend on the underlying issue brought forth by this research.

dries’s picture

dries commented

Thanks for the additional research Earnie. Much appreciated.

module_rebuild_cache() looks weird to me -- it's not quite clear what system_get_files_database() does. It's not clear why a filename should suddenly change. There is also a TODO in the code that suggests we can cleanup module_rebuild_cache() a bit. Looks like there might be quite a bit of room for improvement here. :/

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new module_list_resolution.patch_2.txt20.5 KB

Attached is the rework for adding the module_list_bootstrap API of this function.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new module_list_resolution.patch_3.txt19.45 KB

I'm an idiot. I had the file open making changes and didn't save them before uploading.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new module_list_resolution.patch_4.txt18.84 KB

I am refreshing the patch for the additional use of module_list.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

- What happed to the fixed list in install.php? You removed that but have not commented on why it was possible.
- $sort seems to be a confusing parameter name to me, as either you pass TRUE or FALSE, the result will be sorted, but will be sorted differently... maybe name that parameter $byname or something

Anonymous’s picture

Anonymous (not verified) commented

Install.php was the only place I could find that used it and my testing proves that it isn't needed; therefore I removed it considering it to be clutter code. The $sort name was taken from the existing module_list function; I do not see the need to change it.

keith.smith’s picture

keith.smith commented
Status: Needs review » Needs work

Patch no longer applies cleanly.

# patch -p0 < module_list_resolution.patch_4.txt
patching file install.php
Hunk #1 succeeded at 58 (offset 18 lines).
patching file update.php
Hunk #1 succeeded at 332 (offset -11 lines).
patching file includes/bootstrap.inc
Hunk #1 succeeded at 501 (offset 31 lines).
patching file includes/install.inc
patching file includes/menu.inc
Hunk #1 succeeded at 851 with fuzz 2 (offset 323 lines).
patching file includes/module.inc
Hunk #2 FAILED at 19.
Hunk #4 succeeded at 208 (offset -37 lines).
Hunk #5 succeeded at 315 (offset 33 lines).
Hunk #6 succeeded at 275 (offset -37 lines).
Hunk #7 succeeded at 413 (offset 33 lines).
1 out of 7 hunks FAILED -- saving rejects to file includes/module.inc.rej
patching file modules/block/block.module
Hunk #1 succeeded at 165 (offset 12 lines).
patching file modules/filter/filter.module
Hunk #1 succeeded at 630 (offset -4 lines).
patching file modules/search/search.module
Hunk #1 succeeded at 226 (offset 21 lines).
Hunk #2 succeeded at 310 (offset 7 lines).
patching file modules/system/system.install
patching file modules/system/system.module
Hunk #1 succeeded at 1751 (offset 264 lines).
Hunk #2 FAILED at 1768.
1 out of 2 hunks FAILED -- saving rejects to file modules/system/system.module.rej
patching file modules/user/user.module
Hunk #2 FAILED at 965.
Hunk #3 FAILED at 1612.
Hunk #4 succeeded at 2002 (offset 90 lines).
Hunk #5 FAILED at 2588.
Hunk #6 succeeded at 2714 (offset 191 lines).
Hunk #7 succeeded at 2635 (offset 90 lines).
Hunk #8 succeeded at 2780 (offset 191 lines).
3 out of 8 hunks FAILED -- saving rejects to file modules/user/user.module.rej

Anonymous’s picture

Anonymous (not verified) commented

I'm currently on vacation and the development isn't available. I'll update the patch after I return on July 2nd.

Earnie

Anonymous’s picture

Anonymous (not verified) commented
Version: 6.x-dev » 7.x-dev
Status: Needs work » Needs review
StatusFileSize
new module_list_resolution.patch_5.txt15.9 KB

Updated patch to HEAD revision. Patch should apply cleanly now.

birdmanx35’s picture

birdmanx35 commented

Patch no longer applies cleanly:

$ patch -p0 < module_list_resolution.patch
patching file update.php
Hunk #1 FAILED at 332.
1 out of 1 hunk FAILED -- saving rejects to file update.php.rej
patching file includes/bootstrap.inc
Hunk #1 succeeded at 518 with fuzz 1 (offset 17 lines).
patching file includes/install.inc
Hunk #1 FAILED at 22.
1 out of 1 hunk FAILED -- saving rejects to file includes/install.inc.rej
patching file includes/menu.inc
Hunk #1 succeeded at 1100 (offset 245 lines).
patching file includes/module.inc
Hunk #4 succeeded at 257 (offset 44 lines).
Hunk #5 succeeded at 301 (offset 44 lines).
Hunk #6 succeeded at 327 (offset 44 lines).
Hunk #7 succeeded at 369 (offset 56 lines).
Hunk #8 succeeded at 443 (offset 62 lines).
patching file modules/block/block.module
Hunk #1 succeeded at 236 (offset 71 lines).
patching file modules/filter/filter.module
Hunk #1 succeeded at 325 (offset -302 lines).
patching file modules/search/search.module
Hunk #1 FAILED at 226.
Hunk #2 succeeded at 268 (offset -58 lines).
1 out of 2 hunks FAILED -- saving rejects to file modules/search/search.module.rej
patching file modules/system/system.install
Hunk #1 succeeded at 255 (offset 103 lines).
patching file modules/system/system.module
Hunk #1 FAILED at 1819.
Hunk #2 FAILED at 1836.
Hunk #3 FAILED at 3121.
3 out of 3 hunks FAILED -- saving rejects to file modules/system/system.module.rej
patching file modules/user/user.module
Hunk #2 FAILED at 2023.
Hunk #3 succeeded at 1868 (offset -956 lines).
Hunk #4 succeeded at 2400 (offset -446 lines).
Hunk #5 FAILED at 2444.
2 out of 5 hunks FAILED -- saving rejects to file modules/user/user.module.rej

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

@birdmanx35: After six months, you must be joking! ;t Feel free to submit an update.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Closed (duplicate)

See #222109: module_list() should be rewritten as 2 functions: module_list() and module_list_bootstrap()