In system.admin.inc, we have

<?php
/**
* 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.

Files: 
CommentFileSizeAuthor
#24 doc_fix_system_settings_overview-1352272-d6-24.patch450 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#18 doc_fix_system_settings_overview-1352272-d6-18.patch435 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#15 doc_fix_system_settings_overview-1352272-d7-15.patch762 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 37,581 pass(es).
[ View ]
#8 doc-fix-system_settings_overview-doc-1352272.patch706 bytessven.lauer
PASSED: [[SimpleTest]]: [MySQL] 36,992 pass(es).
[ View ]
#3 remove-system_settings_overview-1352272.patch782 bytesLSU_JBob
PASSED: [[SimpleTest]]: [MySQL] 34,108 pass(es).
[ View ]
#1 remove-system_settings_overview-1352272.patch1.19 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 34,168 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.19 KB
PASSED: [[SimpleTest]]: [MySQL] 34,168 pass(es).
[ View ]

The attached patch just rips out the obsolete function.

Status:Needs review» Needs work
Issue tags:+Novice

Looks 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:

function system_themes_page() {
@@ -1498,7 +1481,7 @@ function system_site_information_settings() {
     '#options' => drupal_map_assoc(array(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 15, 20, 25, 30)),
     '#description' => t('The maximum number of posts displayed on overview pages such as the front page.'),
     '#access' => (variable_get('site_frontpage')=='node'),
-  );
+  );
   $form['error_page'] = array(
     '#type' => 'fieldset',
     '#title' => t('Error pages'),

Tagging as novice. If the patch is rerolled without the whitespace added to the bracket it should be RTBC.

StatusFileSize
new782 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,108 pass(es).
[ View ]

No whitespace added to bracket and function is just ripped out. picked up from the novice queue.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

The updated patch in #3 applied cleanly and passed all tests.

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

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed to 8.x. Moving to 7.x so we can update the phpDoc fix (but not remove the function itself).

Component:system.module» documentation
Status:Patch (to be ported)» Needs review
Issue tags:-needs backport to D7
StatusFileSize
new706 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,992 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

That 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:

modules/simpletest/tests/upgrade/drupal-6.bare.database.php:  'page_callback' => 'system_settings_overview',
modules/simpletest/tests/upgrade/drupal-6.filled.database.php:  'page_callback' => 'system_settings_overview',

Can someone investigate?

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

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

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

Ah, that makes sense. Definitely RTBC then. :)

Status:Reviewed & tested by the community» Needs work

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

StatusFileSize
new762 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,581 pass(es).
[ View ]

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

Status:Needs work» Needs review

Version:7.x-dev» 6.x-dev
Status:Needs review» Patch (to be ported)

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

Status:Patch (to be ported)» Needs review
StatusFileSize
new435 bytes
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Lazy Sunday afternoon patch.

Status:Needs review» Reviewed & tested by the community

That looks like good wording for Drupal 6. Thanks!

Status:Reviewed & tested by the community» Needs work

In D6 it seems to be a menu callback still:

$ grep -r "system_settings_overview" *
modules/system/system.admin.inc:function system_settings_overview() {
modules/system/system.module:    'page callback' => 'system_settings_overview',

Why are we removing that information from the phpdoc header?

Good point, I guess we should put that back in.

So there would be no change then?

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

Status:Needs work» Needs review
StatusFileSize
new450 bytes
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Gotcha. Something like this?

Status:Needs review» Reviewed & tested by the community

That looks fine. Thanks!

Status:Reviewed & tested by the community» Fixed

Committed, thanks.

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

Issue summary:View changes

Fixed path