Now that #132524: Port Admin Menu to 6.x has been completed, we need to re-implement most of the improved functionality of 5.x-2.5 into 6.x-1.0.

For now, only most can be implemented, because Panels 2 has not been ported yet.
I also imagine a final code and documentation clean-up for this issue.

http://drupal.org/node/268128 contains an overview about 5.x-2.5.

CommentFileSizeAuthor
#26 admin_menu.2.5.patch7.09 KBsun
#22 am-features-cleanup-268373-6x-22.patch6.5 KBpwolanin
#15 admin_menu-DRUPAL-5--2.clean-up.patch6.78 KBsun
#15 admin_menu-DRUPAL-6--1.patch5.49 KBsun
#13 am-5.2-features-268373-7x-13.patch23.28 KBpwolanin
#12 am-5.2-features-268373-12.patch20.81 KBpwolanin
#11 am-5.2-features-268373-10.patch20.63 KBpwolanin
#10 admin_menu.2.5.patch11.72 KBsun
#8 admin_menu.2.5.patch11.79 KBsun
#6 admin_menu.2.5.patch11.39 KBsun
#4 admin_menu-DRUPAL-6--1.patch723 bytessun
#2 admin_menu-DRUPAL-6--1.patch723 bytessun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

we should also add an update hook to make sure that users get a clean update?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
admin_menu-DRUPAL-6--1.patch723 bytes

Good point. So let's start with that :)

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Active » Needs work

Before this, I'd actually copy the code from the uninstall hook that deletes all the links - that way you can be sure the user has a clean rebuild (but only once, as opposed to every time).

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
admin_menu-DRUPAL-6--1.patch723 bytes

Hrm. Why copying? ;)

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

The uninstall should actually also make some variable_del() calls too.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
admin_menu.2.5.patch11.39 KB

Attached patch re-implements 90% of all new features in 5.x-2.5.

Missing: Views support -- however, I didn't look into Views 2 yet. It's possible that it already exposes admin links like CCK does. Leaving this for a follow-up patch and committing this one first.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

Much more of the new code should be in the .inc file.

The update hook needs to do a menu rebuild with menu_rebuild() - clearing the menu cache does not cause a rebuild. Actually, in this case you don't even need menu rebuild - you could clear the cache just for admin_menu, and then set the rebuild flag for admin menu.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
admin_menu.2.5.patch11.79 KB

Well, do you mean to alter the update this way?

Also moved all menu callbacks into admin_menu.inc.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

A couple minor problems - for example:

+  // Add developer modules toggle link.
+  if (user_access('administer site configuration')) {
+    $current_state = variable_get('admin_menu_devel_modules_enabled', NULL);
+    $links[] = array(
+      'title' => isset($current_state) ? t('Enable developer modules') : t('Disable developer modules'),
+      'path' => 'admin_menu/toggle-modules',
+      'weight' => 88,
+      'parent_path' => $icon_path,
+    );
+  }

The call to user_access() does not below here - the router item should handle the access.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
admin_menu.2.5.patch11.72 KB

Thanks for reviewing. Yes, that access check is indeed duplicate and removed in attached patch.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Reviewed & tested by the community
FileSize
am-5.2-features-268373-10.patch20.63 KB

do people really want this on by default?

  $form['tweaks']['admin_menu_tweak_modules'] = array(
    '#type' => 'checkbox',
    '#title' => t('Collapse fieldsets on modules page'),
    '#default_value' => variable_get('admin_menu_tweak_modules', 1),
    '#description' => t('If enabled, fieldsets on the <a href="!modules-url">Site building &raquo; Modules</a> page will be initially collapsed.', array('!modules-url' => url('admin/build/modules'))),
  );

Attached patch I'm reasonably happy with. Moved some more code to the .inc file. Also, removed unneeded rebuild call from devel settings page, use 'destination', and add message when developer modules are enabled or disabled.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Version: 6.x-1.0-beta » 6.x-1.x-dev
Status: Reviewed & tested by the community » Fixed
FileSize
am-5.2-features-268373-12.patch20.81 KB

committed the attached to 6.x

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
am-5.2-features-268373-7x-13.patch23.28 KB

committed the attached to HEAD, which includes a couple additional updates from http://drupal.org/node/224333

However, I have not really tested it.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

