Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Silverstripe 5 #80

Merged
merged 10 commits into from
Mar 6, 2023
Merged

Add support for Silverstripe 5 #80

merged 10 commits into from
Mar 6, 2023

Conversation

chrispenny
Copy link
Collaborator

@chrispenny chrispenny commented Feb 15, 2023

RFC: Extract Elastic-specific classes into a separate module #81

This PR includes the removal of the Elastic integration, and this is now going to be provided by Silverstripe Search Service - Elastic module.

Breaking changes

  • The Elastic integration classes will all have a new namespace. Any references you have will need to be updated.
  • There have been some PHP 8.1 linting/declaration updates. If you have extended any of our classes, there might be some signature updates that you need to make.
  • EnterpriseSearchService::environmentizeIndex() is no longer static, and this is now part of the contract for IndexingInterface. If you were referencing or overriding this method, then you might need to update your implementation.
  • (Other than the above) there were no changes to processing logic.
  • No method constructors or method property orders were changed.

Silverstripe 5 support

There actually wasn't much that needed to change. The default property value in DataObjectFetcher was the most significant change.

@chrispenny chrispenny force-pushed the feature/silverstripe-5 branch 2 times, most recently from 80c306e to 33f3a6b Compare February 15, 2023 19:06
@chrispenny chrispenny force-pushed the feature/silverstripe-5 branch from 653601a to 655c155 Compare February 15, 2023 20:55
@chrispenny chrispenny force-pushed the feature/silverstripe-5 branch from 51e9c10 to b2039ec Compare March 5, 2023 21:42
@chrispenny
Copy link
Collaborator Author

@madmatt keen to get your thoughts on this. I still need to run through manual testing, but I think all major changes have now been made.

In particular, I really don't know anything about how Algolia configuration would work, and I think this module is (currently) quite opinionated in how it expects you to provide your index configuration.

CC: @GuySartorelli

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done a proper review as I don't have a way to run elastic atm but this looks like a great step in the right direction.
Once this gets merged it should probably have a beta release at first, to leave room for any breaking changes needed to let this work with services (e.g. algolia) other than elastic.

chrispenny and others added 2 commits March 6, 2023 12:34
Co-authored-by: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com>
@madmatt
Copy link
Member

madmatt commented Mar 6, 2023

Nice one, thanks @chrispenny! Overall it looks good, I agree with you about how index configuration is managed, perhaps I can work through that as part of the Algolia change when I get to it.

@chrispenny chrispenny marked this pull request as ready for review March 6, 2023 19:58
@chrispenny
Copy link
Collaborator Author

Manual testing completed in silverstripe/silverstripe-search-service-elastic#1. I'm about as happy as I'm going to get, considering I won't be able to test this on a real project for quite some time.

I'll look to merge this soon, and I'll tag this as a beta.

Thanks for all the help, team :D

@chrispenny chrispenny merged commit a6aa0b2 into master Mar 6, 2023
@chrispenny chrispenny deleted the feature/silverstripe-5 branch March 6, 2023 23:03
@chrispenny chrispenny restored the feature/silverstripe-5 branch March 6, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants