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

Fix the whole 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)

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.

Comments

markcarver’s picture

das-peter’s picture

Status: Active » Needs review
FileSize
9.18 KB
PASSED: [[SimpleTest]]: [MySQL] 228 pass(es). View

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.

markcarver’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
PASSED: [[SimpleTest]]: [MySQL] 228 pass(es). View

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

FileSize
10.17 KB
PASSED: [[SimpleTest]]: [MySQL] 228 pass(es). View

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

FileSize
10.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch admin_menu_js_7.x-2.x_api-2219467-4--3-0.patch. Unable to apply patch. See the log in the details link for more information. View
1.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch admin_menu_js_7.x-2.x_api-2219467-4--6.diff. Unable to apply patch. See the log in the details link for more information. View

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.

markcarver’s picture

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

Updating title

markcarver’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.

markcarver’s picture

Status: Needs work » Needs review
FileSize
24.25 KB

Added user roles to the hash parameters.

markcarver’s picture

Issue summary: View changes