Updated: Comment #18

Problem/Motivation

Converting search_view to Controller/Form.

Proposed resolution

Remaining tasks

Use YAML discovery for local tasks (needs built on #2050919: Replace local task plugin discovery with YamlDiscovery)

#2003482: Convert hook_search_info to plugin system
#2050919: Replace local task plugin discovery with YamlDiscovery

Original report by @vijaycs85

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
#53 search-1987806-53.patch17.24 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,953 pass(es).
[ View ]
#53 interdiff.txt2.21 KBtim.plunkett
#51 1987806-search-controller-51.patch16.91 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 59,699 pass(es).
[ View ]
#51 interdiff.txt881 byteskim.pepper
#50 1987806-search-controller-50.patch16.91 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#50 interdiff.txt1.18 KBkim.pepper
#45 1987806-search-controller-45.patch16.94 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 59,045 pass(es).
[ View ]
#41 1987806-search-controller-41.patch16.93 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,747 pass(es).
[ View ]
#41 interdiff.txt1.92 KBkim.pepper
#37 1987806-search-controller-37.patch16.92 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,953 pass(es).
[ View ]
#37 1987806-diff-35-37.txt1.92 KBvijaycs85
#35 1987806-search-controller-35.patch16.78 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,809 pass(es).
[ View ]
#35 1987806-diff-32-35.txt2.18 KBvijaycs85
#32 drupal8.search-module.1987806-32.patch16.84 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,927 pass(es), 8 fail(s), and 1 exception(s).
[ View ]
#32 interdiff.txt3.5 KBdisasm
#29 drupal8.search-module.1987806-29.patch22.88 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,502 pass(es), 490 fail(s), and 45 exception(s).
[ View ]
#29 interdiff.txt780 bytesdisasm
#26 drupal8.search-module.1987806-26.patch22.86 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,507 pass(es), 477 fail(s), and 153 exception(s).
[ View ]
#26 interdiff.txt4.38 KBdisasm
#21 drupal8.search-module.1987806-21.patch22.76 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#21 interdiff.txt9 KBdisasm
#18 drupal8.search-module.1987806-18.patch21.17 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,410 pass(es), 494 fail(s), and 93 exception(s).
[ View ]
#18 interdiff.txt9.21 KBdisasm
#16 1987806-search-controller-16.patch16.5 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 58,472 pass(es), 487 fail(s), and 86 exception(s).
[ View ]
#9 1987806-search-view-controller-9.patch9.75 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,682 pass(es), 47 fail(s), and 1 exception(s).
[ View ]
#9 interdiff.txt3.33 KBkim.pepper
#6 1987806-search-view-controller-6.patch9 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,304 pass(es), 43 fail(s), and 1 exception(s).
[ View ]
#6 interdiff.txt4.6 KBkim.pepper
#4 1987806-search-view-controller-4.patch8.32 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,638 pass(es), 150 fail(s), and 2 exception(s).
[ View ]
#4 interdiff.txt1.82 KBkim.pepper
#2 1987806-search-view-controller-2.patch8.4 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,175 pass(es), 268 fail(s), and 135 exception(s).
[ View ]
#2 interdiff.txt1.02 KBkim.pepper
#1 1987806-search-view-controller-1.patch8.65 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,433 pass(es), 359 fail(s), and 169 exception(s).
[ View ]

Comments

Assigned:Unassigned» kim.pepper
Status:Active» Needs review
StatusFileSize
new8.65 KB
FAILED: [[SimpleTest]]: [MySQL] 55,433 pass(es), 359 fail(s), and 169 exception(s).
[ View ]

Initial attempt.

StatusFileSize
new1.02 KB
new8.4 KB
FAILED: [[SimpleTest]]: [MySQL] 55,175 pass(es), 268 fail(s), and 135 exception(s).
[ View ]

Fixed up docs, and removed ControllerInterface since we aren't injecting anything.

Status:Needs review» Needs work

The last submitted patch, 1987806-search-view-controller-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.82 KB
new8.32 KB
FAILED: [[SimpleTest]]: [MySQL] 55,638 pass(es), 150 fail(s), and 2 exception(s).
[ View ]

Use _content instead of _controller, try fixing routing, and clean up docs.

Status:Needs review» Needs work

The last submitted patch, 1987806-search-view-controller-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.6 KB
new9 KB
FAILED: [[SimpleTest]]: [MySQL] 55,304 pass(es), 43 fail(s), and 1 exception(s).
[ View ]

I needed to include the search.pages.inc to get the form validate and submit functions to work.

Also cleaned up some code style issues.

Status:Needs review» Needs work

The last submitted patch, 1987806-search-view-controller-6.patch, failed testing.

This is looking pretty great!

+++ b/core/modules/search/lib/Drupal/search/Access/SearchCheck.phpundefined
@@ -0,0 +1,34 @@
+

Ha, one empty line too much :)

