Convert search_view() to a new style controller

Initial attempt.

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

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

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.

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?

• Fixes issues in #8
• Injects module_handler
• Uses new ControllerInterface namespace.

@dawehner ~ is NULL in yaml

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

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.

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

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

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.

#21: drupal8.search-module.1987806-21.patch queued for re-testing.

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.

fixing return value on access checker

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.

reroll and test failure fixes.

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

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.

Opened #2105661: Add support for $form_state['redirect_route']

Thank you!

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.

Docs fixes for #40

As #40 is fixed so back to RTBC.

Thank you!

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

Re-roll

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.

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

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

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.

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

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

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

Not quite there yet.

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

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

Applying template