MAJOR: Move to new Enterprise Search dependency

[chrispenny]

Fixes #71
Fixes #62
Replaces #70

Manual Testing

• Configure dev task
• ReIndex dev task
  • From 0 documents
  • With existing documents
• ClearIndex dev task
  • Requires index name to be provided
  • Clears index once provided
• Index (add) through Page publish
• Index (add) through Asset publish
• Index (remove) through Page un-publish
• Index (remove) through Asset un-publish
• Search Service model admin
  • Permissions: Admin can do it all
  • Permissions: Check CMS Access denied
  • Permissions: Check ReIndex denied
  • Permissions: Check Reindex allowed

Enterprise Search and PHP 8.1

elastic/enterprise-search-php#25

Focus area

EnterpriseSearchService and EnterpriseSearchServiceTest

This is where most of the effort was spent.

I've completely refactored the way we perform tests. We now mock the responses from the API, rather than mocking responses from the method as a whole.

Lots of refactoring where I've broken single methods down into separate parts to help make testing easier (and more robust).

Document creation

Other than fixing linting, and trying to remove "complexity" (EG: by switching to "exit early" strategies), I haven't changed much (or anything) about how we create DataObjectDocuments.

To be honest, it's all confusing to me... I tried to stay away from this area as much as possible.

Breaking changes

PHP

Minimum PHP requirement has been increased to PHP 8.

Because this was going to be a breaking change anyway, it seemed like a good time to start adding PHP 8 supported type hints, etc.

Environment variables

Usages of the name APP has been replaced with ENTERPRISE.

Before:

APP_SEARCH_ENGINE_PREFIX=""
APP_SEARCH_API_KEY=""
APP_SEARCH_API_SEARCH_KEY=""
APP_SEARCH_ENDPOINT=""

After:

ENTERPRISE_SEARCH_ENGINE_PREFIX=""
ENTERPRISE_SEARCH_API_KEY=""
ENTERPRISE_SEARCH_API_SEARCH_KEY=""
ENTERPRISE_SEARCH_ENDPOINT=""

Method return types

Some methods previously returned void or $this, which meant they didn't give any useful feedback from the request we had just made.

Methods like EnterpriseSearchService::configure() now return info about the response (in this case, an array of the active configuration for each index).

Methods with changed return types:

• IndexingInterface::configure()
• IndexingInterface::addDocument()
• IndexingInterface::removeDocument()
• BatchDocumentInterface::addDocuments()
• BatchDocumentInterface::removeDocuments()

Pretty much every property/parameter everywhere

So, I think I've been able to keep the general API for all methods (that being, the order and purpose of params), but a lot of methods have had type declarations added, so if folks are extended our classes and overriding methods, then these would likely be breaking changes for them.

Where possible, properties have been given type hints (where they didn't previously). Exceptions are the usual properties that extend some other vendor classes that they have to match.

NaiveSearchService

Practically, I don't think anyone would have been accessing this class, but the namespace has changed to be consistent with other classes.

Before: SilverStripe\SearchService\Services\Naive\NaiveSearchService
After: SilverStripe\SearchService\Service\Naive\NaiveSearchService

Maybe breaking

Default HTTP Client

I don't think this will be a breaking change, as the old App Search already explicitly used Guzzle as its HTTP Client. However, the new Enterprise search uses PSR-18 "discovery". I've found this to be a bit too fragile (EG: It was breaking for me with Symfony Client), so I've added a new config to our ClientFactory to allow folks to specify what client they would like instantiated. Default is Guzzle.

SilverStripe\Core\Injector\Injector:
  Elastic\EnterpriseSearch\Client:
    factory: SilverStripe\SearchService\Service\EnterpriseSearch\ClientFactory
    constructor:
      # original configs
      host: '`ENTERPRISE_SEARCH_ENDPOINT`'
      token: '`ENTERPRISE_SEARCH_API_KEY`'
      # new config
      http_client: '%$GuzzleHttp\Client'

recordBaseClass and pageContent

We had two conflicting configs, one defined these fields in snake case (EG: record_base_class), and the other defined them in camel case (EG: recordBaseClass). I've standardised all configs to snake case, as that is consistent with other field names.

Looking through some of my own projects, we seem to use the snake case, so this might not be a breaking change.

New features

New Permission for triggering ReIndex through Service Admin

SearchAdmin_ReIndex permission has been added so that it's not longer only ADMIN who are able to trigger a full ReIndex through the Model Admin area.

How sad that this is my only new feature...

[andrewandante]

Wow what a ride. A few comments, nothing major at all though, excellent work!

Wondering whether it would be wise to provide an upgrade guide of some sort? Mostly it'd just be swapping AppSearch for EnterpriseSearch all over the shop, and updating the ENV vars, but could still be worth adding to the Readme? Maybe just the changelog when the major is tagged? Not sure what the recommended steps are there.

[chrispenny]

Docs regarding the changes for v2:

f35b509

[andrewandante]

Magnificent. LGTM!