Problem/Motivation

For the same reason as ticket #3454606: Add query object to QueryParamsEvent., I'd need to alter some boost to certain searches, based on data that has been added to the search_api query object by a query processor.

Proposed resolution

Pass the query to \Drupal\elasticsearch_connector\Event\QueryParamsEvent, so that listening event handlers have access to it.

Remaining tasks

  1. Write initial patch — done for 8.0.x by @drdam in merge request !99 and for 9.0.x by @mparker17 in merge request !182
  2. Add tests — done by @drdam by #13
  3. Review and feedback — done by @mparker17 in #14
  4. RTBC and feedback — done by @mparker17 in #15
  5. Commit to 9.0.x — done by @mparker17 in #19
  6. Commit to 8.0.x — done by @mparker17 in #21
  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.

API changes

When constructing a \Drupal\elasticsearch_connector\Event\QueryParamsEvent, you must now pass a \Drupal\search_api\Query\Query to the constructor.

Adds \Drupal\elasticsearch_connector\Event\QueryParamsEvent::getQuery() : \Drupal\search_api\Query\Query

Data model changes

None.

CommentFileSizeAuthor
#2 3539863_addQueryObjectToQueryParameterEvent-2.patch2.03 KBdrdam
addQueryObjectToQueryParameterEvent.patch2.03 KBdrdam

Issue fork elasticsearch_connector-3539863

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

drdam created an issue. See original summary.

drdam’s picture

drdam commented
StatusFileSize
new 3539863_addQueryObjectToQueryParameterEvent-2.patch2.03 KB

Fix patch

mparker17 made their first commit to this issue’s fork.

mparker17 opened merge request !99

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 8.0.0-alpha4 » 8.0.x-dev
Issue summary: View changes
Status: Active » Needs work
Issue tags: +Needs tests

@drdam, thank you for the contribution!

I committed your code to a merge request (and updated the issue summary) to make it easier for me to review.

I changed the version to 8.0.x-dev because this issue applies to the latest development version as well.

I took a very quick look at your patch: it looks good to me (I anticipate some phpcs notices about documentation, but I expect that will be easy to fix).

Thanks for tagging this with "Needs tests"! Is there anything that I can do to help with writing tests for this change?

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

I notice that Testbot is detecting a failing test.

Fortunately, I think it should be pretty easy to resolve: the code that constructs a new QueryParamsEvent in \Drupal\Tests\elasticsearch_connector\Unit\SearchAPI\Query\QueryParamBuilderTest::testBuildQueryParams() will need to be updated to pass a Query (or a test double for a Query).

mparker17’s picture

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

(small update to issue summary)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

Quick note, I ran phpcbf on the code, and it fixed the phpcs errors.

mparker17’s picture

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

Also, we should add or update a change record to assist people upgrading from 8.x-7.x. Note we have a draft, not-complete-yet change record with some information on events that existed in the 8.x-7.x and their equivalents in 8.0.x (if applicable) — not sure if we need to update that, or create a new change record.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

(sorry for the noise: I rebased onto the latest 8.0.x, so that the phpstan pipeline wouldn't warn us about a deprecation that was fixed in 8.0.x)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

Briefly checking the status of this issue after releasing 8.0.0-alpha5...

  1. The existing tests are failing, indicating a change in behavior. Essentially, there are two paths forward: update the tests, or fix the regression. I apologize that I haven't ported this documentation to Elasticsearch Connector yet, but I wrote some documentation for the Sitemap module that explains this in more detail.
  2. The new code path also doesn't have any tests, and it will need a test before I can merge it.
  3. There are some phpstan lints to fix
  4. We should write a change record for this change
drdam’s picture

drdam commented

The error are on the test who send a queryParamsEvent, need the new query param.

I'm fighting with phpunit and prophesize ....

drdam’s picture

drdam commented
Status: Needs work » Needs review
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

@drdam apologies for the delay in reviewing. This looks good. I've made a small tweak to use QueryInterface consistently in QueryParamsEvent (i.e.: instead of a Query sometimes). If tests pass, I'll create a 9.0.x merge request, merge it, then merge your merge request. Thank you very much for the contribution, and your patience with me!

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 8.0.x-dev » 9.0.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -Needs change record

@drdam wrote a test and I've added https://www.drupal.org/node/3576752 as a change record, so those tags can be removed.

The tests also still pass after I rebased yesterday and changed Query -> QueryInterface in the event, and everything looks good to me, so I'm going to mark this as RTBC and proceed with merging to both 9.0.x and 8.0.x.

mparker17 changed the visibility of the branch 9.0.x to hidden.

mparker17 opened merge request !182

mparker17’s picture

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

Updated the issue summary for record-keeping purposes

  • mparker17 committed 0ac74b60 on 9.0.x 
    feat: #3539863 Add query object to QueryParamsEvent

By: drdam
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)

Merging to 8.0.x next.

  • mparker17 committed 3d4d1be1 on 8.0.x 
    feat: #3539863 Add query object to QueryParamsEvent

By: drdam
By:...
mparker17’s picture

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

Merged to 8.0.x. I will update this issue when this change is released. Thanks again for your patience!

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.

Status: Fixed » Closed (fixed)

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

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