Refactor .module logic

Following are the methods needs to be moved to utility service, working on it will update and move in review.

• _searchstax_add_tracking
• _searchstax_search_alter_solr_query
• _searchstax_is_searchstax_solr
• _searchstax_get_query_keys
• _searchstax_stringify_complex_keys

All methods now added to service, waiting for 📌 Test Coverage: Fix PHPCS warnings. Active to land.

The changes from 📌 Test Coverage: Fix PHPCS warnings. Active have been merged. There are a few related warnings, plan to address shortly. Currently awaiting the merge of 📌 Test Coverage: Fix cspell warnings. Active . Once that is complete, we will proceed with adding the test coverage for the service.

Waiting for 📌 Test coverage Active to land.

Requesting review.

drunken monkey → made their first commit to this issue’s fork.

Thanks for your work on this!
I made a few updates and created a change record → .

There’s one more thing before I can merge this, though: The new service should have an interface that defines all the methods it provides, the implementations should then just be documented with {@inheritdoc}. Then only this interface should be used for any type-hinting. (It seems we should also add the interface FQN as an alias for the service identifier, to be in line with how Core does it?)
Could you please make that final change?

Otherwise, this looks good now, thanks again!

Thank you for the review, will take a look and update accordingly.

I now added the interface, and also renamed the service class since SearchStaxUtilityService was a bit verbose.

However, I also thought of something else: Does it really make sense to have alterSolrQuery() in the utility service if there is already another service, SearchSubscriber, which has a method just forwarding to it? Wouldn’t it make more sense to just have the whole code for this in the SearchSubscriber class itself and just call that service from the Solr query alter hook?
Or is there a good reason not to do that? (I’d then also add an interface for the SearchSubscriber service that covers that method.)

Finally, I’m not sure stringifyComplexKeys() needs to be public (and, thus, on the interface) as it’s really only used internally by the service class. Would it be OK with you if we made it protected?

I believe we should continue keeping it in the service, as it might be beneficial in other ways in the future, although I don't have a strong reason for this approach at the moment.

I also thought of something else: Does it really make sense to have alterSolrQuery() in the utility service if there is already another service, SearchSubscriber, which has a method just forwarding to it? Wouldn’t it make more sense to just have the whole code for this in the SearchSubscriber class itself and just call that service from the Solr query alter hook?

Agree with you on making stringifyComplexKeys(), protected as it's used internally.

Finally, I’m not sure stringifyComplexKeys() needs to be public (and, thus, on the interface) as it’s really only used internally by the service class. Would it be OK with you if we made it protected?

OK, then let’s keep it in the service, I have no strong feelings either way. I just changed the method visibility and then merged.
Thanks again!

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