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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sven.lauer’s picture

Status: Active » Needs review
FileSize
1.19 KB

The attached patch just rips out the obsolete function.

Devin Carlson’s picture

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.

LSU_JBob’s picture

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

Devin Carlson’s picture

Status: Needs work » Needs review
Devin Carlson’s picture

Status: Needs review » Reviewed & tested by the community

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

sven.lauer’s picture

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

Dries’s picture

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

sven.lauer’s picture

Component: system.module » documentation
Status: Patch (to be ported) » Needs review
Issue tags: -Needs backport to D7
FileSize
706 bytes

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.

jhodgdon’s picture

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?

sven.lauer’s picture

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.

sven.lauer’s picture

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.

sven.lauer’s picture

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.

jhodgdon’s picture

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

webchick’s picture

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.

Albert Volkman’s picture

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.

Albert Volkman’s picture

Status: Needs work » Needs review
jhodgdon’s picture

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.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
435 bytes

Lazy Sunday afternoon patch.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

That looks like good wording for Drupal 6. Thanks!

Gábor Hojtsy’s picture

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?

jhodgdon’s picture

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

Albert Volkman’s picture

So there would be no change then?

jhodgdon’s picture

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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
450 bytes

Gotcha. Something like this?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

That looks fine. Thanks!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks.

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

Anonymous’s picture

Issue summary: View changes

Fixed path