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.
CommentFileSizeAuthor
#29 2219467-29.patch25.17 KBmarkhalliwell
#29 interdiff-2219467-27-29.txt2.27 KBmarkhalliwell
#27 2219467-27.patch25.23 KBmarkhalliwell
#27 interdiff-2219467-24-27.txt2.89 KBmarkhalliwell
#24 2219467-24.patch24.83 KBmarkhalliwell
#22 fix_client_side_caching-2219467-22.patch13.91 KBcosolom
#20 fix_client_side_caching-2219467-20.patch14.01 KBcosolom
#17 fix_client_side_caching-2219467-17.patch23.92 KBcosolom
#16 fix_client_side_caching-2219467-16.patch23.91 KBplach
#12 fix_client_side_caching-2219467-12.patch24.25 KBmarkhalliwell
#10 fix_client_side_caching-2219467-10.patch24.25 KBmarkhalliwell
#6 admin_menu_js_7.x-2.x_api-2219467-4--6.diff1.32 KBvadym.kononenko
#6 admin_menu_js_7.x-2.x_api-2219467-4--3-0.patch10.67 KBvadym.kononenko
#5 admin_menu_js_7.x-2.x_api-2219467-4--2.patch10.17 KBvadym.kononenko
#4 admin_menu_js_7.x-2.x_api-2219467-4.patch10.21 KBdas-peter
#2 admin_menu_js_7.x-2.x_api-2219467-2.patch9.18 KBdas-peter
#1 admin_menu_js_7.x-2.x_api-2219467-1.patch4.11 KBmarkhalliwell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markhalliwell’s picture

das-peter’s picture

Status: Active » Needs review
FileSize
9.18 KB

Coming 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:

"....Please work towards the larger solution:...."
"...So please.... stop barking up the wrong tree here...."
"...I would be more than happy to release a stable 7.x-2.0 version of the JS module...."

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.

"fix" something that has already been fixed

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.

markhalliwell’s picture

Hm, 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.

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?

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.

das-peter’s picture

FileSize
10.21 KB

Playing 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.

vadym.kononenko’s picture

I'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

vadym.kononenko’s picture

I'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.

The last submitted patch, 6: admin_menu_js_7.x-2.x_api-2219467-4--3-0.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: admin_menu_js_7.x-2.x_api-2219467-4--6.diff, failed testing.

markhalliwell’s picture

Title: Change API implementation for JS 7.x-2.x » Fix client side caching (including js_7.x-2.x)

Updating title

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
24.25 KB

Ok. 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).

Status: Needs review » Needs work

The last submitted patch, 10: fix_client_side_caching-2219467-10.patch, failed testing.

markhalliwell’s picture

Added user roles to the hash parameters.

markhalliwell’s picture

Issue summary: View changes
purushotam.rai’s picture

Not 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.

plach’s picture

Issue tags: +Needs reroll

No longer applies

plach’s picture

Chris Matthews’s picture

Status: Needs review » Needs work

The 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

truls1502’s picture

Issue tags: +Needs reroll
cosolom’s picture

Issue tags: -Needs reroll
truls1502’s picture

Great job! We need an RTBC before it can be committed! :)

markhalliwell’s picture

markhalliwell’s picture

Assigned: Unassigned » markhalliwell

Reviewing commits to see what has changed since I originally created the patch in #12.

Status: Needs review » Needs work

The last submitted patch, 24: 2219467-24.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

markhalliwell’s picture

Assigned: markhalliwell » Unassigned
Status: Needs work » Needs review
FileSize
2.89 KB
25.23 KB

This 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.

markhalliwell’s picture

Assigned: Unassigned » markhalliwell
Status: Needs review » Needs work
+++ b/admin_menu.module
@@ -276,9 +276,15 @@ function admin_menu_page_build(&$page) {
+  $cache_server_enabled = !$complete && variable_get('admin_menu_cache_server', TRUE);
...
-  if (!$complete && !empty($_COOKIE['has_js']) && variable_get('admin_menu_cache_client', TRUE)) {

Just realized that these two variables are actually not the same... ugh

markhalliwell’s picture

Decided to just move something variables around and simplify some of the if statements.

Status: Needs review » Needs work

The last submitted patch, 29: 2219467-29.patch, failed testing. View results

markhalliwell’s picture

Issue summary: View changes

Updated IS to account for all the API changes.

markhalliwell’s picture

Title: Fix client side caching (including js_7.x-2.x) » Fix client side caching
Issue summary: View changes
Status: Needs work » Needs review

More IS changes.

markhalliwell’s picture

truls1502’s picture

Thank you a lot @markcarver! I greatly appreciate your contribution! We need an RTBC before it can be committed! :)

Chris Matthews’s picture

Status: Needs review » Reviewed & tested by the community

The 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

thalles’s picture

Thanks guys!

  • thalles committed 18ddc86 on 7.x-3.x authored by markcarver
    Issue #2219467 by markcarver, cosolom, vadym.kononenko, das-peter, plach...
thalles’s picture

Status: Reviewed & tested by the community » Fixed

#29 Fixed!

Status: Fixed » Closed (fixed)

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

jstoller’s picture

I 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:

  • Drupal 7.69
  • Administration menu 7.x-3.0-rc6+21-dev
  • JS Callback Handler 7.x-2.5
  • jQuery Update 7.x-3.0-alpha5+1-dev (jquery v1.7)