Convert the page callback "views_ajax_autocomplete_taxonomy" to a new-style Controller, using the instructions in the WSCCI Conversion Guide.

@see views_menu() in core/modules/views/views.module

Files: 
CommentFileSizeAuthor
#35 vdc-1979038-35.patch11.1 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,264 pass(es). View
#35 interdiff.txt1.93 KBdawehner
#32 vdc-1979038-32.patch10.72 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,888 pass(es). View
#32 interdiff.txt1.66 KBdawehner
#28 interdiff.txt12.07 KBdawehner
#28 views_taxonomy_ajax-1979038-28.patch9.81 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,381 pass(es), 2 fail(s), and 0 exception(s). View
#22 vdc-1979038-22.patch9.71 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,533 pass(es). View
#22 interdiff.txt663 byteseffulgentsia
#21 vdc-1979038-21.patch8.88 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,064 pass(es), 1 fail(s), and 1 exception(s). View
#21 interdiff.txt945 byteseffulgentsia
#16 vdc-1979038-16.patch7.78 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,298 pass(es). View
#16 interdiff.txt592 byteseffulgentsia
#15 vdc-1979038-15.patch8.08 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,281 pass(es). View
#15 interdiff-13-15.txt529 byteststoeckler
#13 vdc-1979038-13.patch7.76 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,312 pass(es). View

Comments

xjm’s picture

Status:Active» Postponed

Since this is an AJAX callback rather than a full page callback, we should wait on #1944472: Add generic content handler for returning dialogs.

xjm’s picture

Issue tags:+VDC
jibran’s picture

tstoeckler’s picture

I think instead of converting it to a route, we should probably convert Views to use [#1801356‎], no?

dawehner’s picture

Theoretically #1169964: Taxonomy autocomplete: decouple from Field API and factor out a helper so that code is reusable would have allowed us to not have our own code, but well now that taxonomy uses the entity reference callback we are basically lost.

glennpratt’s picture

@tstoeckler that issue doesn't appear to exist, what were you trying to reference?

tstoeckler’s picture

Don't know why Drupal.org hates me, but the issue does exist: http://drupal.org/node/1801356. It's the issue (that's already in) about a generic entity reference autocomplete. The problem is that views wants an autocomplete that does not rely on a field, but only filters taxonomy terms by bundle. I still think that should be a generic utility somewhere, but @dawehner is right, that Entity Reference doesn't make sense, as that is solely about fields. Maybe we can make a generic EntityAutocomplete in \Drupal\Core\Entity that Entity Reference extends?

amateescu’s picture

Actually, there is a patch for ER that tries to solve exactly this use case, make the generic entity reference available as a form element that doesn't rely on fields. The code is not quite ready yet and I've been awfully slow at reviewing it, but Views is exactly the reason why I pushed my colleague (@goldorak) to submit it as a D8 patch first. Here's the issue: #1959806: Provide a generic 'entity_autocomplete' Form API element

tstoeckler’s picture

Re #8: Thanks @amateescu, I wasn't aware of that issue. Should we re-title this "Remove views_ajax_autocomplete_taxonomy()" and postpone on that one?

amateescu’s picture

Yep, that sounds like a good plan.

tstoeckler’s picture

Title:Convert views_ajax_autocomplete_taxonomy() to a Controller» Remove views_ajax_autocomplete_taxonomy()
Status:Active» Postponed

OK. :-)

effulgentsia’s picture

Title:Remove views_ajax_autocomplete_taxonomy()» Convert views_ajax_autocomplete_taxonomy() to a Controller
Status:Postponed» Active

#1959806: Provide a generic 'entity_autocomplete' Form API element is currently categorized as a feature request and has had no activity in 3 months. Meanwhile, this issue is part of a critical meta, so shouldn't be postponed on that one.

dawehner’s picture

Status:Active» Needs review
FileSize
7.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,312 pass(es). View

It is good to know that views already has a test for it.

In theory you could argue to just move this code to the autocomplete controller of taxonomy and reuse most of the code.

jibran’s picture

Status:Needs review» Reviewed & tested by the community
+++ b/core/modules/views/lib/Drupal/views/Controller/TagsAutocompleteController.php
@@ -0,0 +1,125 @@
+} ¶

