Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zhgenti’s picture

Assigned: Unassigned » zhgenti
zhgenti’s picture

Status: Active » Needs review
FileSize
4.21 KB

Status: Needs review » Needs work

The last submitted patch, use-symfony-request-for-search-module-1999426-7449800.patch, failed testing.

zhgenti’s picture

Status: Needs work » Needs review
FileSize
4.22 KB

Namespace added

Status: Needs review » Needs work

The last submitted patch, use-symfony-request-for-search-module-1999426-3.patch, failed testing.

zhgenti’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, use-symfony-request-for-search-module-1999426-3.patch, failed testing.

kim.pepper’s picture

+++ b/core/modules/search/search.api.phpundefined
@@ -370,7 +370,7 @@ function hook_update_index() {
+ * This example pulls additional search keywords out of the request object,

Should this just be 'querystring'?

+++ b/core/modules/search/search.api.phpundefined
@@ -386,11 +386,13 @@ function hook_update_index() {
+  $keys = \Drupal::request()->request->get('keys');
...
+  $sample_search_keys = \Drupal::request()->request->get('sample_search_keys');

$request->request is actually the $_POST params. If we aren't sure, we can just use $request->get() and it will search query, post and attributes.

We should create a $request variable and re-use here too.

+++ b/core/modules/search/search.pages.incundefined
@@ -16,10 +16,10 @@
+  if (!$keys && \Drupal::request()->request->has('keys')) {
+    $keys = trim(\Drupal::request()->request->get('keys'));

Let's extract a $request variable here too.

+++ b/core/modules/search/tests/modules/search_extra_type/search_extra_type.moduleundefined
@@ -23,9 +23,9 @@ function search_extra_type_search_info() {
+  $search_conditions = \Drupal::request()->request->has('search_conditions');

I think you want $request->get() instead of $request->has()

zhgenti’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

Corrected all issues posted in previous comment. But following part will fail testing:

-  if (isset($_GET['destination'])) {
-    unset($_GET['destination']);
+  if (\Drupal::request()->query->has('destination')) {
+    \Drupal::request()->query->remove('destination');
   }

In a similare issue #1999436: Use Symfony Request for taxonomy module unset($_GET['destination']) was left, do you guys think this is the way it should be done? Perhaps this could be separate issue to refactor all occurrences of destination unsetting?

Status: Needs review » Needs work

The last submitted patch, use-symfony-request-for-search-module-1999426-8.patch, failed testing.

zhgenti’s picture

Status: Needs work » Needs review

Hey guys, testing failed... Please see my previous comment and suggest how to proceed.
Thanks

kim.pepper’s picture

Status: Needs review » Needs work

Hi zhgenti.

Looks like this issue depends on the changes going on in common.inc. #1998696: Use Symfony Request for core includes If you can help out there, that will unblock a number of these conversions.

Thanks,

Kim

zhgenti’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, use-symfony-request-for-search-module-1999426-8.patch, failed testing.

zhgenti’s picture

Status: Needs work » Needs review
FileSize
4.19 KB
marcingy’s picture

Status: Needs review » Needs work

The last submitted patch, use-symfony-request-for-search-module-1999426-14.patch, failed testing.

kim.pepper’s picture

Issue tags: +Needs reroll

Needs reroll

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.54 KB

Reroll.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good

kim.pepper’s picture

#19: 1999426-search-request-19.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/search/search.api.php
    @@ -380,7 +380,7 @@ function hook_update_index() {
    + * This example pulls additional search keywords out of the querystring,
    

    should be query string not querystring - see other documentation in drupal for example the line above :)

  2. +++ b/core/modules/search/search.api.php
    @@ -380,7 +380,7 @@ function hook_update_index() {
    + * This example pulls additional search keywords out of the querystring,
      * (i.e. from the query string of the request). The conditions may also be
    

    The comment needs reflowing as well.

  3. +++ b/core/modules/search/search.api.php
    @@ -396,15 +396,16 @@ function hook_update_index() {
    +  $keys = \Drupal::request()->get('keys');
    ...
    +  $sample_search_keys = \Drupal::request()->get('sample_search_keys');
    
    +++ b/core/modules/search/search.pages.inc
    @@ -19,10 +19,11 @@
    +  $request_keys = \Drupal::request()->get('keys');
    
    @@ -52,7 +53,8 @@ function search_view($module = NULL, $keys = '') {
    +  $form_id = \Drupal::request()->request->get('form_id');
    
    +++ b/core/modules/search/tests/modules/search_extra_type/search_extra_type.module
    @@ -23,9 +23,9 @@ function search_extra_type_search_info() {
    +  $search_conditions = \Drupal::request()->get('search_conditions');
    

    We are in procedural code here no need for the leading \

  4. +++ b/core/modules/search/search.pages.inc
    @@ -19,10 +19,11 @@
    +  // Also try to pull search keywords out of the request to
       // support old GET format of searches for existing links.
    

    Comment needs reflowing

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
4.15 KB

Thanks Alex. Fixes for #22

star-szr’s picture

Assigned: zhgenti » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.1 KB

Search module is changed a lot, so only 2 hunks + new for 'destination'

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

David Hernández’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice
+++ b/core/modules/search/search.pages.inc
@@ -21,10 +21,11 @@
+  // Also try to pull search keywords out of the request to support old GET
+  // format of searches for existing links.
+  $request_keys = \Drupal::request()->get('keys');
+  if (!$keys && !empty($request_keys)) {
+    $keys = trim($request_keys);
   }

Later on in the same function we are assigning $request = \Drupal::request(); we should move that above here and use it.

kim.pepper’s picture

Status: Needs work » Postponed

Let's wait until #1987806: Convert search_view() to a new style controller goes in. It only leaves us the changes in search.module

kim.pepper’s picture

Issue summary: View changes

Added blocker

cosmicdreams’s picture

Shouldn't be blocked anymore since #1998696: Use Symfony Request for core includes is now resolved.

cosmicdreams’s picture

kim.pepper’s picture

kim.pepper’s picture

Status: Needs work » Needs review

Re-rolled

Status: Needs review » Needs work

The last submitted patch, 32: 1999426-search-request-32.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Couldn't be more simple. Also, I can confirm that these are the only $_GET's in the search module.

Xano’s picture

Xano’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 1999426-search-request-32.patch, failed testing.

Xano’s picture

Status: Needs work » Closed (fixed)

This was apparently fixed somewhere else already. There are no occurrences of $_GET in any of Search's files anymore.