+++ b/core/modules/search/lib/Drupal/search/Routing/SearchController.phpundefined
@@ -0,0 +1,93 @@
+   * Page callback: Presents the search form and/or search results.

Maybe just remove "Page callback"...

+++ b/core/modules/search/search.routing.ymlundefined
@@ -4,3 +4,13 @@ search_reindex_confirm:
+    module: ~

Just wondering: What does "~" stand for?

Status:Needs work» Needs review
StatusFileSize
new3.33 KB
new9.75 KB
FAILED: [[SimpleTest]]: [MySQL] 55,682 pass(es), 47 fail(s), and 1 exception(s).
[ View ]
  • Fixes issues in #8
  • Injects module_handler
  • Uses new ControllerInterface namespace.

@dawehner ~ is NULL in yaml

Status:Needs review» Needs work

The last submitted patch, 1987806-search-view-controller-9.patch, failed testing.

Status:Needs work» Postponed

I'm postponing until #2003482: Convert hook_search_info to plugin system is in as it touches the issues with routes.

Status:Postponed» Active

Un-postponing

kim.pepper: do you still plan to do this one? If not, perhaps it should be assigned to pwolanin.

jhodgdon Yes, I've started on this. I'll post something shortly.

Status:Active» Needs review
StatusFileSize
new16.5 KB
FAILED: [[SimpleTest]]: [MySQL] 58,472 pass(es), 487 fail(s), and 86 exception(s).
[ View ]

OK, this is not working at all, but I though I should post it so pwolanin could help out.

It's a lot more work that I originally thought. It requires:

- a route controller
- a form interface
- 2 different access checks
- a route subscriber

I'm some way through this, but hit a few issues. Happy for anyone else to carry it forward.

Kim

Status:Needs review» Needs work

The last submitted patch, 1987806-search-controller-16.patch, failed testing.

Issue summary:View changes

Added link to plugin issue

Status:Needs work» Needs review
StatusFileSize
new9.21 KB
new21.17 KB
FAILED: [[SimpleTest]]: [MySQL] 58,410 pass(es), 494 fail(s), and 93 exception(s).
[ View ]

