Thank you for your your work on this module. I've used it quite a bit!

I'm currently working on an event subscriber that alters the search parameters based on different criteria. In the event dispatchers, there is limited context available to the event for determining what triggered the search. In my example, I need the search id $this->query->getSearchId() for conditional logic.

Do you have thoughts on how this should be provided? I'm happy to write a patch to set the protected property on the event, but wanted to get feedback before doing so.

If I don't hear back, I may take a crack at it for review.

CommentFileSizeAuthor
#3 elasticsearch_connector-additiona_context_for_search_events-3075907-1.patch3.65 KBandyg5000
#2 elasticsearch_connector-additiona_context_for_search_events-3075907.patch3.48 KBandyg5000

Issue fork elasticsearch_connector-3075907

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

andyg5000 created an issue. See original summary.

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented
Category: Support request » Feature request
Status: Active » Needs review
StatusFileSize
new elasticsearch_connector-additiona_context_for_search_events-3075907.patch3.48 KB

I needed the patch anyway ;)

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented
StatusFileSize
new elasticsearch_connector-additiona_context_for_search_events-3075907-1.patch3.65 KB

First patch missed updating a call to new BuildSearchParamsEvent()

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

mparker17 opened merge request !120

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs issue summary update

I've created merge request !120 from the patch in #3, and fixed the merge conflicts. Then, I hid the patch files, because Testbot no longer tests patch files, and the patch file no longer applies cleanly. If you need an updated patch file, you can download one from the merge request. For what its worth, the team at Lullabot recommends using local copies of patch files: from personal experience, local patch files often makes composer run faster and more reliably.

I'm in the middle of preparing for a release, more specifically, ensuring that as many patches as possible will apply on the new release, i.e.: by resolving merge conflicts, i.e.: hopefully saving people time when the new release comes out.

This means I haven't had a chance to review the code. However, a brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back to "Needs work". Automated tests ultimately benefit you. They ensure that future changes (i.e.: by other contributors) will not break the functionality that you (or your client) depends on. If you need help writing tests, please ask (although I have a large backlog of tickets to work on, and I volunteer my time on the 8.x-7.x branch, so I cannot guarantee a speedy response - thanks in advance for understanding)!

We should also update the issue summary before release, so I've added the "Needs issue summary update" tag. An issue summary helps a maintainer understand why a change was necessary ("Problem/Motivation"), why you chose a particular solution ("Proposed resolution"), and how to test the change manually ("Steps to reproduce"). After an issue is fixed, a good issue summary documents what changed and what could be impacted ("User interface changes", "API changes", "Data model changes"), for people upgrading.

mparker17’s picture

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

Note that PHPCS is showing the following lints...

FILE: src/Event/PrepareSearchQueryEvent.php
-----
FOUND 1 ERROR AFFECTING 1 LINE
-----
 125 | ERROR | Public method name "PrepareSearchQueryEvent::getSearchAPIQuery" is not in lowerCamel format (Drupal.NamingConventions.ValidFunctionName.ScopeNotCamelCaps)
-----
FILE: src/Event/BuildSearchParamsEvent.php
-----
FOUND 1 ERROR AFFECTING 1 LINE
-----
 99 | ERROR | Public method name "BuildSearchParamsEvent::getSearchAPIQuery" is not in lowerCamel format (Drupal.NamingConventions.ValidFunctionName.ScopeNotCamelCaps)
-----

PHPCS wants these functions to be named getSearchApiQuery.

These functions aren't used in the patch. I assume @andyg5000 has a custom module or patch that uses these functions? If so, then I'll let @andyg5000 fix this lint, so that they can update their custom module or patch at the same time.