search

collective / collective.solr

Solr search engine integration for Plone
https://pypi.org/project/collective.solr/
22 stars 46 forks source link

Python 3/Plone 5.2 #229

Closed reinhardt closed 5 years ago

reinhardt commented 5 years ago

I'll throw this out there for discussion - this is not (yet) fit to merge but it gets the test failures down to a handful and seems to work fine in a Plone 5.2 instance (no extensive manual tests yet, though). It contains the work by derFreitag and the plone-5.2-compat and collective-indexing-wip branches.

It still needs cross-checking in Python 2 and older Plone versions. That may also lead to a decision about whether to support all versions in one code base or to branch out, see #210.

I couldn't yet make sense of the failures of testReindexPathIndex and test_retry_on_conflict. Maybe we can have a chat about those at some point (@tisto?).

reinhardt commented 5 years ago

Travis is having trouble because it's not using the python 3 compatible version of plone.restapi. Locally I do - will try to port it to travis.

reinhardt commented 5 years ago

OK, that's better. Of the 6 failures/errors, two are probably caused by the same thing (the status 500 ones) and one seems easy to fix but needs a decision how to do it (showinsearch).

I haven't looked at the robot tests yet.

tisto commented 5 years ago

@reinhardt let's do a call next week, shall we? I'd be interested to push this further!

pbauer commented 5 years ago

I found out that this branch no longer works in Python 2. Is that intentional? Would fixes for Python 2 be welcome?

reinhardt commented 5 years ago

@tisto, a call sounds good, yes!

@pbauer, I've been concentrating on the python 3 side and have taken a few short cuts. I'm working on restoring python 2 support. Help is appreciated of course! We should coordinate a bit to avoid conflicts.

tisto commented 5 years ago

@reinhardt could you send me your email address to tisto@plone.org? I will be on holidays next week. Maybe we can find a time in the next days to do a call.

reinhardt commented 5 years ago

OK, a02e2bb gets all unit tests green for me locally. Let's hope travis agrees.

For the robot tests I'll have to look into setting up chromedriver, which I might postpone for a bit.

reinhardt commented 5 years ago

Well, that wasn't too hard. All green now!

tisto commented 5 years ago

@reinhardt w00t!!!

tisto commented 5 years ago

I forgot to mention that I tried Solr/Plone 5.2 locally and everything works like a charm as far as I can tell after a quick review.

We would use this in two upcoming projects (event though, we don't need to Python 3 / Plone 5.2 support yet). Just to make sure we can help to find issues as early as possible...

reinhardt commented 5 years ago
  1. Squashing would be nice I guess, yes. I think I can invest a little more time and do that.
  2. I'd like to double check, but the tests do pass in both the Plone 4 and Plone 5 setups. Maybe c.indexing gets pulled in some other way?!
  3. Can't think of any. :-)
reinhardt commented 5 years ago

Glad I checked - the travis config is botched. The Plone 4 job is actually installing Plone 5. I previously replaced the Plone version in buildout.cfg, unaware that the travis script was looking for this specific version. I'll make the substitution more robust.

reinhardt commented 5 years ago

The different Plone versions are now properly tested. I've fixed the missing collective.indexing on travis and also an ImportError in the tests for Plone < 5.2.

Plone 4.3 has the problem that it doesn't support the ISolrFields behavior. We could restore the old Archetypes schemaextender and make both registrations conditional. However, the GenericSetup step that activates the behavior for the default types breaks on Plone 4 and I'm not sure how best to make that one conditional. We could copy the default profile and conditionally register the correct one depending on the Plone version. That's of course a pain to maintain but if we aim to drop support for Plone 4 soon(-ish) then we could maybe put up with that for a while. What do you think?

pbauer commented 5 years ago

This branch is still broken with the version of plone.restapi that ships with Plone 5.2.

reinhardt commented 5 years ago

Thanks for the hint, I forgot that it's still pinned in our own versions.cfg...

tisto commented 5 years ago

@reinhardt would be nice if we could keep Plone 4 compatibility. Then I could test the new release with another project of ours. Otherwise, I'd have to create a branch and stick with the old one, which is also fine with me. I leave that decision up to you...

reinhardt commented 5 years ago

Is it OK to just remove the SolrFlareSerializer? The tests pass without it, and its base, the BrainSerializer, has been removed as well. Both should be automatically replaced by the summary serializer. But I'm not familiar with plone.restapi and would be grateful if someone could double check.

If that's fine then only the rebase/squash remains to do, and maybe another pair of eyes on this branch.

reinhardt commented 5 years ago

OK, now the travis builds are really green. :-D

tisto commented 5 years ago

@reinhardt Carsten could have a second look if nobody else steps up. He will be back tomorrow. I would squash and merge then and do a 8.0.0a1 release right away (just in case we might need the 7.x branch, which I hope we don't).

Regarding the SolrFlareSerializer, I'd like to hear @sneridagh's opinion since he added that. I don't recall our use case but we need to double check if c.solr works with p.restapi and Volto. Here is the original PR:

https://github.com/plone/plone.restapi/pull/767/files

tisto commented 5 years ago

@reinhardt I merged this PR because we would like to test the branch in our non-py3 projects and we need another PR for that: https://github.com/collective/collective.solr/pull/220.

We might still have to solve the p.a.contenttypes issue but after that we can cut an alpha release.