maybe close to ready for a 6.x-1.0?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Fixed » Needs work
FileSize
admin_menu-DRUPAL-6--1.patch5.49 KB
admin_menu-DRUPAL-5--2.clean-up.patch6.78 KB

Thanks for the additional clean-up! :)

Re #11: Yes. I still can't believe why it has ever been considered better to have those package categories opened by default. The admin/build/modules page is unusable this way, at least on all of our sites (with commonly > 50 modules installed). So it's absolutely required to have those fieldsets closed by default. All in all, one of the primary goals of admin_menu is to improve usability/user experience in all cases where core lacks.

Since this was your first commit to admin_menu, there is (like always) room for improvement:

  1. Testing: As shown at drupalmodules.com, admin_menu is not 100% reliable, thus lacks in testing. The last patch still contained a bug in update 6000 (introduced by myself; see attached patch) and introduced a new one (see below). Let's review and test each patch at least once.
  2. Variable renaming: I'm not very happy with renaming the variable $current_state to $saved_state, because a) admin_menu_toggle_modules() is now inconsistent with admin_menu_admin_menu(), and b) porting improvements between D5 and D6 gets even harder, if both branches also include unnecessary changes like this. But, well, since $saved_state is more appropriate than $current_state, we have to merge this change into D5 now.
  3. Code clean-up: Same as previous, such changes need either to be avoided, or merged into DRUPAL-5--2 as well. Of course, merging is definitely preferred. For this reason, I've switched the order of hook_menu() and hook_init() in attached patch for D6.
  4. HEAD: Good job! However, you missed to create a corresponding changelog entry. In rare cases where there's no issue to reference, I just omit the #1234 part.
  5. Commit message: As mentioned earlier, all commit messages should always be 1:1 identical to changelog entries and adhere to Drupal's best practice on commit messages (which is actually a Standard for admin_menu).

After disabling devel modules, the following error message appears, permanently:

warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'devel_menu_title_theme_developer' was given in .\includes\menu.inc on line 501.

I've already committed attached patch for 5.x-2.x.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

btw: HEAD/7.x still depends on #268050: Unable to create dev snapshot from HEAD for D7 unfortunately...

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

so - this is CNW just because of the snapshot issue?

I had not been following the commit to CHANGELOG convention for my own projects so far, but it's a good idea, so I'll pay more attention if it comes up again.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

erm... no, patch in #15 fixes a missing [] in update 6000. The permanently displayed error message after disabling devel modules (probably introduced by your patch in #11) still needs to be fixed.

The clean-up patch for DRUPAL-5--2 is the only one I've committed so far.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

That patch looks like it does a lot of other stuff too - did you run cvs up before doing the diff?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Yes, of course, as already outlined in #15: $saved_state is inconsistent currently, missing punctuation in messages in admin_menu_toggle_modules(), missing variable_dels in hook_uninstall(), and re-ordering of hook_menu() and hook_init() to make merging differences between 5.x and 6.x easier.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

ah, ok - I misinterpreted you most recent comment as meaning the only change was intended to be in hook_install.

The error message looks like it means we need a real menu rebuild, etc.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
am-features-cleanup-268373-6x-22.patch6.5 KB

how about this?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

We should probably move that rebuilding code into a helper function, since #173570: Add link to disable non-required core modules for upgrading will require it, too.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

hmm, sure - though I usually don't disable when updating...

Also, I think starting with 6.x all updates will run even if the modules are disabled - i.e. there may be no point to that idea.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

worth switching to use this function to load the include?

http://api.drupal.org/api/function/module_load_include/6

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community
FileSize
admin_menu.2.5.patch7.09 KB

Heh... well, yes, I think that is the best way. Thanks for this pointer; it makes #270691: Return full path for drupal_get_path()/drupal_get_filename() obsolete. ;)

I also didn't know that .install files compatible to a previous version of core are loaded during an upgrade. Anyway, it seems that would unnecessarily hold-off this patch. So I've replaced the two instances of include_once() in attached patch, and after a final test/confirmation to ensure everything works, it can be committed.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Reviewed & tested by the community » Fixed

committed and released as 6.x-1.0

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Great!

I've updated the release notes to outline all changes since 5.x-2.4, using the cvs-release-notes script (which is another benefit of following the changelog/commit message syntax).

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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