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

DEP Move solr-php-client with PHP 8.1 compatbility to thirdparty folder #320

Closed

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Apr 19, 2022

Issue silverstripe/silverstripe-framework#10250

I attempted to update this upstream but got no response. Looks abandoned. So just adding to thirdparty folder instead.

The COPYING.md file says it's ok to copy this, as long as copyright is retained

Includes a fix to add method attribute to Document::getIterator() for PHP 8.1 compatibility

@emteknetnz emteknetnz marked this pull request as ready for review April 20, 2022 05:07
@dhensby
Copy link
Contributor

dhensby commented Apr 20, 2022

Potentially time to migrate to https://github.com/solariumphp/solarium ? I think I looked into this quite some time ago, not sure what happened to it, though.

@emteknetnz
Copy link
Member Author

That seems like substantially more work. There was only a single line required to get this php 8.1 compatible (at least according to unit tests).

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've checked, and the only difference between thirdparty/solr-php-client and the v1.0.0 tag of the package in github is the change made in the PR referenced in the description.

This module honestly feels a bit legacy at this point - personally I'm okay with taking the less-effort approach in this case, but given there may be some controversy I'll leave it for someone else to merge.

@@ -154,6 +155,9 @@ protected function getSource()
*/
public function process()
{
if (!DB::is_active()) {
Copy link
Member

Choose a reason for hiding this comment

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

What prompted this? It doesn't seem related to the rest of the PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

PR that went into 3.9 though wasn't merged up - #318

@dhensby
Copy link
Contributor

dhensby commented Apr 21, 2022

Yeah, I think that moving to solarium should be a long-term project and not a way to solve php 8.1 compatibility. We should probably aim not to be reliant on libraries that are abandoned or haven't seen any effort in 6 years!

Copy link

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

My preference would be to do a sminee/phpunit and fork the repo on the Silverstripe domain and stick a big nasty warning at the top that it's not supported. But this solution could work as well.

FYI Silverstripe Ltd has moved on to using Elastic for search in its new project. We'll keep maintaining this module because a lot of people are still using it, but it's unlikely we'll update it to work with the later version of Solr.

@@ -24,7 +24,6 @@
"php": "^7.4 || ^8.0",
"silverstripe/framework": "^4.10",
"monolog/monolog": "~1.15",
"ptcinc/solr-php-client": "^1.0",

Choose a reason for hiding this comment

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

If we decide to include the ptcinc/solr-php-client into this module, the composer file should have a provide key to make explicit that this other packages has been bundled in.

If we decide to fork the module, the fork should have the provide key.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have sufficient permissions to fork to the silverstripe account. If you'd like to go this route could you please fork it.

Copy link
Contributor

Choose a reason for hiding this comment

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

fork here: https://github.com/silverstripe/solr-php-client - I can delete it if we decide we don't want it

@GuySartorelli
Copy link
Member

FYI

I was the primary author. This was created for a product which no longer exists, but was acquired by PTC. There is unlikely to be any changes here as I am no longer part of PTC. Anyone still using this probably forks to publish or vendor their own version. Sorry for your inconvenience.

@emteknetnz
Copy link
Member Author

Thanks for forking the module @dhensby - we'll use that. I've created a follow up PR to fix a couple of things on the fork

@emteknetnz emteknetnz closed this Apr 25, 2022
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.

4 participants