Problem/Motivation

The current creation of the autocomplete url_value in js_process_autocomplete() doesn't work with enabled language prefixes and most likely also breaks in other scenarios (language subdomain?).
Reason for this is that we can't add a q parameter on our own if url() will generate it's own q parameter.
Excerpt from url():

    if (!empty($path)) {
      $query['q'] = $path;
    }
    if ($options['query']) {
      // We do not use array_merge() here to prevent overriding $path via query
      // parameters.
      $query += $options['query'];
    }

So if $path isn't empty $query['q'] is set and will be kept! This is exactly what happens if there are path prefixes.

Proposed resolution

Build the required URL on our own by using $base_path and parsing the url for the frontpage provided by url().
Attached patch tries to do this and respect possible rewrites happening in url() - it seems to work fine with language path prefixes.

Remaining tasks

reviews needed

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#8 interdiff-2873484-6-8.txt2.45 KBmarkhalliwell
#8 2873484-8.patch3.51 KBmarkhalliwell
#6 js-autocomplete_not_working_with_path_prefixes_language_domains-2873484-6.patch2.7 KBdas-peter
js-autocomplete_not_working_with_path_prefixes_language_domains.patch3 KBdas-peter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

das-peter created an issue. See original summary.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs review » Postponed (maintainer needs more info)

I really don't understand how this is any different than the current implementation, except maybe more verbose/manual.

This is, for the most part, a direct replication of form_process_autocomplete(), which a few modifications to handle the necessary js_* parameters.

+++ b/js.module
@@ -319,35 +319,37 @@ function js_process_autocomplete($element) {
+    $automcomplete_path = url($element['#autocomplete_path'], array('alias' => TRUE));
...
-        'q' => $element['#autocomplete_path'],
+        'q' => $automcomplete_path,

I think what you're saying is that the autocomplete path needs the proper prefix. In which case, wouldn't just the following line change suffice?

'q' => url($element['#autocomplete_path'], array('alias' => TRUE)),
das-peter’s picture

das-peter CreditAttribution: das-peter at Cando commented
Status: Postponed (maintainer needs more info) » Needs review

@Mark thanks for the feedback :) I try to clarify:

I think what you're saying is that the autocomplete path needs the proper prefix. In which case, wouldn't just the following line change suffice?
'q' => url($element['#autocomplete_path'], array('alias' => TRUE)),

Unfortunately that doesn't work because we're setting q but q is overwritten to <front> in url().
Simply, we can not manually set the parameter q if we don't use clean urls.
Full excerpt of the relevant handling in url():

  // With Clean URLs.
  if (!empty($GLOBALS['conf']['clean_url'])) {
    $path = drupal_encode_path($prefix . $path);
    if ($options['query']) {
      return $base . $path . '?' . drupal_http_build_query($options['query']) . $options['fragment'];
    }
    else {
      return $base . $path . $options['fragment'];
    }
  }
  // Without Clean URLs.
  else {
    $path = $prefix . $path;
    $query = array();
    if (!empty($path)) {
      $query['q'] = $path;
    }
    if ($options['query']) {
      // We do not use array_merge() here to prevent overriding $path via query
      // parameters.
      $query += $options['query'];
    }
    $query = $query ? ('?' . drupal_http_build_query($query)) : '';
    $script = isset($options['script']) ? $options['script'] : '';
    return $base . $script . $query . $options['fragment'];
  }

If you wonder why $path isn't empty with <front> - it's exactly because of the language prefix. <front> becomes e.g. /fr - triggering the overwrite of the q parameter.

We might be able to generate the url as now, parse it and replace q again. Could by a bit less verbose.
Is that preferred?

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented

because of the language prefix. becomes e.g. /fr - triggering the overwrite of the q parameter.

Ah! I see what you're saying now. Hm, that's unfortunate.

I just didn't like the idea of using parse_url if it could have been avoided :-/

The other concern I had was that it was removing the clean_url temporary override, which gave me pause because this is the way it is done in core.

I suppose since this is manually constructing the URL (as external), it will always construct it as a non-clean url, yes?

Could by a bit less verbose. Is that preferred?

Yes, if possible. The documentation around this needs to be a little clearer as well.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs review » Needs work
das-peter’s picture

das-peter CreditAttribution: das-peter at Cando commented
Status: Needs work » Needs review
FileSize
js-autocomplete_not_working_with_path_prefixes_language_domains-2873484-6.patch2.7 KB

Okay here we go. I re-wrote the patch - can't say it looks any nicer :| All the parsing / glueing back together is quite nasty.
I've re-used the "glue" function from advagg: http://cgit.drupalcode.org/advagg/tree/advagg.module#n4931
And I tried to add comments to explain why we need to do all the nasty.
@Mark Please have a look and tell me what you think :) Thanks!

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs review » Needs work

This is even more verbose than the last patch and it introduces the new js_http_build_url function, that's only used once.

I don't think that is necessary here.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs work » Fixed
FileSize
2873484-8.patch3.51 KB
interdiff-2873484-6-8.txt2.45 KB

Considering that this hasn't gotten much attention and I really don't have a better idea (or time to come up with something better), I'm just going to commit this. I made a few minor spacing and typo changes as well as making _js_http_build_url a private function.

  • markcarver committed eb5c142 on 7.x-2.x authored by das-peter 
    Issue #2873484 by das-peter, markcarver: Autocomplete integration not...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.