Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.I'd really love to help co-maintain this module. I've already filed a couple issues and have 'fixed' them all in my local branch. To see, here's a diff of how I'd like to improve and the changes I'd like to make to the module.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | patch_commit_dd158adcad2d.patch | 10.49 KB | deciphered |
| #2 | 1125012-adminselect.patch | 7.63 KB | dave reid |
| #1 | 1125012-adminselect.patch | 7.7 KB | dave reid |
Comments
Comment #1
dave reidfor reference...
Comment #2
dave reidLeft some debugging in the last patch...
Comment #3
decipheredSure, no problem as I don't have the time to maintain all my modules, but I do want to look over the patches first, so give me a few hours (should be able to read over it all during my lunch break) and I'll get back to you.
Comment #4
decipheredI've read over the patch and in general I like it, make a single hook makes way more sense.
I'm still going to insist that the integration of admin, admin_menu and toolbar be kept in separate files, as I don't like using module_exists() when my standard practices will only include the files if the module exists anyway. The other upside is it's a great way to point users to examples of how to provide their own integration.
This includes any functions that are specific to the modules integration.
Other than that I'm happy, so either you can re-roll, or I can myself when I have a bit more time (this arvo likely). After that, I'll push out 1.1 for both 6.x and 7.x and then add you to the co-maintainers.
Let me know what you think.
Comment #5
dave reidIdeally that integration would go into admin_menu.module and admin.module themselves and we wouldn't have to maintain integration in admin_select at all (ironically, I could push for the admin_menu integration since I co-maintain there). It should be easy to convice those modules to do so considering it only takes about 5 lines of code to integrate. If more modules wanted to add integration, that's what an admin_select.api.php file is for - to show how to do it. They shouldn't be adding un-versioned files into a module folder that will just get removed when someone follows the proper module update procedure (wiping out old files first, then re-downloading the module).
I really disagree about having includes that are always included on every request. It's unnecessary and having to run an file_exists() for every single enabled module is worse for performance than three module_exists() functions, of which the result could be cached.
So if I can't convince you that it's better to do it that way, then I'm fine just submitting patches. I have a bad habit of taking code and changing it to the 'way I would do it' which can grate some people. And so I'd rather not co-maintain if we can't agree on a fundamental feature. :/
Comment #6
decipheredNew patch.
Discussion in progress on IRC.
Comment #7
decipheredWhile we never got to finish our discussion, I have releases 1.1 for both D6 and D7 and added you as a co-maintainer.
If it is your wish to re-write the specific section we disagreed on I will concede the issue, I don't have time for all my modules anyway and this was nearly left as a sandbox module, it was based off something I wrote to quell a dispute on which was better, admin or admin menu, I only re-wrote it last night as I had a request for a mate to do so.