Creating SearchForm Class, injecting plugin.search.manager. Changing SearchController to use new form, removing search_form.

  1. +++ b/core/modules/search/lib/Drupal/search/Access/SearchAccessCheck.php
    @@ -0,0 +1,52 @@
    +class SearchAccessCheck implements AccessCheckInterface {
    +++ b/core/modules/search/lib/Drupal/search/Access/SearchPluginAccessCheck.php
    @@ -0,0 +1,28 @@
    +class SearchPluginAccessCheck extends SearchAccessCheck {

    This should use StaticAccessCheckInterface

  2. +++ b/core/modules/search/lib/Drupal/search/Access/SearchAccessCheck.php
    @@ -0,0 +1,52 @@
    +    $account = $request->attributes->get('_account');
    +    return !empty($account) && $account->hasPermission('search content') && $this->searchPluginManager->getActiveDefinitions();
    +++ b/core/modules/search/lib/Drupal/search/Access/SearchPluginAccessCheck.php
    @@ -0,0 +1,28 @@
    +    $account = $request->attributes->get('_account');
    ...
    +    $access = !empty($account) && $account->hasPermission('search content');

    This should just be return $this->searchPluginManager->getActiveDefinitions() ? static::ALLOW : static::DENY;
    and the route should _permission: 'search content' and _access_mode: 'ALL'

  3. +++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
    @@ -0,0 +1,114 @@
    +  /**
    +   * @param Request $request
    +   * @param int $plugin_id
    +   *   Plugin ID.
    +   * @param string $keys
    +   *   search keywords.
    +   *
    +   * @return array
    +   *   Form Array
    +   *
    +   */

    These need some help

  4. +++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
    @@ -0,0 +1,114 @@
    +      return new RedirectResponse(url($path, array('absolute' => TRUE)));

    Use $this->urlGenerator()

  5. +++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
    @@ -0,0 +1,114 @@
    +
    +}
    +
    diff --git a/core/modules/search/lib/Drupal/search/Form/SearchForm.php b/core/modules/search/lib/Drupal/search/Form/SearchForm.php

    Extra blank line

  6. +++ b/core/modules/search/lib/Drupal/search/Form/SearchForm.php
    @@ -0,0 +1,91 @@
    +class SearchForm extends FormBase implements ContainerInjectionInterface {

    You don't need to implement this

  7. +++ b/core/modules/search/lib/Drupal/search/Form/SearchForm.php
    @@ -0,0 +1,91 @@
    +   *   The search plugin manager.
    +   *
    +   */
    +  public function __construct(SearchInterface $search_plugin) {
    +    $this->searchManager = $search_plugin;
    +
    +  }
    +
    +  /**

    Some weird extra blank lines

  8. +++ b/core/modules/search/lib/Drupal/search/Form/SearchForm.php
    @@ -0,0 +1,91 @@
    +    $form['plugin_id'] = array('#type' => 'value', '#value' => $plugin->getPluginId());
    +    $form['basic'] = array('#type' => 'container', '#attributes' => array('class' => array('container-inline')));
    ...
    +    $form['basic']['processed_keys'] = array('#type' => 'value', '#value' => '');
    +    $form['basic']['submit'] = array('#type' => 'submit', '#value' => $this->t('Search'));

    This should be wrapped onto multiple lins

  9. +++ b/core/modules/search/lib/Drupal/search/Form/SearchForm.php
    @@ -0,0 +1,91 @@
    +    $form['#validate'][] = 'search_form_validate';
    +    $form['#submit'][] = 'search_form_submit';

    Why aren't these ported?

  10. +++ b/core/modules/search/lib/Drupal/search/Form/SearchForm.php
    @@ -0,0 +1,91 @@
    +    $plugin->searchFormAlter($form, $form_state);

    Hm, what's all this?

  11. +++ b/core/modules/search/lib/Drupal/search/Routing/SearchRouteSubscriber.php
    @@ -0,0 +1,71 @@
    +        $collection->add('search_view.' . $plugin_id, $route);
    +++ b/core/modules/search/search.module
    @@ -183,10 +180,7 @@ function search_menu() {
    +        'route_name' => 'search_view',

    This should match

Status:Needs review» Needs work

The last submitted patch, drupal8.search-module.1987806-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9 KB
new22.76 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Regarding #10, SearchInterface has a method that allows a search plugin to alter the form before it's returned to the user. See line 85 of SearchInterface.php.

Addressing remaining comments in #19.

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

The last submitted patch, drupal8.search-module.1987806-21.patch, failed testing.

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

Assigned:kim.pepper» Unassigned

Status:Needs review» Needs work

The last submitted patch, drupal8.search-module.1987806-21.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.38 KB
new22.86 KB
FAILED: [[SimpleTest]]: [MySQL] 58,507 pass(es), 477 fail(s), and 153 exception(s).
[ View ]

I'm not sold on the access checks the way they are, with the one extending the other. At any case, this is going to be dependent on the current user service being accessible in access checks because the SearchPluginManager::pluginAccess takes $account as a parameter.

+++ w/core/modules/search/lib/Drupal/search/Access/SearchPluginAccessCheck.php
@@ -0,0 +1,33 @@
+    $plugin_id = $requirements['_search_plugin_view_access'];
+    return $access && $this->searchManager->pluginAccess($plugin_id, $account);

Let's better return static::ALLOW and static::DENY.

Status:Needs review» Needs work

The last submitted patch, drupal8.search-module.1987806-26.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new780 bytes
new22.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,502 pass(es), 490 fail(s), and 45 exception(s).
[ View ]

fixing return value on access checker

Status:Needs review» Needs work

The last submitted patch, drupal8.search-module.1987806-29.patch, failed testing.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Status:Needs work» Needs review
StatusFileSize
new3.5 KB
new16.84 KB
FAILED: [[SimpleTest]]: [MySQL] 58,927 pass(es), 8 fail(s), and 1 exception(s).
[ View ]

reroll and test failure fixes.

Status:Needs review» Needs work

The last submitted patch, drupal8.search-module.1987806-32.patch, failed testing.

Those failures appear to be real (in search tests).

Status:Needs work» Needs review
StatusFileSize
new2.18 KB
new16.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,809 pass(es).
[ View ]

Fixing test cases(Locally green!!!).

Credit: Thanks to @tim.plunkett and @dawehner for the support on IRC to fix test failures.

Status:Needs review» Needs work
  1. +++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
    @@ -7,25 +7,109 @@
    +  /**
    ...
    +  public function view(Request $request, $plugin_id = NULL, $keys = NULL) {

    This method seem to lack a one line description.

  2. +++ b/core/modules/search/lib/Drupal/search/Form/SearchForm.php
    @@ -0,0 +1,120 @@
    +<?php
    +/**
    + * @file
    + * Contains \Drupal\search\Form\SearchForm.
    + */
    +namespace Drupal\search\Form;

    There are newlines missing here.

  3. +++ b/core/modules/search/lib/Drupal/search/Form/SearchForm.php
    @@ -0,0 +1,120 @@
    +/**
    + * The Search Form.
    + */
    +class SearchForm extends FormBase {

    I guess a little bit more documentation could make it clearer what is going on.

  4. +++ b/core/modules/search/lib/Drupal/search/Form/SearchForm.php
    @@ -0,0 +1,120 @@
    +    $form_state['redirect'] = $form_state['action'] . '/' . $keys;

    Someone could open a follow up to add support for routes on form redirect.

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
new16.92 KB
PASSED: [[SimpleTest]]: [MySQL] 58,953 pass(es).
[ View ]

thanks for the quick review @dawehner. here is the update on individual item with revised patch.

#36.1 - Fixed
#36.2 - Fixed
#36.3 - Fixed
#36.4 - Created new follow up at #2105657: Add support for routes in form redirect.

Status:Needs review» Needs work

Status:Needs work» Reviewed & tested by the community

Thank you!

Status:Reviewed & tested by the community» Needs work

The documentation on this patch needs some work.

a) The controller's view() method is documented as:

+  /**
+   * Renders search page.
+   *

This is not good English and it is not accurate. How about:

Creates a render array for the search page.

b) The 2nd and 3rd parms for this same method:

+   * @param string $plugin_id
+   *   The id of a plugin, i.e. the data type.
+   * @param string $keys
+   *   search keywords.

In $plugin_id, it is not accurate to say "i.e. the data type". Just say "The ID of a search plugin (and note that it is ID not id).
In $keys, this is not following standards. It needs to start with "S" not "s".

c) In SearchForm class:

+  /**
+   * Constructs a search form.
+   *
+   * @param \Drupal\search\SearchPluginManager
+   *   The search plugin manager.
+   *
+   */
+  public function __construct(SearchPluginManager $search_plugin) {
+

There is an extra line in the doc block. And if you are going to fix that, you should also change "a" to "the" in the first line.

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
new16.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,747 pass(es).
[ View ]

Docs fixes for #40

Status:Needs review» Reviewed & tested by the community

As #40 is fixed so back to RTBC.

Thank you!

Status:Reviewed & tested by the community» Needs work

Needs a reroll

git ac https://drupal.org/files/1987806-search-controller-41.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 17338  100 17338    0     0  15008      0  0:00:01  0:00:01 --:--:-- 17746
error: patch failed: core/modules/search/search.routing.yml:16
error: core/modules/search/search.routing.yml: patch does not apply

Status:Needs work» Needs review
StatusFileSize
new16.94 KB
PASSED: [[SimpleTest]]: [MySQL] 59,045 pass(es).
[ View ]

Re-roll

Status:Needs review» Reviewed & tested by the community

I verified this is the same patch (one context line in routing file changed; everything else the same)... Assuming test bot agrees, this should be back to RTBC.

Issue tags:-WSCCI-conversion

#45: 1987806-search-controller-45.patch queued for re-testing.

Issue tags:+WSCCI-conversion

#45: 1987806-search-controller-45.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
    @@ -7,25 +7,110 @@
    +use Symfony\Component\HttpFoundation\RedirectResponse;

    Not needed

  2. +++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
    @@ -7,25 +7,110 @@
    +    $build['search_form'] = drupal_get_form(SearchForm::create($this->container()), $plugin);

    This should be changed to drupal_get_form('\Drupal\search\Form\SearchForm', $plugin); and then you won't need the use statement either.

Status:Needs work» Needs review
StatusFileSize
new1.18 KB
new16.91 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Fixes for #49 plus make use of the new formBuilder interface.

StatusFileSize
new881 bytes
new16.91 KB
PASSED: [[SimpleTest]]: [MySQL] 59,699 pass(es).
[ View ]

Damn. Wrong method. Should be getForm() not buildForm().

Status:Needs review» Reviewed & tested by the community

Thanks! That looks like the correct fix for #49 to me. Back to RTBC.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.21 KB
new17.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,953 pass(es).
[ View ]

Not quite there yet.

Status:Needs review» Reviewed & tested by the community

Thanks Tim. Much better. Forgot we were already using ContainerInjectionInterface. Back to RTBC.

Status:Reviewed & tested by the community» Fixed

Committed 8546c8f and pushed to 8.x. Thanks!

Issue summary:View changes

Applying template

Status:Fixed» Closed (fixed)

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