Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
- Client side caching doesn't always work
- The JS module has a new branch/API (7.x-2.x) due to security issues (SA-CONTRIB-2016-063.
Proposed resolution
Refactor and fix the whole client side caching process.
Remaining tasks
Create a patch
User interface changes
None
API changes
- Added
admin_menu_get_hash(array $parameters)
- Added
hook_admin_menu_get_hash_alter(array &$parameters)
- Added support for JS Callback Handler (7.x-2.x)
- Removed unnecessary
admin_menu_cache_get($cid)
- Removed unnecessary
admin_menu_cache_set($cid, $data)
- Removed support for JS Callback Handler (7.x-1.x)
- Caching of the admin menu has been moved to the
admin_menu_page_build()
function;admin_menu_output()
will now always [re]build the admin menu when invoked.
Data model changes
- Removed usage of the unnecessary
cache_admin_menu
bin. - Created a more stable hash identifier generator to use through out client side caching code.
Comment | File | Size | Author |
---|---|---|---|
#29 | 2219467-29.patch | 25.17 KB | markhalliwell |
#29 | interdiff-2219467-27-29.txt | 2.27 KB | markhalliwell |
Comments
Comment #1
markhalliwellComment #2
das-peter CreditAttribution: das-peter commentedComing from here #1981308: Call to undefined function field_info_fields() in admin_menu.map.inc on line 111
I personally think the answer #28 there isn't acceptable. Here's why I personally think that:
There's no way you can force people to switch to JS 2.x without even having a dev relase / or at least a hint that a 2.x branch is available and should be used due security and what not enhancements.
If you want people to bark up another tree, give em one.
What about people using other modules that integrate with JS 1.x? Do you expect everyone to re-write these modules just for the sake of having a reasonably fast admin menu?
I mean I can do that, I'd even say I like to live dangerously and often use bleeding edge versions. But not everyone is technically capable, limited by processes (production deployment / dependency management) or just willing to do so.
So attached is a patch that should work with BOTH versions of JS. Not nice but a band-aid.
And it gives developers at least a chance to fix issues with the 1.x integration and another tree to bark up. ;)
I even added a hint in the status report about JS 2.x. Would be nice if you could provide a dev or an alpha release of JS 2.x or mention it on the project page.
Comment #3
markhalliwellHm, I could have sworn there was at least the dev version on the project page. I apologize, work has been hectic for many months. I've also gone ahead and created 7.x-2.0-beta1 as you suggested and will be updating the project page shortly.
FWIW though, your comment above kind of proves the point I was attempting to get across in the other issue: "forward thinking" and "please, for the love of all drupal, move to this issue". You did just that and also informed me of some things that I had honestly thought were already done. So this is, in fact, actual progress.
Also, the fact that you have found a way to support both version is, very interesting to say the least. I will have to review it when I get the time. I'm still not sure if that is the right approach though.
Actively supporting outdated/insecure code is setting everyone and every site up for failure. If we don't "force", as you call it, people do not upgrade and still run insecure code. Now, in the very rare cases where that really isn't an option, that is why we have artifact patches that retroactively "fix" some issue without having to fully upgrade. It doesn't mean we should commit that artifact patch (which inevitably supports the insecure code) to the upstream repo.
In the case of the JS module, the reason why it has changed so drastically was because of an unfortunate and an unavoidable API/concept change. Because the JS module is basically bypassing Drupal and implementing our own bootstrap process, it wasn't just a "simple" fix. Hence the major version bump from 7.x-1.x to 7.x-2.x.
So I kind of fail to see your point of view of being "forced". This isn't the first time this has happened in software. The fact of the matter is that sometimes software changes so drastically you have to reintegrate the other software the relies on it. So yes, modules will have to re-write their integration with the JS module... it's inevitable.
Now, that all being said... I really would like to move past this debate and just focus on actually getting both of these out the door.
I really would like to release 7.x-2.0 as soon as possible. I will admit that I am really not that proactive in issue queues. Instead, I am rather reactive and it's basically just the squeaky issues that get my attention. My time is valuable and stretched between many different projects. I utilize d.o the most for the dashboard tracking of issue updates/progress.
Comment #4
das-peter CreditAttribution: das-peter commentedPlaying around with JS 2.x I found an issue in my previous patch. I lost a part from the original 2.x only patch by Mark.
Comment #5
vadym.kononenko CreditAttribution: vadym.kononenko commentedI've verified patch #4.
Works fine, but... I'm using nginx as web server with 'perusio'-based config.
I've debugged required 'js' options ('token', 'module' and 'callback') is empty when we use 'GET' method for http queries.
[QUERY_STRING] string "q=/js/admin_menu/cache/79b37669c36ea7d14c6c7c602f81e024"
[REQUEST_URI] string "/js/admin_menu/cache/79b37669c36ea7d14c6c7c602f81e024?js_module=admin_menu&js_callback=cache&js_token=cgnDjtuBJwAWo5h0hIqXVI7za0CjNft1F7rD4j__PC0"
So, I've changed it to 'POST' and now it works as expected.
It could be similar to this issue https://www.drupal.org/node/1036962
Comment #6
vadym.kononenko CreditAttribution: vadym.kononenko commentedI've found while menu builing theme() function is using and this require to enable full drupal bootstrap... and we can flush cache not only through web.
Comment #9
markhalliwellUpdating title
Comment #10
markhalliwellOk. I've gone through the entire "client side caching" workflow, both with and without the JS (7.x-2.x) module.
This should also fix a lot of other bugs in this issue queue regarding client side caching.
I went ahead and removed the 7.x-1.x JS BC support since that branch is no longer supported (SA-CONTRIB-2016-063).
Comment #12
markhalliwellAdded user roles to the hash parameters.
Comment #13
markhalliwellComment #14
purushotam.rai CreditAttribution: purushotam.rai at QED42 commentedNot sure, if its relevant or not, but I was facing duplicate content issue at my side because of admin_menu.js client side caching. I tried using #12 patch and post that my content started repeating infinite times. So, I feel something is definitely wrong over here.
Comment #15
plachNo longer applies
Comment #16
plachRerolled
Comment #17
cosolom CreditAttribution: cosolom commentedNeed to add path.inc for include
Comment #18
Chris Matthews CreditAttribution: Chris Matthews commentedThe patch in #17 on 7.x-3.x-dev failed/does not apply to admin_menu.js and admin_menu.module.
error: patch failed: admin_menu.js:18
error: admin_menu.js: patch does not apply
error: patch failed: admin_menu.module:451
error: admin_menu.module: patch does not apply
Comment #19
truls1502Comment #20
cosolom CreditAttribution: cosolom commentedRerolled. But this is without testing
Comment #21
cosolom CreditAttribution: cosolom commentedComment #22
cosolom CreditAttribution: cosolom commentedThis will be better
Comment #23
truls1502Great job! We need an RTBC before it can be committed! :)
Comment #24
markhalliwellI'm not sure how the patch in #20 suddenly became 10kb less, but I imagine it's not a proper re-roll and seems to be missing most of the
admin_menu_output()
changes.Re-rolling from #16.
Comment #25
markhalliwellReviewing commits to see what has changed since I originally created the patch in #12.
Comment #27
markhalliwellThis takes into account the changes made in:
#2211829: admin_menu_tree() can unexpectedly override theme_menu_tree()
#2922483: Switch User not working on inner links
#841516: Add option to change font size
edit: Although the changes made in #2922483: Switch User not working on inner links may not be needed as this issue attempts to solve most of these caching issues which that issue seems to have just put a band-aid on.
Comment #28
markhalliwellJust realized that these two variables are actually not the same... ugh
Comment #29
markhalliwellDecided to just move something variables around and simplify some of the if statements.
Comment #31
markhalliwellUpdated IS to account for all the API changes.
Comment #32
markhalliwellMore IS changes.
Comment #33
markhalliwellComment #34
truls1502Thank you a lot @markcarver! I greatly appreciate your contribution! We need an RTBC before it can be committed! :)
Comment #35
Chris Matthews CreditAttribution: Chris Matthews commentedThe patch in #29 to admin_menu.js and admin_menu.module applied cleanly to admin_menu 7.x-3.x-dev and looks good to me.
This would be good to include for issue #3021941: Plan for Administration menu 7.x-3.0 release
Comment #36
thallesThanks guys!
Comment #38
thalles#29 Fixed!
Comment #40
jstollerI found that the admin menu still disappears when client-side caching is enabled, if I disable the Devel module. With Devel enabled everything was fine, but when I disable it the menu disappears from every page, in both the public theme and the admin theme, except for the Admin Menu config page. Enabling Devel or disabling client-side caching fixes the problem. I assume it's related to this issue, but I can open up a new one if you'd prefer. The site is running: