Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#26 | admin_menu.2.5.patch | 7.09 KB | sun |
#22 | am-features-cleanup-268373-6x-22.patch | 6.5 KB | pwolanin |
#15 | admin_menu-DRUPAL-5--2.clean-up.patch | 6.78 KB | sun |
#15 | admin_menu-DRUPAL-6--1.patch | 5.49 KB | sun |
#13 | am-5.2-features-268373-7x-13.patch | 23.28 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedwe should also add an update hook to make sure that users get a clean update?
Comment #2
sunGood point. So let's start with that :)
Comment #3
pwolanin CreditAttribution: pwolanin commentedBefore 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).
Comment #4
sunHrm. Why copying? ;)
Comment #5
pwolanin CreditAttribution: pwolanin commentedThe uninstall should actually also make some variable_del() calls too.
Comment #6
sunAttached 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.
Comment #7
pwolanin CreditAttribution: pwolanin commentedMuch 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.
Comment #8
sunWell, do you mean to alter the update this way?
Also moved all menu callbacks into admin_menu.inc.
Comment #9
pwolanin CreditAttribution: pwolanin commentedA couple minor problems - for example:
The call to user_access() does not below here - the router item should handle the access.
Comment #10
sunThanks for reviewing. Yes, that access check is indeed duplicate and removed in attached patch.
Comment #11
pwolanin CreditAttribution: pwolanin commenteddo people really want this on by default?
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.
Comment #12
pwolanin CreditAttribution: pwolanin commentedcommitted the attached to 6.x
Comment #13
pwolanin CreditAttribution: pwolanin commentedcommitted the attached to HEAD, which includes a couple additional updates from http://drupal.org/node/224333
However, I have not really tested it.
Comment #14
pwolanin CreditAttribution: pwolanin commentedmaybe close to ready for a 6.x-1.0?
Comment #15
sunThanks for the additional clean-up! :)
Re #11: Yes. I still can't believe why it has ever been considered
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:
$current_state
to$saved_state
, because a)admin_menu_toggle_modules()
is now inconsistent withadmin_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.#1234
part.After disabling devel modules, the following error message appears, permanently:
I've already committed attached patch for 5.x-2.x.
Comment #16
sunbtw: HEAD/7.x still depends on #268050: Unable to create dev snapshot from HEAD for D7 unfortunately...
Comment #17
pwolanin CreditAttribution: pwolanin commentedso - 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.
Comment #18
sunerm... 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.
Comment #19
pwolanin CreditAttribution: pwolanin commentedThat patch looks like it does a lot of other stuff too - did you run cvs up before doing the diff?
Comment #20
sunYes, 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.
Comment #21
pwolanin CreditAttribution: pwolanin commentedah, 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.
Comment #22
pwolanin CreditAttribution: pwolanin commentedhow about this?
Comment #23
sunWe 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.
Comment #24
pwolanin CreditAttribution: pwolanin commentedhmm, 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.
Comment #25
pwolanin CreditAttribution: pwolanin commentedworth switching to use this function to load the include?
http://api.drupal.org/api/function/module_load_include/6
Comment #26
sunHeh... 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.
Comment #27
pwolanin CreditAttribution: pwolanin commentedcommitted and released as 6.x-1.0
Comment #28
sunGreat!
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).
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.