Remove collective.indexing

gforcada commented 5 years ago

Since PLIP https://github.com/plone/Products.CMFPlone/issues/1343 Products.CMFCore has all the functionality that was provided on collective.indexing.

For collective.solr means that it can drop collective.indexing dependency (for Plone 5.1+).

This pull request does the minimal work that was required to be able to do so (basically replacing the imports and remove references to collective.indexing).

Some things that need discussion:

there are two failing test: testReindexIgnoreExceptions and testReindexPathIndex, which need a fix on CMFCore (I'm about to push that) it is related to the reindexObjectSecurity (@ale-rt your fix on master branch does not seem to work)
if this gets merged collective.solr will no longer be compatible with Plone 4.3.x nor 5.0.x, is that fine? We are already only testing against 5.1 right now...
the changes on maintenance.py seem to not have any effect on the test suite, from what I saw on the git log, this was added for the atomic changes back in #86 maybe someone from ftw can say something about it?

tisto commented 5 years ago

@gforcada just a quick reply. We can not drop Plone 4.3 compatibility yet IMHO. We have clients that are still on Plone 4.3 and I would like to use the latest c.solr for those. We could create a 8.x branch but I don't think this is worth the effort right now. The reason we are testing against 5.1 only is that we did a major refactoring and we could not fix all tests for all versions at the same time. I plan t make a first 7.x release soon, we need to fix a few things first though.

tisto commented 5 years ago

@gforcada this does not look too bad. We have two test failures on the PR (when running the tests against Plone 5.1):

Failure in test testReindexIgnoreExceptions (collective.solr.tests.test_server.SolrMaintenanceTests)
Traceback (most recent call last):
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/Users/timo/workspace/plone/collective.solr/src/collective/solr/tests/test_server.py", line 308, in testReindexIgnoreExceptions
    self.assertEqual(numFound(self.search()), len(DEFAULT_OBJS))
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 513, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 506, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 7 != 6

    61/85 (71.8%)

Failure in test testReindexPathIndex (collective.solr.tests.test_server.SolrServerTests)
Traceback (most recent call last):
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/Users/timo/workspace/plone/collective.solr/src/collective/solr/tests/test_server.py", line 580, in testReindexPathIndex
    self.assertEqual(search('+path_parents:\/plone\/news\/folder'), 0)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 513, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 506, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 1 != 0

tisto commented 5 years ago

@gforcada I have a pull request ready to be merged that works without c.indexing. Though, I do not understand where the collective.indexing "getOwnIndex" method ended up:

https://github.com/plone/collective.indexing/blob/e5dff54499763704ffb81caecebcfb2476e66b78/src/collective/indexing/indexer.py#L14

The test failures in my branch are exactly the same two failures that we see in your branch and that I mentioned above. I need c.solr running on Plone 5.1 for a client project and I honestly don't know how to fix this. Would you mind having a look?

tisto commented 5 years ago

@gforcada turns out the test failures are not related to the getOwnIndex function. Though, something in the c.indexing assimilation causes those failures...

reinhardt commented 5 years ago

Note that #229 works without c.indexing (and has green tests).

gforcada commented 5 years ago

I did close this pull request and Timo re-opened it, you could ask him what was his purpose and if he thinks he still needs it, I have no idea.

tisto commented 5 years ago

Closing in favor of #229

collective / collective.solr

Remove collective.indexing #206