Other then this white space RTBC.

tstoeckler’s picture

FileSize
529 bytes
8.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,281 pass(es). View

There we go. Leaving at RTBC.

effulgentsia’s picture

Status:Reviewed & tested by the community» Needs review
FileSize
592 bytes
7.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,298 pass(es). View

CNR for bot.

effulgentsia’s picture

Status:Needs review» Needs work

TaxonomyIndexTid::valueForm() needs to change the #autocomplete_path to #autocomplete_route_name. If #16 passes tests, that also means we're missing test coverage for this autocomplete appearing.

jibran’s picture

+++ b/core/modules/views/views.module
@@ -322,13 +322,6 @@
-    'theme callback' => 'ajax_base_page_theme',

I was also going to remove the menu item but @dawehner said we can't remove menu items with theme callback.

effulgentsia’s picture

That's generally correct, but autocomplete controllers aren't ajax in the sense of Drupal's Ajax API, so don't require this theme callback.

effulgentsia’s picture

Issue tags:+Needs tests

Tagging per #17.

effulgentsia’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
FileSize
945 bytes
8.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,064 pass(es), 1 fail(s), and 1 exception(s). View

Here's the test.

effulgentsia’s picture

FileSize
663 bytes
9.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,533 pass(es). View

And the fix.

dawehner’s picture

Oh right I agree to remove the 'theme callback'. The request is not a classical ajax request, which pulls down potentially other CSS but it just returns the JSON result.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

It is green it has tests and clean conversion so back to RTBC.

kim.pepper’s picture

#22: vdc-1979038-22.patch queued for re-testing.

catch’s picture

Conversion looks fine, but while we're moving this, why not just put it in taxonomy module?

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/views/lib/Drupal/views/Controller/TagsAutocompleteController.php
@@ -0,0 +1,125 @@
+use Drupal\Core\Controller\ControllerInterface;

This needs to use the new Drupal\Core\Controller\ContainerInjectionInterface as ControllerInterface no longer exists. See #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface

dawehner’s picture

Status:Needs work» Needs review
FileSize
9.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,381 pass(es), 2 fail(s), and 0 exception(s). View
12.07 KB

This adds a new method on the taxonomy autocompletion and reuses as much as possible.

Status:Needs review» Needs work
Issue tags:-VDC, -WSCCI-conversion

The last submitted patch, views_taxonomy_ajax-1979038-28.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+VDC, +WSCCI-conversion

The last submitted patch, views_taxonomy_ajax-1979038-28.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
FileSize
1.66 KB
10.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,888 pass(es). View

Fixed it.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

As #26 is addressed. Back to RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Manually tested works fine

But

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
    @@ -129,34 +130,82 @@ public function autocomplete(Request $request, $entity_type, $field_name) {
    +   * @return \Symfony\Component\HttpFoundation\JsonResponse|\Symfony\Component\HttpFoundation\Response
    +   *   When valid field name is specified, a JSON response containing the
    +   *   autocomplete suggestions for taxonomy terms. Otherwise a normal response
    +   *   containing an error message.
    +   */
    +  public function autocompletePerVid(Request $request, VocabularyInterface $taxonomy_vocabulary) {
    

    This will never return a normal response.

  2. +++ b/core/modules/views/includes/ajax.inc
    @@ -80,53 +80,5 @@ function views_ajax_form_wrapper($form_id, &$form_state) {
    -  $tags_typed = Tags::explode(Drupal::request()->query->get('q'));
    -  $tag_last = Unicode::strtolower(array_pop($tags_typed));
    ...
    -      $term_matches[$prefix . $n] = String::checkPlain($name);
    ...
    -  return new JsonResponse($term_matches);
    

    All the respective use statements for these classes can be removed from the top of this file. They are no longer used.

dawehner’s picture

Status:Needs work» Needs review
FileSize
1.93 KB
11.1 KB
PASSED: [[SimpleTest]]: [MySQL] 59,264 pass(es). View

Good points!

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC as #34 is fixed.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.