As suggested by Dries in #464792: Notice/warnings on submitting aggregator settings form, the Aggregator settings page is in a non-standard place. It should be under Site configuration rather than Content management. This patch moves it to its new home. This page is going to take on greater importance now due to aggregator's pluggable architecture.

Please look over the text for title and description of the menu item and let me know what you think. Let me know if I missed any other mentions/uses of the old path.

CommentFileSizeAuthor
#4 465398-move-aggregator-settings-4.patch4.5 KBakahn
#1 465398-move-aggregator-settings-1.patch4.25 KBakahn
move-aggregator-settings.patch4.25 KBakahn

Comments

akahn’s picture

akahn commented
StatusFileSize
new 465398-move-aggregator-settings-1.patch4.25 KB

Missed a period.

dries’s picture

dries commented
Issue tags: +Favorite-of-Dries

Adding to my favorites.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
- * admin/content/aggregator/settings . Use as title the natural name of the
+ * admin/settings/aggregator . Use as title the natural name of the

Please remove the blank space after the path. In general, though, we also want to rework PHPDoc comments so they wrap consistently at 80 chars when doing stuff like that.

-  $items['admin/content/aggregator/settings'] = array(
-    'title' => 'Settings',
+  $items['admin/settings/aggregator'] = array(
+    'title' => 'Aggregator settings',

AFAIK, we do not append "settings" to the title. All items below "Site configuration" are settings, by definition.

akahn’s picture

akahn commented
Status: Needs work » Needs review
StatusFileSize
new 465398-move-aggregator-settings-4.patch4.5 KB

I agree with no space after the path. I thought maybe there was a reason it was like that before.

It looks like of the modules I have enabled, the only one with "settings" is "Regional settings." The reason I like "Aggregator settings" is that when you're on the page, having just the title "Aggregator" is not very descriptive. You need to look at the path or the breadcrumb to know where you are. "Aggregator settings" in that context is more clear. Obviously on the "Site configuration" page it's redundant.

But here's a patch with both changes.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

If you want the page title to be "Aggregator settings", then you need to do drupal_set_title(t('Aggregator settings')); in the page callback.

However, since this would have to be done for all configuration pages, I wonder whether we shouldn't add a new property to hook_menu() elements instead to allow to define the page title separately from the menu item title. As in:

'title' => 'Aggregator',
'page title' => 'Aggregator settings',
'page callback' => 'drupal_get_form',
'page arguments' => array(...),

That would make sense to me. But definitely a separate issue.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

FYI: #465958: Allow to define separate 'page title' in hook_menu()

Status: Fixed » Closed (fixed)
Issue tags: -Favorite-of-Dries

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

Issue tags: +Favorite-of-Dries

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.