Problem/Motivation

I want to modify the $params query parameters being constructed by the QueryParamBuilder service, based on the attached view’s configuration.

Proposed resolution

Add an AlterParamsEvent that is triggered immediately after $params['track_total_hits'] = TRUE;, allowing modifications to this parameter.

Remaining tasks

  1. Write an 8.0.x merge request - merge request !158 created by @filsterjisah in #3
  2. Write a 9.0.x merge request - merge request !195 created by @mparker17 in #10
  3. Maintainer review - done by @mparker17 by #10
  4. Community review - done by @fathershawn in #14
  5. Commit to 9.0.x - done by @mparker17 in #16
  6. Commit to 8.0.x - done by @mparker17 in #18
  7. Release 9.0.x - released in 9.0.0-alpha3 by @mparker17
  8. Release 8.0.x - released in 8.0.0-alpha7 by @mparker17

User interface changes

None.

Introduced terminology

None.

API changes

Adds an AlterSearchQueryParamsEvent that is triggered immediately after $params['track_total_hits'] = TRUE;, allowing modifications to this parameter.

Data model changes

None.

Release notes snippet

Added an AlterSearchQueryParamsEvent to modify query parameters created by QueryParamBuilder

Issue fork elasticsearch_connector-3554964

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

filsterjisah created an issue. See original summary.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Component: Elasticsearch Connector Search API » Code

@filsterjisah, This sounds like a useful addition! Let me know if there's anything I can do to help!

filsterjisah opened merge request !158

filsterjisah’s picture

filsterjisah commented

Instead of using the proposed solution in the description, I added a new AlterParamsEvent. This event is triggered immediately after $params['track_total_hits'] = TRUE; allowing modifications to this parameter. Additionally, when we want to display the full Elastic query, this parameter is included as well.

filsterjisah’s picture

filsterjisah commented
Status: Active » Needs review
sokru’s picture

sokru commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Tests should be added.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Title: Allow modification of search params based on the current $query object » Add AlterParamsEvent to modify query parameters created by QueryParamBuilder
Issue summary: View changes

Updated the issue title to capture the new scope. I also updated the issue summary.

I have yet to review this; I'm also going to see if I can add a test.

Off the top of my head, I may suggest we rename the event AlterSearchQueryParamsEvent for clarity / disambiguation with other types of parameters (e.g.: when creating an index).

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue tags: -Needs tests

I fixed a bug where changes to the parameters array were ignored. I also added some comments.

Then I wrote tests.

I do think it's worth renaming the event to AlterSearchQueryParamsEvent; so if tests pass here, then I'll rename the event.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Title: Add AlterParamsEvent to modify query parameters created by QueryParamBuilder » Add AlterSearchQueryParamsEvent to modify query parameters created by QueryParamBuilder
Status: Needs work » Needs review

Okay! The tests pass after the rename, so I think this is ready for community review.

@filsterjisah, I would greatly appreciate it if you could test the new changes, to make sure they work. Note you will have to alter the class name of the event you're listening to (i.e.: from AlterParamsEvent to AlterSearchQueryParamsEvent). If it works, then please change the status of this ticket to "Reviewed & tested by the community".

I will also create a 9.0.x merge request shortly.

mparker17 opened merge request !195

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 8.0.x-dev » 9.0.x-dev
Issue summary: View changes
Issue tags: +needs backport to 8.0.x

Created a 9.0.x merge request, which I will merge to first. Adding a tag so I don't forget to merge to 8.0.x.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue tags: +change record added

Added a change record; tagging so I remember to publish it.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Title: Add AlterSearchQueryParamsEvent to modify query parameters created by QueryParamBuilder » Add an event to modify query parameters created by QueryParamBuilder
Issue summary: View changes

I've cleaned up my own code a bit; apologies for the noise.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#3574791: Support for rule retriever queries.

This looks correct! And it will allow the rule retriever code to easily move to a sub-module!

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

Awesome, thanks! I'll merge this shortly and publish the change record.

  • mparker17 committed b8b7b993 on 9.0.x 
    feat: #3554964 Add an event to modify query parameters created by...
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 9.0.x-dev » 8.0.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -needs backport to 8.0.x

Merged to 9.0.x; merging to 8.0.x next.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Patch (to be ported) » Fixed
Issue tags: -change record added

Merged to 8.0.x and published the change record.

I will update this issue when this change is released!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

The changes in this issue were released in elasticsearch_connector-9.0.0-alpha3, and elasticsearch_connector-8.0.0-alpha7

Status: Fixed » Closed (fixed)

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