[WIP] Plone 5 compatibilty #78
Conversation
|
It's OK for me. (deleting plone5 branch ) |
Not only because plone.org/products will die soon, but also because it's so useful to have this URL show up on pypi.
Conflicts: setup.py solr.cfg src/collective/solr/exportimport.py src/collective/solr/interfaces.py src/collective/solr/tests/test_server.py
|
@tomgross I would love to see this getting merged. Do you see any chance to rebase? If you don't have time to work on this, maybe someone else can take over. We could also think about organizing a chipin to fund this... |
Conflicts: src/collective/solr/testing.py
|
Hi @tisto this is almost done and works for our Plone 5 installation. The biggest issue ar the test failures resulting from the move of the configuration to plone.app.registry. Any help is appreciated here. |
|
@tomgross that's good to hear! I briefly looked into this and it seems that the connection settings that are stored as unicode lead to problems with Solr in the test setup. I tried to add a few encode/decode statements but that did not help so far. Strange that this does not seem to happen in real life though. |
|
@tomgross I would like to put some effort into fixing this. Is it ok if I work on your branch or do you prefer if I create my own branch? |
|
Hi @tisto It is perfectly ok to work on my branch 👍 |
…r Plone < 5." This reverts commit 77bf7a3.
…uring Plone site creation. Make sure solr is always active during the tests though.
|
|
||
| logger = getLogger('collective.solr.manager') | ||
| marker = object() | ||
|
|
||
|
|
||
| class BaseSolrConnectionConfig(object): |
There was a problem hiding this comment.
@tomgross why did you remove the SolrConnectionConfig? We need this to support multiple Plone instances in one Zope.
There was a problem hiding this comment.
@tisto Configuration moved to the registry. There is no need for a separate way to config solr and the registry is relative to a Plone instance which should allow multiple Plone instances in on Zope. Did I miss something?
There was a problem hiding this comment.
@tomgross in theory yes, in practice no. Things are way more complex than that. By removing the ISolrConnectionConfig you also removed the IZCMLSolrConnection config lookup. This removal requires a considerable refactoring both in the code and in the test setup.
I tried to mock the registry lookup for the unit tests, but this did not work out... :(
There was a problem hiding this comment.
Do we really need this complexity? I mean it is just a connection between a CMS and an index based on an api (collective.indexing). It shouldn't be that hard. PloneIntranet moved away from collective.solr because of this complexity.
``plone.recipe.zope2instance.ctl`` and ``zopectl.command`` provide totally different arguments to the entry point methods.
…is always properly loaded in the test setup.
|
@tomgross @do3cc I further investigated the current test failures in this branch. There are two kind of failures:
There are a lot of test isolation issues here. I started to just comment out tests that bleed into other tests in order to find the real test failures. I'm down to two test failures. Though, even after fixing those, the isolation issues will remain. Some of them might have been triggered by the plone registry refactoring but I think a fair amount have nothing to do with this pr actually. Maybe we should start cleaning up the master branch before moving to this pr...
This can be solved by mocking the registry lookup. I did some tests that look promising but I haven't found the right level of mocking yet. This is doable though. The good thing so far is that expect one problem with the "active" setting, I did not find a problem in "real life". |
|
What do you mean with plone.app.testing refactoring? The changes in plone.testing? They may fail now while they weren't failing earlier, but this is only, because test isolation was broken silently before. |
|
Superseded by: #126 |
Another attempt to make c.solr compatible with Plone 5. Some tests are still failing but I think this is the best what we have at the moment. Any takers to help fixing the tests in Plone 5?
@karalics @do3cc Are you good to delete the plone5 branch? All the relevant parts have been moved and it is kinde of messed up.