Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#31 1987858-30.patch13.47 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,403 pass(es).
[ View ]
#31 interdiff.txt462 bytesjibran
#25 interdiff.txt642 bytestim.plunkett
#25 taxonomy-1987858-25.patch13.92 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,242 pass(es).
[ View ]
#23 taxonomy-1987858-23.patch13.91 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#23 interdiff.txt6.19 KBtim.plunkett
#16 1987858-16.patch10.83 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987858-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 interdiff.txt2.51 KBjibran
#13 1987858-13.patch10.78 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,029 pass(es), 0 fail(s), and 6 exception(s).
[ View ]
#13 interdiff.txt5.49 KBjibran
#10 1987858-taxonomy_autocomplete-controller-10.patch9.64 KBoenie
PASSED: [[SimpleTest]]: [MySQL] 57,829 pass(es).
[ View ]
#10 interdiff.txt1.96 KBoenie
#8 1987858-taxonomy_autocomplete-controller-8.patch9.53 KBoenie
PASSED: [[SimpleTest]]: [MySQL] 58,215 pass(es).
[ View ]
#5 1987858-taxonomy_autocomplete-controller-3.patch9.39 KBoenie
PASSED: [[SimpleTest]]: [MySQL] 55,660 pass(es).
[ View ]
#3 1987858-taxonomy_autocomplete-controller-2.patch9.1 KBoenie
PASSED: [[SimpleTest]]: [MySQL] 55,823 pass(es).
[ View ]
#2 1987858-taxonomy_autocomplete-controller-1.patch9.19 KBoenie
FAILED: [[SimpleTest]]: [MySQL] 57,041 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

Comments

Assigned:Unassigned» oenie

I'll give this conversion a shot.

Status:Active» Needs work
StatusFileSize
new9.19 KB
FAILED: [[SimpleTest]]: [MySQL] 57,041 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new9.1 KB
PASSED: [[SimpleTest]]: [MySQL] 55,823 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new9.39 KB
PASSED: [[SimpleTest]]: [MySQL] 55,660 pass(es).
[ View ]

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new9.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,215 pass(es).
[ View ]

Rerolled.

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

Status:Needs work» Needs review
StatusFileSize
new1.96 KB
new9.64 KB
PASSED: [[SimpleTest]]: [MySQL] 57,829 pass(es).
[ View ]

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

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.

Issue tags:-Needs reroll+Needs manual testing
StatusFileSize
new5.49 KB
new10.78 KB
FAILED: [[SimpleTest]]: [MySQL] 58,029 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

Fixed everything from #11.

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.

Status:Needs work» Needs review
StatusFileSize
new2.51 KB
new10.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987858-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed the test failure ;)

Status:Needs review» Reviewed & tested by the community

Back to green.

Status:Reviewed & tested by the community» Needs review

Assigned:oenie» Unassigned

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

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

Assigned:Unassigned» tim.plunkett

Working on this, I noticed a couple odd things.

Assigned:tim.plunkett» Unassigned
Issue tags:-Needs manual testing
StatusFileSize
new6.19 KB
new13.91 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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?

Status:Needs work» Needs review
StatusFileSize
new13.92 KB
PASSED: [[SimpleTest]]: [MySQL] 58,242 pass(es).
[ View ]
new642 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.

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new462 bytes
new13.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,403 pass(es).
[ View ]

Fixed #27.

Status:Needs review» Reviewed & tested by the community

This is good to go now.

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.