Posted by disasm on November 4, 2012 at 4:12am
13 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | forms system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | API clean-up, WSCCI |
Issue Summary
Problem/Motivation
The new router system breaks the ability to have '/' in an autocomplete_path. As such, it's been suggested that we change the parameters passed to autocomplete as a GET string instead.
This issue was discovered in #1801356: Entity reference autocomplete using routes.
Also, is being discussed in a more general sense in #1827544: [Discussion] Drop automated passing of extra argument: Y/N.
Proposed resolution
Modify Form API #autocomplete_path to pass parameters in a GET string instead.
Remaining tasks
User interface changes
API changes
All modules implementing autocomplete_path callbacks would need to modified to use new syntax.
Comments
#1
Actually... wouldn't they need to be modified to specify a controller, not a path, and then Drupal takes care of forming the appropriate URL?
#2
Hmm... That's an interesting idea...
I was more envisioning they would manually add a route to the module's YAML file, like /taxonomy/autocomplete that specified the controller, and the #autocomplete_path in the form API would just be:
<?php$form['element']['#autocomplete_path'] = 'taxonomy/autocomplete';
?>
Then we'd modify the AJAX stuff in autocomplete_path to call taxonomy/autocomplete?string=Foo
I am curious more about your idea though. Maybe we can discuss it during the WSCCI meeting tomorrow?
#3
Fixing title.
#4
KISS?
Didn't update all autocomplete-related tests yet, as I wasn't sure whether this simple solution will be accepted.
#5
The last submitted patch, drupal8.autocomplete-query.4.patch, failed testing.
#6
Works for me. Here's test fixes and a couple minor code fixes.
#7
+++ b/core/modules/taxonomy/taxonomy.pages.inc@@ -102,21 +102,14 @@ function taxonomy_term_feed(Term $term) {
-function taxonomy_autocomplete($field_name, $tags_typed = '') {
- // If the request has a '/' in the search text, then the menu system will have
- // split it into multiple arguments, recover the intended $tags_typed.
- $args = func_get_args();
- // Shift off the $field_name argument.
- array_shift($args);
- $tags_typed = implode('/', $args);
+function taxonomy_autocomplete($field_name) {
+ // A comma-separated list of term names entered in the autocomplete form
+ // element. Only the last term is used for autocompletion.
+ $tags_typed = drupal_container()->get('request')->query->get('q');
IMO, the DX change above is a wash, but...
+++ b/core/modules/user/user.pages.inc@@ -14,9 +14,9 @@
-function user_autocomplete($string = '') {
+function user_autocomplete() {
$matches = array();
- if ($string) {
+ if ($string = drupal_container()->get('request')->query->get('q')) {
The DX of this got much worse, and will be annoying for every contrib module that implements an autocomplete callback. However, I think we'll be able to improve it as we move controllers to the new route system, because the controller will be able to receive $request (or maybe even $query or $q) as a direct parameter. So please, let's not hold this up on that.
#8
Thanks!
The Views page callback argument handling looks strange, but those two Views callbacks for users and taxonomy terms shouldn't exist in the first place... ;)
Also, we might want to consider to introduce two base Autocomplete controllers in the future; one for single term autocompletion, and the much more interesting, one for autocompleting a term in a text field supporting multiple tags — it looks like we're duplicating quite some complex code there. Lastly, we have an issue somewhere for introducing a generic entity autocomplete route/callback/handler (but I'm unable to find it right now).
Anyway, for now this simple change seems to be required and a long-overdue clean-up and simplification, so I'd think this is ready to go?
#9
#10
Turns out that ?q= is absolutely great and would make our autocomplete callbacks automatically compatible with #1271622: Seek a better autocomplete replacement for core (jQuery TokenInput / Select2 / Typeahead.js by Twitter) :)
#11
Actually, I'm not sure what we're waiting for. The patch looks great.
#12
#6: drupal8.autocomplete-query.6.patch queued for re-testing.
#13
Patch looks good to me. It's even got some good documentation in core/includes/form.inc. If it's green, I don't see why it isn't ready.
#14
This looks good to me, too. Any further Symfony-ification can and should happen in follow-up issues.
#15
#16
Committed to 8.x. Thanks.
#17
Automatically closed -- issue fixed for 2 weeks with no activity.
#18
Does this need a change notice? I would think yes, but I'm not sure. In writing one for #1817656: Let user_autocomplete also find the anonymous user. I wanted to reference this, but there isn't one.
#19
I wrote some code related to this at the top of http://drupal.org/node/1896862 that could probably be stolen for this (and then removed from there).
#20
#21
Created http://drupal.org/node/1901518 and updated the change noticed mentioned in #19.
#22
That change notice looks good. Seems to actually be more straightforward for module devs.
#23
Automatically closed -- issue fixed for 2 weeks with no activity.