Make path.inc swappable
Eaton - October 12, 2008 - 03:35
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (code needs work) |
Description
Drupal's path aliasing system is a serious performance hotspot, and there's currently no way to change the path retrieval algorithm that it uses. Making it swappable in the same way that cache.inc is swappable would make it vastly easier to switch high-traffic sites to memcached.
Haven't had time to roll the patch yet, but I think it could resolve the long-standing dispute between "load all paths at once" and "load paths as needed" sites.
#1
I support this direction.
For this to happen though, there should be some juggling around.
cache.inc provides only 3 functions, path.inc on the other hand has more function, which an alternate path.inc should provide.
So, these are the ones that a new path.inc would need to provide.
drupal_init_path()drupal_lookup_path($action, $path = '', $path_language = '')
drupal_get_path_alias($path, $path_language = '')
drupal_get_normal_path($path, $path_language = '')
drupal_match_path($path, $patterns)
drupal_is_front_page()
The following functions though need to a new home, so I moved them to common.inc.
drupal_get_title()drupal_set_title($title = NULL, $output = CHECK_PLAIN)
arg($index = NULL, $path = NULL) {
Patch attached.
#2
Actually, drupal_match_path() can be moved as is to common.inc too.
Attaching new patch.
Also, attached is a path-null.inc for testing that does not do anything other than provide the function stubs.
#3
Moving arg() that late is gonna cause some problems, at least for custom code. I'd prefer to move that to bootstrap.inc. arg() is quite brief.
This patch is a good idea.
#4
Moved arg() to bootstrap.inc.
#5
Who will be the first to write a path-memcache.inc when this gets in?
#6
subscribing
#7
I can see some idiots setting 'path_inc' to empty. Better check to see if the value of path_inc exists and if not use the default with a status message in admin/logs/status.
#8
It is not a bad idea, but we don't do this for cache_inc. We will have similarly missing functions and things will stop working badly if they do that..
Let us leave this for a future patch, and focus on this one for now.
#9
I don't think we should base the merit of your current patch
basedon the usability issues of a previous patch. I'll leave it up to Webchick to decide.#10
@earnie
How long does it take someone to realize that the error of their ways if they do that? A message will say function blah is missing.
This is overengineering, and extra code that will execute on every page load, yet will help with only 0.01% of the cases.
Let us have some real reviews now ...
Anyway, here is an update patch after the most recent commits.
#11
arg() must be in path.inc because it can not be used without drupal_init_path -- if you put in bootstrap then some will surely use it from hook_boot and get back bad results. While we do not babysit broken code for sure, this is a great way to introduce bugs into everyone's code -- if you do not explicitly test your page with a path alias then it is going to break. Even core tests do not test that kind of bugs... I would much rather see most of path.inc staying put, and only drupal_lookup_path moved into something pluggable.