Overlay does not work with prefixed URL paths

overlay-parent.js does not take into account the fact that the URL may have a prefix.

There seems to be no API to get the current Drupal URL prefix. The only (ugly) way I know to do it reliably is:

$original_path = $path = '';
$options = array('external' => FALSE, 'prefix' => '');
drupal_alter('url_outbound', $path, $options, $original_path);
return $options['prefix'];

Ideally, a prefix would be set at Drupal.settings.prefix, just like Drupal.settings.basePath.

In this patch, I store the prefix in Drupal.settings.overlay.prefix, so that I can at least show how this bug might be fixed.

Fix my patch to avoid a warning about index: prefix not being set.

Must it be a git diff for the test bot to run it?

$language_url->prefix is only equal to the actual prefix if:

(1) The URL detection method in the language detection and selection tab is selected.
(2) No hook_url_outbound_alter (besides locale_url_outbound_alter) modifies the prefix.

So my original proposal still seems to be the only way to reliably and robustly determine the prefix.

Drupal.settings.pathPrefix should be set once we settle how best to determine the prefix.

Re-rolled with above suggestions. AFAIK, there are no tests for overlay, and I don't have the Drupal JS test-fu to start.

I don't think language_url_split_prefix is all we need to determine the URL prefix, because any module can modify the prefix. language_url_split_prefix will only work if only the locale module sets a prefix.

Actually, $original_path will be passed by reference:

Fatal error: Cannot pass parameter 4 by reference in /var/www/drupal/seven/modules/overlay/overlay.module on line 475

Just wondering, is this needs work for a reason?

Setting to needs review for now... :)

I only left it as "needs work" because I think there should be a better way to get the path prefix: my method seems a bit ugly.

I added drupal_get_path_prefix() and I set Drupal.settings.pathPrefix (and I added similar tests to those for basePath).

Correct tests.

chx says this can't go in as-is, because we're calling an alter hook from more than one place. He's going to brainstorm with jpmckinney on another solution.

function drupal_get_path_prefix() {
  $prefix = &drupal_static(__FUNCTION__);
  if (!isset($prefix)) {
    url('', array('prefix' => &$prefix));
  }
  return $prefix;
}

wouldnt this work with a bit of comments, of course...?

New patch with chx's suggestion. Running test on my machine was taking too long.

Tests passed locally. Added comments.

Modified comments.

I followed the instructions aforementioned in issue's details, applied latest patch on today's drupal 7.x-dev and it works :).

+++ includes/common.inc	23 Apr 2010 23:59:06 -0000
@@ -2525,6 +2525,23 @@ function drupal_get_path($type, $name) {
+    // url() generates the prefix using hook_url_outbound_alter(). Instead of
+    // running the alter hook again here, extract the prefix from url().
+    url('', array('prefix' => &$prefix));
+    $prefix = empty($prefix) ? '' : $prefix;

This seems to be a bit of a hack to me. The better solution would probably to implement drupal_get_path_prefix() properly, and then to re-use it in url() or something. Either way, if we introduce a drupal_get_path_prefix() it should probably be used by the rest of Drupal.

+++ includes/common.inc	23 Apr 2010 23:59:06 -0000
@@ -3549,6 +3566,7 @@ function drupal_add_js($data = NULL, $op
             array('basePath' => base_path()),
+            array('pathPrefix' => drupal_get_path_prefix()),

My only reservation is that the API is not consistent with base_path().

@Dries: should the function be named path_prefix() to be consistent with base_path()?

It is a bit of a hack, but every alternative explored thus far was a bigger hack. url() gets the prefix in its $options array, and then gives modules an opportunity to alter it by calling drupal_alter('url_outbound', $path, $options, $original_path). Code in that alter could change the prefix based on the $path and on the contents of $options. Really, drupal_get_path_prefix() is a bit of a lie, as it really only gets the prefix for a URL whose path is "" and which has no other options.

Perhaps we should not add drupal_get_path_prefix(), and just do the prefix extraction dance in drupal_add_js().

Downgrading from critical, the overlay not popping up on these paths doesn't render the site unusable, it just renders the overlay unusable.

If this was the last critical bug against Drupal 7, and no viable patch, then I don't think it should block release, no.

Frankly I'm fed up that 1/10 critical issues are due to overlay module, for something which is a completely optional admin-only feature that is to say the least controversial, and that percentage appears to be going up as the number of critical issues overall reduces.

There are various issues with hook_url_outbound_alter() and hook_url_inbound_alter() - notably the inability to tell what's happened to paths without running the hooks again in most cases too. I haven't re-read this entire issue, but could we pass an additional argument into those hooks for "prefix" and let modules implementing the hook update that - that'd then mean we could access it from drupal_get_path_prefix() or similar too no?

I'm fine with a critical issue dealing with the inability to know what hook_url_*_alter() are doing without invoking them again, and if necessary postponing this on that. I was looking at the D7 port of globalredirect recently, and that's pretty much broken if anything implements hook_url_inbound_alter() too - not to mention it has to re-invoke the equivalent hooks in D6 too.

@plach could you do a reroll but without drupal_get_path_prefix() as @jpmckinney suggests in #28? That will get the patch to be committed much faster.

I am pleased that my patch from #22 has more or less made it in :) Thanks for the reviews, plach!

Changing to major as per tag.

Tag update

Simple reroll. The url() hack is a bit clever, but I agree it's the best option at this point in the cycle.

Re-reroll (local workspace was out of date).

Still applies

#41: 759844-41-overlay-path-prefix.patch queued for re-testing.

#41 fixes the problem, I can now load the Overlay when using path prefix.

This patch worked fine for me in Seven, Garland & Bartik. Without it there's no access to Overlay for the arabic language I was testing it with.

If Overlay is going to be in core (especially default core) it really needs to be able to work in RTL.

Possibly a slightly different issue but the overlay also does not seem to work with language domains...

Same steps to reproduce, but use language domains on the language settings page, and use "domain" instead of "prefix" on the settings for URL detection.

I can also confirm that #41 works as intended (it makes it possible to load the overlay when using language prefixes).

What I meant in #46 is actually a different issue: language switching does not work at all when the overlay is active; not with language domains nor with prefixes. As per plach's suggestion I have opened a new issue for that: #918738: Overlay does not allow language switching

This is really ugly. The overlay has a really poor handling of URLs, and will completely break as soon as URL aliases are used or URL rewriting takes place.

We need to get rid of Drupal.overlay.isAdminLink completely. The client side is definitely not the place to check internal URLs.

Agreed that the client side is a bad place to check internal URLs. Nobody has ever been happy about it. However: a) it's not as dire as you make out, since aliasing admin URLs isn't that common, and b) nobody has been able to come up with a better option. It's not a trivial undertaking. And until we have a patch to replace it, we need this incremental fix.

