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
StatusFileSize
new7.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

StatusFileSize
new529 bytes
new8.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
StatusFileSize
new592 bytes
new7.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
StatusFileSize
new945 bytes
new8.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,064 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Here's the test.

effulgentsia’s picture

StatusFileSize
new663 bytes
new9.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
StatusFileSize
new9.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,381 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new12.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

#28: views_taxonomy_ajax-1979038-28.patch queued for re-testing.

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
StatusFileSize
new1.66 KB
new10.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
StatusFileSize
new1.93 KB
new11.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.