Posted by torvall on June 18, 2010 at 9:05am
2 followers
Jump to:
| Project: | Google Translate Links |
| Version: | 6.x-2.0 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Wicher Minnaard |
| Status: | needs work |
| Issue tags: | aggressive caching, block caching |
Issue Summary
With aggressive cache mode enabled, the flags block appears in translated pages (creating the nested translation issue).
If normal cache mode is enabled, the flags block is hidden in translated pages.
Ideally, this module should work with aggressive mode, but if that's not possible, it should, at least, provide that information so that the user is notified on the admin/settings/performance page.
Comments
#1
Thanks torvall.
It's not possible to get this to work in aggressive mode since on each request it must be decided whether to show the block (we don't want to show the block when we are being viewed through the google translate proxy).
There is such a thing as BLOCK_NO_CACHE but it rather goes against the spirit of agressive caching (massive speedups) to do an extra request for each request.
I disabled the block generation in aggressive caching mode so as to prevent the block contents from ever getting into the cache. And there's a warning on admin/logs/status now.
Fixes are in CVS (commit), please test.
#2
Hello Wicher,
I tested the latest version from CVS HEAD and ran into some issues.
First, I tried to simply update the module by copying the files from CVS over the originals and got this:
warning: Missing argument 2 for variable_get(), called in /var/wwwlib/drupal-6/sites/all/modules/gtranslate_links/gtranslate_links.module on line 23 and defined in /var/wwwlib/drupal-6/includes/bootstrap.inc on line 575.So I reverted back to the original version (2.0), disabled the module, deleted all files inside the module folder (except the enabledlangs/ folder), copied the version from CVS and when I tried to enable the module, it had the checkbox disabled and stated that:
This version is incompatible with the 6.17 version of Drupal core#3
Hi torvall,
re the warning: Fixed in CVS (diff)
re the incompatibility message:
For releases (with proper version numbers), the Drupal.org packaging scripts add compatibility information to gtranslate_links.info . CVS versions do not have this information. But you can add it yourself, put lines like this:
version = "6.x-2.0"core = "6.x"
into gtranslate_links.info.
#4
Hello Wicher,
I updated to the latest CVS version and the warning is gone, but the message is not showing up on admin/reports/status (although I do have a duplicate "GD Library" info on that table).
Also, you committed with line 26 commented out on gtranslate_links.module:
<?php// if ($https || $ownrequest || $cache_aggressive || !gtranslate_is_translatable($url)) return;
?>
I "uncommented" that line locally, and it's working well (apart from the missing message).
IMO, the best solution (and most practical for the user) would be to have the message appear at the admin/settings/performance page. For that to happen, you only need to add an empty hook_boot() or hook_exit() implementation to the module like:
<?phpfunction gtranslate_links_boot() {}
?>
#5
Thanks again, torvall. Fixes in CVS.
Yes, I agree, it would be nice to have a warning on admin/settings/performance and I browsed the source to find out how to get one up there.
So earlier on I tried hook_boot(), but for some reason my D5 install doesn't pick it up. Just now I tried hook_exit and that seems to do the trick.
I guess it's a good idea to keep the other notice in the status report though, don't you think?
#6
Hey Wicher,
There are still a few issues. First, the gtranslate_links.install file also needs the second parameter for variable_get(). After reinstalling, I got this warning:
warning: Missing argument 2 for variable_get(), called in /var/wwwlib/drupal-6/sites/all/modules/gtranslate_links/gtranslate_links.install on line 5 and defined in /var/wwwlib/drupal-6/includes/bootstrap.inc on line 575.And with the hook_exit implementation I get the white page of doom with this error message when navigating to admin/reports/status:
Fatal error: Call to undefined function drupal_get_path() in /var/wwwlib/drupal-6/sites/all/modules/gtranslate_links/gtranslate_links.module on line 12After that, any request to any page always gives a white page with the same error. I had to delete the module from the system registry by hand by doing:
delete from system where name like 'gtranslate_links';If the gtranslate_links_exit() function is commented out, the module works well (but the message does not show at the performance page).
I don't know if there should be a notice at the status report page, because I think that's intended for core stuff, but the warning at the admin/settings/performance is important. I seldom look at the status report, but I do check the performance page once in a while (especially after enabling new modules).
On a side note, I just noticed in the logs that for every restricted page I browse as an authenticated user there is an "access denied" entry due to the visibility check. It's not a show-stopper, but I disabled the block for logged in users anyway, since we use the logs a lot (only admins and editors log in, so the block not necessary in my case, but there may be other cases...)
#7
Hmmm, missed that second variable_get(). Thanks for noticing.
I'll have to install a D6 site (I'm on D5) to test the following hypothesis:
* D5 wants gtranslate_links_exit(), not gtranslate_links_boot(), to generate the cache warning
* D6 borks on gtranslate_links_exit(), but not on gtranslate_links_boot().
Maybe both are solvable in a straightforward way. Maybe not, and then it'll just drop off the performance page. Having a stub (fake) function just to get Drupal to show a warning doesn't seem quite right anyway, especially if it can trigger the white page of death (nice expression).
Status report warnings do not appear to be just for core modules. See the api docs. I have a couple of third-party modules that put notices up there.
Furthermore, you wrote:
On a side note, I just noticed in the logs that for every restricted page I browse as an authenticated user there is an "access denied" entry due to the visibility check. It's not a show-stopper, but I disabled the block for logged in users anyway, since we use the logs a lot (only admins and editors log in, so the block not necessary in my case, but there may be other cases...)
Are you saying you still get 403s for restricted pages even when you disable the block for logged in users? That would be a bug.
I'd love to get rid of the HTTP HEAD thing alltogether, but there does not seem to be a way of getting permission info for the 'current' page (see this forum post.
You could consider dropping all requests originating from localhost from your access logs.
#8
Hello Wicher,
Actually, D6 borks on gtranslate_links_boot() as well. I tested it and got the usual "Call to undefined function drupal_get_path()".
I did some research into the init/boot/exit hooks scheme and it turns out that init on D5 and on D6 are not really the same thing. On D5, hook_init and hook_exit are called even on cached pages, whereas on D6 hook_init was replaced by hook_boot. There is still a hook_init on D6 but it is not called on cached page views.
One interesting thing I noticed on the hook_init documentation page is that on the D5 version it states that: and on the D6 version: The D6's hook_boot documentation reads: So, it seems that the presence of these hooks (init/exit on D5 and boot/exit on D6) is causing the errors we're getting. And I agree with you that it isn't a very "elegant" approach.
So... I don't know. Maybe the best thing to do is drop the warning at the performance page and just include it in the module page, README and status page (now I understand your need to have a warning displayed there). It could be interesting to give the user the option to disable the block from being cached even on sites with aggressive cache on.
Regarding the HTTP request and the logging issue, it only logs those 403's when the block is enabled. I looked a bit into that as well, and I think that there may be a way (in D6, at least) to check if the anonymous user has access to the current page using the menu_router table. I tried a few things and failed miserably, but I haven't given up yet. In the next few days I'll try to see if I can come up with something. Maybe there is a similar way to do this on D5 without the request. More info on this at: http://drupal.org/node/102338
#9
Hey Wicher,
Well, I think the solution for the anonymous requests in D6 is close. I've been working on gtranslate_is_translatable() and this is what I got so far:
<?php
function gtranslate_is_translatable($url) {
// This request came in via GT, do not show block (prevents nested translations)
if (strstr($_SERVER["HTTP_VIA"], 'translate.google.com')) return FALSE;
// Visitor is anonymous, therefore the current page will be accessible by GT
global $user;
if ($user->uid == 0) return TRUE;
// User is logged in. Let's see if anonymous users (thinking GT) can reach this page.
if (substr(VERSION, 0, 1) == '5') {
// TODO: Using http requests doesn't seem right. We should be able to do this
// via drupal internals. http:// drupal.org/node/470858
return (substr(drupal_http_request($url, array('Gtranslate_links_loop' => '1', 'Connection' => 'Close'), 'HEAD')->code, 0, 1) == '2');
} else {
// Get the anonymous user object.
$anonymous = drupal_anonymous_user();
// Check if current page is a node. (Other builtin types that may need to be handled: user, book?, forum? etc...)
if(arg(0) == 'node' && is_numeric(arg(1))) {
// Load current node.
$node = node_load(arg(1));
// Check anonymous access to the node.
return node_access('view', $node, $anonymous);
} else {
// Current path.
$path = drupal_get_normal_path(trim($_GET['q'], '/'));
// Menu router item. Indicates the path's access callbacks and arguments.
$router_item = menu_get_item($path);
// Get (de)serialization map.
$map = arg(NULL, $path);
// Build arguments array.
$args = array_merge(menu_unserialize($router_item['access_arguments'], $map), array($anonymous));
// Access control function to execute.
$function = $router_item['access_callback'];
// Execute the access check function with provided parameters.
return call_user_func_array($function, $args);
}
return FALSE;
}
}
?>
I tested this and it works with node and non-node paths and has the expected behaviour. Other built in content types like "user" (and probably "book", "forum") may also need to be handled. When the type is "node", it only checks access for "view" operations. There may be some cases where this is not the intended behaviour. For sanity and security sake, the access operation used should come from the menu_router table. I tried really hard to use the callbacks and arguments for all paths indicated in the menu_router table, but unfortunately that doesn't work for "node" paths (or at least I couldn't get it to work).
For now, it is working in my case as needed, and so I am using it as is. Anyway, I think there is some room for improvement and it may be important to implement the other built in types. Although it works well in this setup, it needs a lot of testing. I have a few other sites (with very different modules and configurations) where I'll test and tune it up.
#10
Forgot to mention that I tested a few use cases and apparently it works perfectly with Content Access.
#11
Hi Torvall,
Good work, it looks promising! I'm swamped with end-of-term exams ATM but I'll look into it soon.
If we can drop the HTTP request, then it's reasonable to set BLOCK_NO_CACHE and have it work in aggressive cache mode (without destroying performance). And then, maybe cache the flag configuration to avoid scanning the flag directory (should be in the OS dentry cache, but still, it will save some syscalls).