Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oenie’s picture

Assigned: Unassigned » oenie

I'll give this conversion a shot.

oenie’s picture

Status: Active » Needs work
FileSize
9.19 KB

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

oenie’s picture

Status: Needs work » Needs review
FileSize
9.1 KB

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

dawehner’s picture

Status: Needs review » Needs work

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.

oenie’s picture

Status: Needs work » Needs review
FileSize
9.39 KB

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

oenie’s picture

Status: Needs review » Postponed

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

effulgentsia’s picture

Status: Postponed » Needs work
Issue tags: +Needs reroll

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.

oenie’s picture

Status: Needs work » Needs review
FileSize
9.53 KB

Rerolled.

dawehner’s picture

Status: Needs review » Needs work

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

oenie’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
9.64 KB
dawehner’s picture

  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()

effulgentsia’s picture

Priority: Normal » Critical

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.

jibran’s picture

Issue tags: -Needs reroll +Needs manual testing
FileSize
5.49 KB
10.78 KB

Fixed everything from #11.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1987858-13.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
10.83 KB

Fixed the test failure ;)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to green.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
oenie’s picture

Assigned: oenie » Unassigned

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

jibran’s picture

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

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

The last submitted patch, 1987858-16.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Working on this, I noticed a couple odd things.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue tags: -Needs manual testing
FileSize
6.19 KB
13.91 KB

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

Manually tested all scenarios.

dawehner’s picture

  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?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
13.92 KB
642 bytes

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.

jibran’s picture

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

dawehner’s picture

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.

jibran’s picture

Status: Needs review » Needs work

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.
jibran’s picture

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

jibran’s picture

jibran’s picture

Status: Needs work » Needs review
FileSize
462 bytes
13.47 KB

Fixed #27.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This is good to go now.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

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