silverstripe / silverstripe-search-service

A service-agnostic search module for Silverstripe CMS
BSD 3-Clause "New" or "Revised" License
5 stars 18 forks source link

Update dependencies and refactor to allow for PHP 8.0 #69

Closed chrispenny closed 2 years ago

chrispenny commented 2 years ago

PHP Unit 9

We need to upgrade PHP Unit in order to get PHP 8 support. The withMethods() method was deprecated in 5.6. The onlyMethods() is the closest I can find to a replacement (I'm hoping it's correct).

Significant (ish) changes

IndexJob was previously setting the Indexer to a property during __construct(). This means that the Indexer is saved as part of our jobData. Queued jobs then uses serialize($jobData) as part of the Job Signature.

Unfortunately, from PHP 8, our Indexer can no longer be serialised. This is because it includes Guzzle, and Guzzle includes CurlHandle, and CurlHandle cannot be serialised.

The solution is to store the "pagination" information is jobData (rather than the Indexer) and then we can just instantiate the Indexer during process().

"Forks"

I had to republish 2 abandoned modules in order to increase the platform requirements to allow for PHP 8. I want to make it really clear that I have no intension of maintaining these modules in any way (in fact I've already abandoned them).

If I had used forks rather than publishing, then every dev using this module would also have to define these forks in their project's composer.json (because Composer doesn't fetch forks recursively), and I didn't think that that would be a great experience.

Manual testing

chrispenny commented 2 years ago

Thanks for the feedback, @emteknetnz! I've actioned all of that. 🤞 tests come back green 😄

chrispenny commented 2 years ago

Tests are now green. I'll run through all of my manual tests again tomorrow and re-mark them as complete once done.

chrispenny commented 2 years ago

@emteknetnz I'm having some issues after defining the properties on the Job classes.

It seems that if the properties are defined (and I've tried private/protected/public) then we get no Job data, but if the properties are left as dynamic, then we do get Job data.

Defined: Screen Shot 2022-06-17 at 07 21 05 Screen Shot 2022-06-17 at 07 21 17

Dynamic: Screen Shot 2022-06-17 at 07 21 31 Screen Shot 2022-06-17 at 07 22 13

Unless there's just something obvious that I'm missing, and considering I'm trying to get this across the line due to it being a blocker for PHP 8.0 and 8.1: Could this please be something we raise for PHP 8.2? Potentially it's also something that QueuedJobs will need to look in to?

chrispenny commented 2 years ago

@emteknetnz I've raised the question on the Queued Jobs module: https://github.com/symbiote/silverstripe-queuedjobs/issues/377

I've also completed another round of my manual testing, and I'm happy that everything in our project is still functioning.

emteknetnz commented 2 years ago

OK fair enough, keep the dynamic properties

I've posted a follow up comment https://github.com/symbiote/silverstripe-queuedjobs/issues/377#issuecomment-1158178650 tl;dr we need to release a new major of queuedjobs to support non-dynamic properties, which we will need to do for PHP 9

emteknetnz commented 2 years ago

Have chatted offline, we'll prioritize PHP 8.0 support and and leave the PHP 8.1 deprecation warnings, for now