Adding caching here seems rather trivial and should definitely be done.

CommentFileSizeAuthor
#14 1032700_4.patch2.41 KBeclipsegc
#10 1032700_3.patch2.38 KBeclipsegc
#4 1032700_2.patch1.82 KBeclipsegc
#2 1032700_1.patch1.82 KBeclipsegc
#1 1032700.patch1.71 KBeclipsegc

Comments

eclipsegc’s picture

eclipsegc commented
Status: Active » Needs review
StatusFileSize
new 1032700.patch1.71 KB

quick patch

eclipsegc’s picture

eclipsegc commented
StatusFileSize
new 1032700_1.patch1.82 KB

Added a comment as to why we're using cache_menu per sun's instructions.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
+++ includes/path.inc
@@ -512,23 +512,30 @@ function path_is_admin($path) {
+    // Using cache_menu cache to ensure that these paths are rebuilt along with the rest of the menu system.
...
+      // Combine all admin paths into one array, and likewise for non-admin paths,

Exceeds 80 chars.

Powered by Dreditor.

eclipsegc’s picture

eclipsegc commented
Status: Needs work » Needs review
StatusFileSize
new 1032700_2.patch1.82 KB

sorry for the code style issue

catch’s picture

catch
he/him
Primary language English
commented

I'm usually all for adding caching usually, but I wouldn't expect implementations of this function to do more than return some very small arrays. Is there a contrib module doing something evil that makes this expensive?

eclipsegc’s picture

eclipsegc commented

not necessarily but I was discussing adding the ability to create admin_paths through page_manager with merlin and he had mentioned that could start getting expensive without some caching. ksenzee and sun both didn't seem to have issues with adding caching there, so I just went ahead and did it so that I could add the functionality I wanted to page_manager w/o objection. It seems a pretty obvious thing to add caching to since as you add more modules it WILL get bigger, so I just went ahead and did it.

Hopefully that explains where I'm going and why.

David_Rothstein’s picture

David_Rothstein commented
Version: 7.0 » 7.x-dev
Component: overlay.module » base system

I think this will cause some issues as described by @ksenzee's comment in http://drupal.org/node/669510#comment-3750808. (Though I think we already have other reasons why a menu rebuild might be needed when switching settings on the Appearance page.)

I'd also be interested in benchmarks; like @catch says this doesn't seem expensive? I'd expect path_is_admin(), which calls this, to be a bit more expensive (since it does all sorts of regular expression matching on the results), but still probably not that bad.

catch’s picture

catch
he/him
Primary language English
commented
Component: base system » overlay.module

OK page manager would presumably need to go look up path definitions or similar to figure out if they're admin paths or not, I can see that getting expensive. This won't do any particular harm if you don't have such a module installed, and the cache is maintained by existing menu cache maintenance anyway, so no objections from me.

Status: Needs review » Needs work

The last submitted patch, 1032700_2.patch, failed testing.

eclipsegc’s picture

eclipsegc commented
Component: overlay.module » base system
Status: Needs work » Needs review
StatusFileSize
new 1032700_3.patch2.38 KB

OK, per David's issue in #7, the patch failed, I think this one should pass: There doesn't appear to be a more elegant way to clear just the cached item we're looking for, so we're clearing the entire menu cache.

eclipsegc’s picture

eclipsegc commented
Component: base system » overlay.module
David_Rothstein’s picture

David_Rothstein commented

Yup, the patch in #10 looks like it fixes the bug I was worried about in #7. It is good that we already have a test that caught that :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
+++ includes/path.inc
@@ -512,23 +512,31 @@ function path_is_admin($path) {
+    if ($cached = cache_get('admin_paths', 'cache_menu')) {

Let's prefix this bin with path_, please. This is working off path.inc, so it should be in the path namespace.

+++ modules/system/system.admin.inc
@@ -280,6 +280,8 @@ function system_themes_admin_form_submit($form, &$form_state) {
+  // Clear the hook_admin_paths() menu cache.
+  menu_cache_clear_all();

Flushing the entire menu cache seems like lots of overhead here - it should be sufficient to do

cache_clear_all('path_admin_paths', 'cache_menu');

Powered by Dreditor.

eclipsegc’s picture

eclipsegc commented
Status: Needs work » Needs review
StatusFileSize
new 1032700_4.patch2.41 KB

per sun's requests

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

dries’s picture

dries commented

Are we confident this is faster?

eclipsegc’s picture

eclipsegc commented

Dries,

The point here is a little less the immediate benefit of any speed (whether there is or not, I can't say) and more an effort to make sure we don't loose whatever speed currently have to other modules.

In the case of page_manager I actually need to parse through every page_manager page and find out if it's intending on being an administrative path. For the moment, page_manager's the only module I KNOW of that is needing this, and we could certainly put the caching into page_manager instead, but my fear was the fact that this gets called on every page load, so if any module complicates this hook in the slightest, we could lose a lot of performance.

Bottom line, I'm trying to prevent future problems, not solve current ones.

Thanks!

Eclipse

eclipsegc’s picture

eclipsegc commented

Is this going to happen? or should individual modules be expected to provide their own caching mechanism for hook_admin_paths? Just needing a final word one way or another.

Thx

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Duplicate issue: #896570: Add db caching to path_get_admin_paths().

moshe weitzman’s picture

moshe weitzman commented

Sorry, we have way way too much caching in our app already. This patch is a good example of the problem. It proposes to add a cache in case a contrib/custom module does something slow. Thats not core's problem. Slow modules should implement own cache. Caches add a lot of DX headache and expiration/invalidation subtleties.

-1 from me.

dawehner’s picture

dawehner
Primary language German
commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
dries’s picture

dries commented
Priority: Major » Normal
Status: Reviewed & tested by the community » Postponed

I'm with Moshe on this. I'm not sure this is needed in core. I don't understand why this is set to 'major' either. I'm going to postpone this issue until we have more reasons to believe this is required in core.

eclipsegc’s picture

eclipsegc commented

And I've got an issue in place to add this sort of caching for page_manager specific admin pages. Sorry if this has side-tracked time, but there was some discrepancy in irc as to whether this was useful or not. Also, sorry I don't recall marking this "major".

damien tournoud’s picture

damien tournoud commented

I'm -1 here as well. The menu system not do caching on behalf of the implemented modules, we should not do that either here.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

So is it a won't fix after -1 form @moshe weitzman, @Dries and @Damien Tournoud

nod_’s picture

nod_
Primary language French
Location Lille
commented
Component: overlay.module » base system
Issue summary: View changes

Overlay is dead to D8 #2088121: Remove Overlay.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I wondered whether hook_admin_paths() isn't obsolete, but found out that path_is_admin() is still being used for the administrative theme switching and some other stuff.

However, in D8, the hook should be renamed to hook_admin_route() (route vs. path), and ideally, routes should be flagged directly in the route definitions already. In turn, the basic (non-altered) information would be "cached" (compiled), as originally proposed here.

Existing issue: #1316692: Convert hook_admin_paths() into declarative properties on routes

moshe weitzman’s picture

moshe weitzman commented

FYI - #2058845: Pre-load non-admin routes. Its does the opposite of caching admin paths.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Postponed » Closed (outdated)

hook_admin_paths is long gone: https://www.drupal.org/node/2224207