If you have ideas on replacing it, though, I'd love to see them in a separate issue.

+++ includes/common.inc
@@ -3653,10 +3653,15 @@ function drupal_add_js($data = NULL, $options = NULL) {
+      // url() generates the prefix using hook_url_outbound_alter(). Instead of
+      // running the drupal_alter('url_outbound') again here, extract the prefix
+      // from url().
+      url('', array('prefix' => &$prefix));

This is an very intelligent solution attempt, I must say. And I've seen a lot.

Technically, this issue is a duplicate of #481560: Provide proper AJAX URL resp. base path depending on context

What you can read over there, and what is not covered by this patch, is a different $base_url, which can also be altered via hook_url_outbound_alter().

Please review aforementioned issue prior to marking this one RTBC again.

That said, the comment should definitely state hook_url_outbound_alter() instead of a PHP construct.

Powered by Dreditor.

Subscribing :)

I was looking through the bug database to see why my overlay does not work in current beta... As soon as I saw the title of this bugreport, I realized what causes it.

http://drupal.org/node/974066

If it can be fixed after RC1, or even at 7.2, it's not critical. Even if it's a really, really bad bug.

plach, seriously, I came up with this solution? shame on me.

Here is a reroll of the patch from #41. It was quite outdated as one could expect. All the AJAX tests seems to have been rewritten since July ;)

So, hopefully I squeezed the tests in correctly here. Lets see what the bot says.

/dixon_ from the Dev Days code sprint in Brussels

@plach:

Actually I missed that. But in general #481560: Provide proper AJAX URL resp. base path depending on context was postponed to 8.x, so I don't think we can do much about it. Or maybe I got something wrong?

But I've now fixed the comment that sun mentioned.

Edit: I'll look into why the patch didn't apply. Could it be because I rolled it from the Git repo?

It should work with Git patches, shouldn't it? The Git repository is fully in sync with CVS. I've now done minor changes to the diff format, so it is the same as in #41. Should work?

Edit: I found out that you need to roll the patch with git diff --no-prefix.

#64: 759844-64-overlay-path-prefix.patch queued for re-testing.

The patch works as intended. Here is another reroll.

#51 is D8 only. This, er, clever workaround is the best solution possible in D7. It needs to go in.

Subscribing

Subscribing

Subscribe

D8 first. Let's get these fixed on D8 and into D7.

Arg, but it's RTBC in 7.x! Why can't we commit it there first? It's basically been re-rolled and RTBC for a year, with only two people who haven't been following it closely chiming it to say that the patch is wrong, and then having other people explain that it's the best solution for 7.x. Read the OP - this is a major bug. You can't have a multilingual site that uses path language prefixes without breaking the overlay. That's major.

