Updated: Comment #19
Problem/Motivation
menu_get_item() is broken for routes that were formerly MENU_CALLBACK.
This prevents drupal_valid_path() from working, which in turn prevents form_process_autocomplete() from working.
Steps to reproduce:
Go to node/add/page
Expand the "Authoring Information" details element
Attempt to change the user
Expected:
A throbber appears before typing, is activated while typing, and what you type is autocompleted.
Actual:
Nothing.
Proposed resolution
While menu_get_item() needs to be fixed, we should also be moving away from path-bound autocomplete.
This adds #autocomplete_route, which takes a route name instead.
Developers should use #autocomplete_route going forward instead of #autocomplete_path.
Remaining tasks
Write tests- Write documentation
- Write change notice
User interface changes
N/A (other than fixed regression)
API changes
API addition of #autocomplete_route, #autocomplete_path is left for hook_menu() page callbacks.
Related Issues
N/A
Comment | File | Size | Author |
---|---|---|---|
#16 | autocomplete-2056627-16.patch | 13.85 KB | dawehner |
#16 | interdiff.txt | 1.38 KB | dawehner |
#15 | autocomplete-2056627-15.patch | 12.46 KB | tim.plunkett |
#15 | interdiff.txt | 3 KB | tim.plunkett |
#13 | autocomplete-2056627-13.patch | 11.45 KB | dawehner |
Comments
Comment #1
tim.plunkettComment #2
pwolanin CreditAttribution: pwolanin commentedWe should at least have a @todo to replace menu_item_route_access() with the new method at #2046737: Add a method to the AccessManager that only needs a route name and parameters
Comment #4
tim.plunkettThis needs to support parameters too (taxonomy autocomplete will need to pass the vocab), so maybe this should just wait on #2046737: Add a method to the AccessManager that only needs a route name and parameters
(that was a random fail)
Comment #5
dawehnerYeah you don't want to use the router provider by the url generator and pass in the route name and paramters.
Comment #6
tim.plunkettWell we still need that to check access. We'll see what it looks like after the AccessManager issue.
Comment #7
tim.plunkettBlocker was in. Will revisit this soon.
Comment #8
dawehnerWorking on the patch 10mins, getting the tests right: 30 minutes. Good night!
I have chosen to use route_name to just tell people that it is the name.
This for sure needs some documentation, though it is seriously too late now.
Comment #9
dawehnerWorking on the patch 10mins, getting the tests right: 30 minutes. Good night!
I have chosen to use route_name to just tell people that it is the name.
This for sure needs some documentation, though it is seriously too late now.
Comment #10
tim.plunkettI was trying to minimize code changes here, but I think this is awkward. We should just move the drupal_valid_path() into the else {} part.
Double blank line. Nice test though!
A bit strange :)
Trailing whitespace
Comment #11
mradcliffePostgresql catches this hard as user entity tries to load the uid 'autocomplete', which fails strict typing. Mysql driver probably ignores or munges the data.
Comment #13
dawehnerThank you for the review!
Comment #15
tim.plunkettFixed some docs and coding style, #2062745: Automatically set the request context on the url generator fixed the bugs.
We should have regression testing for the author autocomplete on the node form.
Comment #16
dawehnerHere is a test for the author form. Yes we could invent even more tests for every damn place a user auto-completion appears but I don't think we need that as we test the same functionality all over again.
Comment #17
dawehnerA general question is whether we want to support the new routing system in drupal_valid_url, see https://drupal.org/node/876580 for a rough comment.
Comment #18
mradcliffeJust a note: It may be nice for new developers to point out why one would use path or route. I think this could be done initially in a change notice and in handbook documentation.
Comment #19
dawehnerOn the longrun you should never use autocomplete_path, that is just in there as long we haven't converted all autocompletions.
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedI think this approach makes sense, but I'm not crazy about:
I'd rather see this issue remove it, so that entity_reference module can be updated to use #autocomplete_route in all cases. Tagging for upgrade path because of that. Looks like #1987858: Convert taxonomy_autocomplete() to a new style controller is close to done. Can we focus on getting that in quickly, and then do the full conversion of #autocomplete_* here?
Comment #21
effulgentsia CreditAttribution: effulgentsia commented@alexpott approves, since this is a critical fallout of router conversion.
Comment #22
tim.plunkettIf we convert the taxonomy autocomplete before this goes in, then it too will be broken...
If that is the last usage of #autocomplete_path, why not put this in now, and then remove #autocomplete_path over there.
Comment #23
dawehnerI agree. There is neither a fundamental problem nor actually a lot of work if we keep autocomplete_path in for now. This seems really like overthinking ;)
Remove wrong tag.
Comment #24
tim.plunkettI posted too many patches to RTBC, but the test looks great!
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedlooks great
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedWell, per #20, putting this in now only fixes the user autocompletes that the patch touches, and leaves ER fields targeting users still using the broken #autocomplete_path. But I guess that's not a further regression, just a very incomplete fix. So ok, let's do this one first. As long as we recognize that this is in fact an API change, not merely an addition, because the retaining of #autocomplete_path is just an artifact of patch order, not something that's releasable.
I don't know why we need the second one to be an absolute URL, but I think that's been that way in HEAD since autocomplete functionality was first added to Drupal. Given that, we should probably be consistent and make the new way also output absolute URLs.
Comment #27
effulgentsia CreditAttribution: effulgentsia commented#26 was an xpost with #25. I'm ok with it being a follow up though, so back to RTBC.
Comment #28
catchCommitted/pushed to 8.x, thanks!
Will need a change notice.
Comment #29
catchComment #30
dawehnerhttps://drupal.org/node/2070985
Comment #31
tim.plunkettUpdated, but I think we're good here https://drupal.org/node/2070985/revisions/view/2812065/2812101
Comment #32
jibranAdded #2071115: Remove #autocomplete_path in favor of #autocomplete_route for #22.
Comment #33
dawehnerGood addition tim!
Comment #34.0
(not verified) CreditAttribution: commentedUpdated issue summary as of comment #19
Comment #35
cilefen CreditAttribution: cilefen commented#2651798: claroAutocompleteTest passes, but log shows a 404