Convert taxonomy_autocomplete() to a new style controller

I'll give this conversion a shot.

Got a patch that conforms to the WSCCI conversion guide, but the autocomplete JSON result doesn't show up when searching.
Any pointers/thoughts ?

Note to self: never assume that all normal strings are automagically transformed into a Drupal\Core\TypedData\String object in a Controller.

Just in general: do you think it's worth to convert the autocompletion once entity reference should be able to deal with it already?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.phpundefined
@@ -0,0 +1,132 @@
+   * Constructs a new \Drupal\taxonomy\Controller\TermAutocompleteController
+   * object.

Let's keep this in one line ...

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.phpundefined
@@ -0,0 +1,132 @@
+   * Menu callback for taxonomy term autocompletion.
...
+   * This callback outputs term name suggestions in response to Ajax requests

It's not really a callback anymore.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.phpundefined
@@ -0,0 +1,132 @@
+  public function autocomplete(Request $request, String $field_name) {

Missing documentation for the $request variable.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.phpundefined
@@ -0,0 +1,132 @@
+      // Error string. The JavaScript handler will realize this is not JSON and
+      // will display it as debugging information.
+      print t('Taxonomy field @field_name not found.', array('@field_name' => $field_name));
+      exit;

Can't we return a proper response with just the text in there?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.phpundefined
@@ -0,0 +1,132 @@
+  }

Just put an empty line before the end of the class.

Correcting this patch although it might become obsolete because of entity autocomplete.

Turns out that this is planned to be rewritten anyway, so maybe wait for a decision on issue #1169964 : Taxonomy autocomplete: decouple from Field API and factor out a helper so that code is reusable

This issue is part of a critical meta issue, so let's not hold it up on the other issue, which doesn't seem to be making much progress anyway.

Rerolled.

This looks nearly ready!

1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
   @@ -0,0 +1,135 @@
   +      return new Response(t('Taxonomy field @field_name not found.', array('@field_name' => $field_name)));
   

   Should there also be a 403 status code?

2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
   @@ -0,0 +1,135 @@
   +    $tags_typed = drupal_explode_tags($tags_typed);
   +    $tag_last = drupal_strtolower(array_pop($tags_typed));
   

   There is Tags::explode and Unicode:strtolower which better should be used here.

3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
   @@ -0,0 +1,135 @@
   +        $matches[$prefix . $n] = check_plain($name);
   

   Let's use String::checkPlain

1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
   @@ -0,0 +1,138 @@
   +use Drupal\Core\Controller\ControllerInterface;
   +use Drupal\Core\Database\Connection;
   +use Symfony\Component\DependencyInjection\ContainerInterface;
   +use Symfony\Component\HttpFoundation\Request;
   +use Symfony\Component\HttpFoundation\Response;
   +use Symfony\Component\HttpFoundation\JsonResponse;
   +use Drupal\Component\Utility\Tags;
   +use Drupal\Component\Utility\Unicode;
   +use Drupal\Component\Utility\String;
   

   It is really odd to have symfony and drupal mixed up.

2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
   @@ -0,0 +1,138 @@
   +    if (!($field = field_info_field($field_name)) || $field['type'] !== 'taxonomy_term_reference') {
   

   field_info_field($field_name) should be directly replaced by an injected 'field.info'->getField($field_name) method call. In general try to have a look at all the procedural functions if you port something.

3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
   @@ -0,0 +1,138 @@
   +      $query = $this->database->select('taxonomy_term_data', 't');
   +      $query->addTag('term_access');
   +
   +      // Do not select already entered terms.
   +      if (!empty($tags_typed)) {
   +        $query->condition('t.name', $tags_typed, 'NOT IN');
   +      }
   +      // Select rows that match by term name.
   +      $tags_return = $query
   +        ->fields('t', array('tid', 'name'))
   +        ->condition('t.vid', $vids)
   +        ->condition('t.name', '%' . db_like($tag_last) . '%', 'LIKE')
   +        ->range(0, 10)
   +        ->execute()
   +        ->fetchAllKeyed();
   

   I am wondering whether you have tried to port this to a entity query (alias entity field query in D7)

4. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
   @@ -0,0 +1,138 @@
   +      $prefix = count($tags_typed) ? drupal_implode_tags($tags_typed) . ', ' : '';
   

   Tags::implode()

All WSCCI conversions are critical, but we're not yet setting their priority as such. Setting this one though because of #2056627: Form API autocomplete is broken for routes. Because of that issue, I'd love to see this one get in quickly, even if a follow up is needed to finish noncritical details.

Fixed everything from #11.

Perfect!

Fixed the test failure ;)

Back to green.

Nope, see https://drupal.org/node/2056627#comment-7767543

I'm not sure whether my input is still relevant here. Removing myself as assignee.

#16: 1987858-16.patch queued for re-testing.

Working on this, I noticed a couple odd things.

Okay, fixed the three usages of it, and fixed some coding standards.

Manually tested all scenarios.

1. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/wizard/Node.php
   --- /dev/null
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TermAutocompleteController.php
   @@ -0,0 +1,160 @@
   + * Returns responses for Views UI routes.
   

   I doubt :p

2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/widget/TaxonomyAutocompleteWidget.php
   @@ -73,7 +73,8 @@ public function formElement(FieldInterface $items, $delta, array $element, $lang
   +      '#autocomplete_route_name' => $this->getSetting('autocomplete_route_name'),
   +      '#autocomplete_route_parameters' => array('field_name' => $this->fieldDefinition->getFieldName()),
   

   I guess both the route name and the route parameters have to be configurable to make sense? No idea to be hoenst.

3. +++ b/core/profiles/standard/config/entity.form_display.node.article.default.yml
   index 87ec22a..e207ea4 100644
   --- a/core/scripts/generate-d7-content.sh
   
   --- a/core/scripts/generate-d7-content.sh
   +++ b/core/scripts/generate-d7-content.sh
   
   +++ b/core/scripts/generate-d7-content.sh
   @@ -122,7 +122,7 @@
            'settings' => array(
              'size' => 60,
   -          'autocomplete_path' => 'taxonomy/autocomplete',
   +          'autocomplete_route_name' => 'taxonomy_autocomplete',
   

   I guess the d7 content sript is for d7?

generate-d7-content.sh has been updated to use entity_create and similar, so I think it's a lost cause.

The route params don't need to be configurable, they weren't before, they were just appended to the path.

Changes looks good to me and it is green. RTBC from me.

generate-d7-content.sh has been updated to use entity_create and similar, so I think it's a lost cause.

Even other people converted other code, there is no reason to convert it here, while knowing it is wrong.

Beside from that I am wondering whether we need an upgrade path for the settings.

oh we are missing one more change quoting @tim.plunkett from #2056627-22: Form API autocomplete is broken for routes

If 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.

Just talked to @dawehner let's move #autocomplete_path removal to follow up. NW for #27.

Added #2071115: Remove #autocomplete_path in favor of #autocomplete_route.

Fixed #27.

This is good to go now.

Committed 499080f and pushed to 8.x. Thanks!