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.

CommentFileSizeAuthor
#53 search-1987806-53.patch17.24 KBtim.plunkett
#53 interdiff.txt2.21 KBtim.plunkett
#51 1987806-search-controller-51.patch16.91 KBkim.pepper
#51 interdiff.txt881 byteskim.pepper
#50 1987806-search-controller-50.patch16.91 KBkim.pepper
#50 interdiff.txt1.18 KBkim.pepper
#45 1987806-search-controller-45.patch16.94 KBkim.pepper
#41 1987806-search-controller-41.patch16.93 KBkim.pepper
#41 interdiff.txt1.92 KBkim.pepper
#37 1987806-search-controller-37.patch16.92 KBvijaycs85
#37 1987806-diff-35-37.txt1.92 KBvijaycs85
#35 1987806-search-controller-35.patch16.78 KBvijaycs85
#35 1987806-diff-32-35.txt2.18 KBvijaycs85
#32 drupal8.search-module.1987806-32.patch16.84 KBdisasm
#32 interdiff.txt3.5 KBdisasm
#29 drupal8.search-module.1987806-29.patch22.88 KBdisasm
#29 interdiff.txt780 bytesdisasm
#26 drupal8.search-module.1987806-26.patch22.86 KBdisasm
#26 interdiff.txt4.38 KBdisasm
#21 drupal8.search-module.1987806-21.patch22.76 KBdisasm
#21 interdiff.txt9 KBdisasm
#18 drupal8.search-module.1987806-18.patch21.17 KBdisasm
#18 interdiff.txt9.21 KBdisasm
#16 1987806-search-controller-16.patch16.5 KBkim.pepper
#9 1987806-search-view-controller-9.patch9.75 KBkim.pepper
#9 interdiff.txt3.33 KBkim.pepper
#6 1987806-search-view-controller-6.patch9 KBkim.pepper
#6 interdiff.txt4.6 KBkim.pepper
#4 1987806-search-view-controller-4.patch8.32 KBkim.pepper
#4 interdiff.txt1.82 KBkim.pepper
#2 1987806-search-view-controller-2.patch8.4 KBkim.pepper
#2 interdiff.txt1.02 KBkim.pepper
#1 1987806-search-view-controller-1.patch8.65 KBkim.pepper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

Assigned: Unassigned » kim.pepper
Status: Active » Needs review
FileSize
8.65 KB

Initial attempt.

kim.pepper’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.82 KB
8.32 KB

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
4.6 KB
9 KB

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.

dawehner’s picture

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?

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.33 KB
9.75 KB
  • Fixes issues in #8
  • Injects module_handler
  • Uses new ControllerInterface namespace.
kim.pepper’s picture

@dawehner ~ is NULL in yaml

Status: Needs review » Needs work

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

kim.pepper’s picture

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.

kim.pepper’s picture

Status: Postponed » Active

Un-postponing

jhodgdon’s picture

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

kim.pepper’s picture

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

kim.pepper’s picture

Status: Active » Needs review
FileSize
16.5 KB

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.

Anonymous’s picture

Issue summary: View changes

Added link to plugin issue

disasm’s picture

Status: Needs work » Needs review
FileSize
9.21 KB
21.17 KB

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

tim.plunkett’s picture

  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.

disasm’s picture

Status: Needs work » Needs review
FileSize
9 KB
22.76 KB

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.

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
kim.pepper’s picture

Assigned: kim.pepper » Unassigned

Status: Needs review » Needs work

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

disasm’s picture

Status: Needs work » Needs review
FileSize
4.38 KB
22.86 KB

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.

dawehner’s picture

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

disasm’s picture

Status: Needs work » Needs review
FileSize
780 bytes
22.88 KB

fixing return value on access checker

Status: Needs review » Needs work

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

xjm’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.5 KB
16.84 KB

reroll and test failure fixes.

Status: Needs review » Needs work

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

jhodgdon’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
16.78 KB

Fixing test cases(Locally green!!!).

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

dawehner’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
16.92 KB

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.

tim.plunkett’s picture

Status: Needs review » Needs work
dawehner’s picture

Status: Needs work » Reviewed & tested by the community

Thank you!

jhodgdon’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
16.93 KB

Docs fixes for #40

jibran’s picture

Status: Needs review » Reviewed & tested by the community

As #40 is fixed so back to RTBC.

jhodgdon’s picture

Thank you!

alexpott’s picture

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

Status: Needs work » Needs review
FileSize
16.94 KB

Re-roll

jhodgdon’s picture

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.

Xano’s picture

Issue tags: -WSCCI-conversion

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

pwolanin’s picture

Issue tags: +WSCCI-conversion

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

alexpott’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
16.91 KB

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

kim.pepper’s picture

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

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.21 KB
17.24 KB

Not quite there yet.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

alexpott’s picture

Issue summary: View changes

Applying template

Status: Fixed » Closed (fixed)

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