- Issue created by @godotislate
- Merge request !14Issue: #347566: Dispatch events when processing resource request that allows... → (Open) created by godotislate
- Status changed to Needs work
4 months ago 10:22pm 19 September 2024 - Status changed to Needs review
3 months ago 5:32pm 20 September 2024 Added tests. The failures are against the previous core major version (9.5.11) and seem to be unrelated to changes here.
- 🇮🇱Israel jsacksick
Do we really need two distinct events? Isn't 1 sufficient? (Not saying we have to have one, just asking)
- 🇺🇸United States mglaman WI, USA
I think one event should be sufficient that allows modifying the filters and sorts before they're applied.
- 🇮🇱Israel jsacksick
Note that if we use property promotion, we need to bump the minimum PHP requirement to 8.
Also, this defines a new module jsonapi_search_api_test_subscriber, let's rename it to just jsonapi_search_api_test so we can reuse the test module in the future if needed.Due to the recent changes, this needs to be rebased as well, but other than that, looks clean and thanks for the changes :).
Rebased and changed the name of the test module to
jsonapi_search_api_test
.Note that if we use property promotion, we need to bump the minimum PHP requirement to 8.
A couple options here:
- Add a PHP version constraint in composer.json. Drupal 9.1 supports PHP 8, so
core_version_requirement
in the info.yml wouldn't need to be changed, but people running D9/PHP 7 would need to update OR - Drop Drupal 9 support from info.yml, since Drupal 10+ requires PHP 8 OR
- I can remove property promotion from the MR if this is out of scope for this issue
Do we really need two distinct events? Isn't 1 sufficient? (Not saying we have to have one, just asking)
I think one event should be sufficient that allows modifying the filters and sorts before they're applied.
The use case on our project was filter integration with search_api_location, similar to ✨ Provide a mechanism for supporting search_api_location Needs work , but with some customization specific to the project. The DX I was going for was to be able to access the
$filter
query param array values directly before the search API query filters were built. It becomes more complicated to alter the filter/conditions once built byFilter::createFromQueryParameter()
, because you have to loop through the array to find the relevant condition.Sort is built slightly differently. I don't recall now why I have the sort event dispatched after
Sort::createFromQueryParameter()
and not on on the query parameter array, but I believe having the sort event manipulate the query parameter array had its own downsides. To be honest, sort was not a use case we needed on our project. I included it in this issue because it seemed natural to support altering both filters and sorts. But maybe altering sort can be punted out of this issue.I can change it to dispatch one event in
IndexResources::Proces()
before executing the query, but that seems redundant to Search API's ownQueryPreExecuteEvent
. Alternatively, we could use the same event object class dispatched in two separate places, but then getters/setters for filter/sorts would be irrelevant depending on whether the event was dispatched inapplySortingToQuery
orapplySortingToQuery
- Add a PHP version constraint in composer.json. Drupal 9.1 supports PHP 8, so