Make the query parameter key a configurable setting

Created on 30 January 2024, 10 months ago
Updated 18 April 2024, 7 months ago

Problem/Motivation

The query parameter key is hardcoded to keys, so that search results page URLs look like https://example.com/search/google?keys=streets. It would be great if this could be changed to a string of choice, like query or search, for a SRP URL like https://example.com/search/google?query=streets

Proposed resolution

Add a text field to the module config for setting the key.

Remaining tasks

User interface changes

API changes

Data model changes

Since this is a schema change, should include a hook_update_N() to set the default value to keys.

✨ Feature request
Status

Fixed

Version

4.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States jenna.tollerson Atlanta, Georgia, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @jenna.tollerson
  • First commit to issue fork.
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.1 & MariaDB 10.3.22
    last update 10 months ago
    1 pass
  • Status changed to Needs review 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States mark_fullmer Tucson

    This feature request makes sense as a plausible use case for sites, and I think this also forces the configuration of the existing query parameter to be refactored in a way that makes it less haphazard across the code, requiring a centralized definition of the query parameter.

    The merge request, above, provides a new "Query parameter key" setting in the Search configuration, as shown in the image below. Community testing of this would be great; automated test coverage is added as part of the merge request.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.1 & MariaDB 10.3.22
    last update 10 months ago
    1 pass
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.1 & MariaDB 10.3.22
    last update 9 months ago
    1 pass
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.1 & MariaDB 10.3.22
    last update 9 months ago
    Patch Failed to Apply
  • πŸ‡ͺπŸ‡¨Ecuador afsch Otavalo

    Hey @markfullmer, thank you for this contribution, I was testing it (patch) but, it looks like there is a conflict with the Core Search code. Let me give you more context.

    I do have the following scenario, Drupal 10.1.8, Web Server nginx/1.24.0, PHP 8.1.26, DB 10.3.39-MariaDB, google_cse 4.0.0-alpha3.

    For this project, I had to create 2 patches (attached), the first for `google_cse` and another for `drupal:search`. The patches update the keyword used in the URL from `keys` to `query`. It works.

    Now, after using your solution I could configure the `Query parameter key` value. Still, I saw something odd with the block added in the Block Layout and the Search form shown on the Search results page, using the first block for searching, effectively uses the `query` parameter, but using the form from the Search results page uses the `keys` parameter. NO RESULTS are displayed in either case.

    - Block and configuration

    - Configuring the parameter keyword

    - Using the Form from Search Results page

    - Using the block added via Layout Builder

    As an important note, this module requires the `drupal:search` module, IMO I'd say to find a way to override the search class so we could have it configurable in this module only.

  • πŸ‡ΊπŸ‡ΈUnited States mark_fullmer Tucson

    Hey afsch! Thanks for the review! In my testing (and intended design), both the block and the standalone search form work with a custom-defined query setting, and the updated test coverage appears to demonstrate this for both contexts, per https://git.drupalcode.org/project/google_cse/-/merge_requests/5/diffs#0...

    IMO I'd say to find a way to override the search class so we could have it configurable in this module only.

    That's the precise intention of the route subscriber here: https://git.drupalcode.org/project/google_cse/-/merge_requests/5/diffs#c...

    I'll take a closer look at the steps you took and see where we're getting different results and report back.

  • πŸ‡ΊπŸ‡ΈUnited States mark_fullmer Tucson

    Ah...at first I thought that the patch attached in #5 was a replica of what is in the merge request. It is not. Can you test what is in the merge request? If that is new to you, you can get a patchfile from https://git.drupalcode.org/project/google_cse/-/merge_requests/5.diff

  • πŸ‡ͺπŸ‡¨Ecuador afsch Otavalo

    Hey Mark, effectively I'm using your patch and the screenshots are the result of using it in my project. The attached files (patches) are the custom solution I'm currently using to have the parameter updated.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.1 & MariaDB 10.3.22
    last update 9 months ago
    1 pass
  • πŸ‡ΊπŸ‡ΈUnited States mark_fullmer Tucson

    Thanks for the additional info, afsch! The problem ended up being some logic that was expecting only the machine name google_cse_search as the search ID. The latest commit to the merge request makes the Controller override work with any machine name.

    If you have a chance to test again, we'd appreciate it!

  • πŸ‡ͺπŸ‡¨Ecuador afsch Otavalo

    Hey @mark_fuller, it works, now I can see the Results Page with results. But, I had to do a small configuration update in the Search block, this is changing the "Search input type" to 'Drupal'. It was previously set to 'Google' option, when submitting for search it was again using the "keys" parameter hence no results.
    Here are the screenshots:

    Results:

    Maybe it needs a small adjustment about that configuration with "Google" option when configuring the search block, but for now, I'm using your solution as a local patch because it works. If you need me for more testing please let me know :)

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.1 & MariaDB 10.3.22
    last update 9 months ago
    1 pass
  • πŸ‡ΊπŸ‡ΈUnited States mark_fullmer Tucson

    It was previously set to 'Google' option, when submitting for search it was again using the "keys" parameter hence no results.

    Thanks, @afsch! This is fixed in https://git.drupalcode.org/project/google_cse/-/merge_requests/5/diffs?c...

  • πŸ‡ͺπŸ‡¨Ecuador afsch Otavalo

    Hey @mark_fullmer, it works like a charm! I tested with the "Google" option for the block configuration.
    For the same search block configured with "Drupal" option something weird happens, this is because it doesn't redirect to the Search Results page, but keeps the current URL adding extra parameters. E.g.:

    • I'm on the homepage: https://mysite.com
    • Using the search block configured with Drupal option for "Search input type".
    • After submit the form it redirects to: https://mysite.com/?cx=450XXXXXXXXXXXc4844&query=department&op=Search&form_build_id=form-vDTRoATXXXXXXXXXXXXXXXkA8KLWQvBCB0&form_id=google_cse_search_box_form
    • Expected: https://mysite.com/search/results?query=department
    • When configured with Google, it just works as expected.
  • πŸ‡ͺπŸ‡¨Ecuador afsch Otavalo

    Hey Mark, Here I leave a custom code I added to resolve the extra query parameters.
    It overrides the default redirect specifying the path and query only, making it cleaner.

    File: google_cse/src/Form/GoogleCSESearchBoxForm.php

    Instead of:

    $form['sa'] = [
      '#type' => 'submit',
      '#id' => 'google-cse-submit',
      '#value' => $this->t('Search'),
    ];
    

    Use:

    $form['actions'] = [
      '#type' => 'actions',
      'submit' => [
        '#type' => 'submit',
        '#value' => $this->t('Search'),
        '#name' => '',
      ],
    ];
    

    And for the submitForm() function add this code:

    public function submitForm(array &$form, FormStateInterface $form_state) {
      $settings = $form_state->getValue('google_search_settings');
      $query_key = $settings['query_key'] ?? GoogleSearch::$defaultQueryKey;
      $query = $form_state->getValue($query_key);
      // TODO: Update to get `search_id` and set the correct path.
      $url = Url::fromUserInput('/search/results', ['query' => [$query_key => $query]]);
      $form_state->setRedirectUrl($url);
    }
    

    Probably this is the faster way to have it fixed. Let me know if that works. Thanks.

  • πŸ‡ΊπŸ‡ΈUnited States mark_fullmer Tucson

    For the same search block configured with "Drupal" option something weird happens, this is because it doesn't redirect to the Search Results page, but keeps the current URL adding extra parameters

    I *think* this may be a case of "works as designed." In other words, my understanding is that the purpose of the Google CSE-provided block is to have a standalone block that can be placed in the layout and will provide a search input plus search results at that place in the layout. It is explicitly intended *not* to redirect to the search page. My understanding is that if someone wants to have the search redirect to the search page, they are expected to use the Drupal core search block, rather than the Google CSE-provided search block.

    If there is a reason that someone would want/need to use the Google CSE-provided search block rather than the Drupal core search block, and have it redirect to the search page, we need to understand why, and I think it would need to be written up as a new feature request, because changing the behavior to redirect to the search page would change how the block behaves on existing sites.

    So, with that said, if there is a use case for this scenario, let's write it up as a separate issue and investigate further (importantly noting that the implementation would need to honor the new 'query_key' configuration option introduced in this issue.

    Based on the rest of the comments, it appears that this issue is ready to be merged.

  • Pipeline finished with Skipped
    8 months ago
    #136794
  • Status changed to Fixed 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States mark_fullmer Tucson
  • πŸ‡ΊπŸ‡ΈUnited States jenna.tollerson Atlanta, Georgia, USA

    Really stoked to see this merged and released, thanks y'all. :)

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

Production build 0.71.5 2024