Anyway, it's not "needs review" for 8.x, because there is no 8.x patch. For 8.x, we should come up with an alternative patch that makes the 7.x workaround unnecessary.

Very sad. It's always been the policy to do it first in the higher revision. It's just that we weren't really ready I guess to open D8. Just push it through into D8 and then it will pop back here and webchick will commit it easily.

Oh I should mention: The reason that this has been languishing in the RTBC queue is that webchick just isn't committing patches that haven't been applied to D8. So you can move it back to the D7 RTBC queue if you want, but I don't think it will do you any good.

The problem is that this patch isn't what anyone wants for D8. This is one of those dangerous situations where we need a fix for D8 so we don't get regressions, but we don't want to commit a hack to D8, so we run the risk of a 300-comment D8 issue about the right fix, while the correct stopgap for D7 sits around not being committed.

To keep that from happening, I submit that this should go into D8 as is and get backported to D7, and then we can figure out the correct fix for D8 at our leisure. The patch still applies to D8, and I've tested that it still works, so back to RTBC.

Agreed, if it's good enough for D7, then by definition it's good enough for D8 (even if not ideal for D8).

And if you want to make that official policy, #1050616: Figure out backport workflow from Drupal 8 to Drupal 7 is the place to comment :) I think this issue is actually a prime example of why we need that...

+1

Yep, couldn't agree more with #76/77. This is precisely why we need to get things into 8.x first - if we /only/ committed this to D7 and waited for the proper fix in D8, that might never come.

+1

Being a victim of this issue, I fully support this patch. Hoping for it to get into 7.1.

Gratuitous re-roll using git format-patch. No other changes. Still applies cleanly to both 7.x and 8.x, but made separate patches anyway.

PLEASE commit, this is sooo needed!

+++ b/includes/common.inc
@@ -3951,10 +3951,15 @@ function drupal_add_js($data = NULL, $options = NULL) {
+      // url() generates the prefix using hook_url_outbound_alter(). Instead of
+      // running the hook_url_outbound_alter() again here, extract the prefix
+      // from url().
+      url('', array('prefix' => &$prefix));
       $javascript = array(

I wonder if this is the best approach for D8. Feels like we could benefit from a cleaner API to determine the prefix? Any thoughts?

Quoting some stupid jerk from #51:

This is a very intelligent solution attempt, I must say. And I've seen a lot.

This is the "best" approach at this point in time. However, please feel free to step through the commit logs of admin_menu from the past 2 years to learn about 3-5 different incarnations of this approach and attempts to solve it differently, which all failed in one way or another. The particular attempt of this patch is not excluded from that, but comparing it to approaches in history, it's at least more solid than all known attempts to date.

The ultimate but yet unknown answer will have to be found in #481560: Provide proper AJAX URL resp. base path depending on context. However, given the complexity and amount of unknown factors that are outlined in detail on aforementioned issue, my prediction on that issue is, that we won't see a "proper" answer for the next 3 years -- if there can be a proper answer at all.

Agreed with @sun. Theoretically the url prefix can be different per path in fact, but I've not seen a module yet that would do it (except maybe some crazy stuff I did back in 2005 or so :). So the main problem is indeed that we need to work with one prefix that JS could apply to all kinds of paths then. I think this patch at least makes overlay work on multilingual sites, where it is otherwise entirely broken, so let's get this one in. Then we can lament on having URL prefix in a page context for Drupal 8 and also on how to figure out the right prefix for all kinds of paths. I think this should go in now.

I did some more digging and I fully understand the complexity now -- thanks for the clarification/pointers sun and gabor.

I've committed this patch to 8.x and 7.x. One less major bug.

I just have upgraded to Drupal 7.12 and the problem seams to be back...
I'm using the standard theme for admin pages and custom theme for non-admin pages.
I use the latest i18n (7.x-1.4) module to translate nodes.
I have 4 languages enabled. - default English
When I open the node translations tab where I can add the translations - only link which works in overlay is non-prefixed link. Other links opens without overlay. See attached image about what the links I'm talking about.

Forgot to change the status... sorry

@Nor4a -- I'd suggest opening a new issue with steps to reproduce, as this particular issue has already been patched. I'd also suggest testing whether the issue is reproducible without i18n, and if not, probably file the issue against i18n.

This is a quick quick soluction, but it seems to work (no patch, no diff... just quick and dirty, haha).

In the file "/modules/overlay/overlay-parent.js" change the variable regExpPrefix to consider all two-letter prefixes followed by an admin path. This let's Overlay consider the admin links regardless the language they are in.

This makes i18n interface switching on node editing of translations in other languages different from the actual not to leave the overlay layer!

Drupal.overlay.isAdminLink = function (url) {
  ...
  var regExpPrefix = '^([a-z]{2}/)?(';
  ...
};

Hope that helps!

Follow-up issue: #1431076: Overlay does not work with prefixed URL paths