In system.admin.inc, we have
/**
* Menu callback; displays a module's settings page.
*/
function system_settings_overview() {
// Check database setup if necessary
if (function_exists('db_check_setup') && empty($_POST)) {
db_check_setup();
}
$item = menu_get_item('admin/config');
$content = system_admin_menu_block($item);
$output = theme('admin_block_content', array('content' => $content));
return $output;
}
The doc header is lying (and has been, at least since D5). Up until D6, this function was used to generate 'admin/settings', but in D7+8, the corresponding page 'admin/config' is actually generated by function system_admin_config_page()---and system_settings_overview() is used nowhere else (in core).
So this function should be removed in D8. And the doc fixed in D7.
Comments
Comment #1
sven.lauer CreditAttribution: sven.lauer commentedThe attached patch just rips out the obsolete function.
Comment #2
Devin Carlson CreditAttribution: Devin Carlson commentedLooks good, doing a text search through core doesn't turn up any results for the function name.
The patch didn't apply though, I don't think that the change made to an unrelated bracket was meant to be included in the patch:
Tagging as novice. If the patch is rerolled without the whitespace added to the bracket it should be RTBC.
Comment #3
LSU_JBob CreditAttribution: LSU_JBob commentedNo whitespace added to bracket and function is just ripped out. picked up from the novice queue.
Comment #4
Devin Carlson CreditAttribution: Devin Carlson commentedComment #5
Devin Carlson CreditAttribution: Devin Carlson commentedThe updated patch in #3 applied cleanly and passed all tests.
Comment #6
sven.lauer CreditAttribution: sven.lauer commentedAdding backport tags, to make sure the doc header gets fixed in D7 and D6. The issue should be moved to the api doc component once the patch is committed.
(We cannot take this out of D7, as this would technically constitute and API change.)
Comment #7
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 7.x so we can update the phpDoc fix (but not remove the function itself).
Comment #8
sven.lauer CreditAttribution: sven.lauer commentedHere is a patch that updates the doc ... not exactly sure whether this is right. The function is a bit awkward to document, as it is unused in D7. Note that, while removing the function completely would technically be a doc change, it is extremely unlikely that anyone uses this function outside core.
Comment #9
jhodgdonThat seems like a reasonable way to document this function...
But it is not quite true that it is used nowhere, or at least it might not be. A grep found:
Can someone investigate?
Comment #10
sven.lauer CreditAttribution: sven.lauer commentedAbout #9: These files are D6 database dumps used for testing the D6->D7 upgrade process. They have system_settings_overview() as the page callback for the main settings page, which is what the function was used for in D6.
Comment #11
sven.lauer CreditAttribution: sven.lauer commentedAbout #9: These files are D6 database dumps used for testing the D6->D7 upgrade process. They have system_settings_overview() as the page callback for the main settings page, which is what the function was used for in D6.
Comment #12
sven.lauer CreditAttribution: sven.lauer commentedAbout #9: These files are D6 database dumps used for testing the D6->D7 upgrade process. They have system_settings_overview() as the page callback for the main settings page, which is what the function was used for in D6.
Comment #13
jhodgdonAh, that makes sense. Definitely RTBC then. :)
Comment #14
webchickI think we should add a @deprecated doxygen property here. Not sure exactly the syntax for it, but grepping reveals we're using that elsewhere in field.module.
Comment #15
Albert Volkman CreditAttribution: Albert Volkman commentedIt appears that the convention is to place @deprecated followed by a @see pointing to the function that its replacing. If so, here's that patch.
Comment #16
Albert Volkman CreditAttribution: Albert Volkman commentedComment #17
jhodgdonThis patch still applied and was fine, so I went ahead and committed it. Thanks!
This issue is marked as backport to D6, because the docs for system_settings_overview() in D6 are totally wrong. Don't backport the current D7 patch exactly though, because the function is not deprecated in D6.
Comment #18
Albert Volkman CreditAttribution: Albert Volkman commentedLazy Sunday afternoon patch.
Comment #19
jhodgdonThat looks like good wording for Drupal 6. Thanks!
Comment #20
Gábor HojtsyIn D6 it seems to be a menu callback still:
Why are we removing that information from the phpdoc header?
Comment #21
jhodgdonGood point, I guess we should put that back in.
Comment #22
Albert Volkman CreditAttribution: Albert Volkman commentedSo there would be no change then?
Comment #23
jhodgdonWe need a change. The current docs for system_settings_overview() says "Menu callback; displays a module's settings page.", which is totally wrong. We just want the new docs to include "Menu callback:" and then the correct information.
Comment #24
Albert Volkman CreditAttribution: Albert Volkman commentedGotcha. Something like this?
Comment #25
jhodgdonThat looks fine. Thanks!
Comment #26
Gábor HojtsyCommitted, thanks.
Comment #27.0
(not verified) CreditAttribution: commentedFixed path