Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Hello, and thanks for a wonderful module. Makes it so easy to get started with having the solr search multilingual.
I just tested the upgrade to 1.1 and it seems the module is now broken for me. From what I can see this is related to this commit: http://cgit.drupalcode.org/apachesolr_multilingual/commit/?id=139a93852d... (#2307367: make the module compatible with solr attachment). Here you set a variable called documents to equal new empty array, thus overwriting the parameter (passed by reference) called $documents.
It is not 100% clear to me why that code is there, but it sure breaks the indexing. The only document that's getting indexed is the last on in the $additional_documents_langcodes array. I would be happy to provide a patch for this, but as I said - I don't even know what that code is supposed to do.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 2315085.patch | 1.61 KB | mkalkbrenner |
Comments
Comment #1
mkalkbrennerSeems like a copy&paste bug. Can you give this patch a try and report quickly if it works?
Comment #3
mkalkbrennerThe bug seems to occur if you index multiple documents. It's not visible for a single document or using the devel module for a node.
Comment #4
eiriksmHey, thanks for that fix! That was really quick!
It also works great, as expected.
I have a couple of suggestions though (although I see you have already commited the patch):
- The code comments about
Generic use case for future reference. Callbacks can allow you to send back multiple documentsis now duplicated. IMO it belongs better where you call the callback, than where you explain that $tmp_documents is not in use.- Speaking of which, it still is not clear to me what $tmp_ducuments should be used for ("in a future use case"). Could there be a code comment about this?
- Since this is a regression in the newest version, it should probably be a test for it. I'll volunteer to write it if that makes sense.
- Also, since this makes the "recommended version" kind of unusable (at least for me). Would you consider releasing a new version for this fix so I don't need to be stuck at either -dev or old version?
Thanks again, have a great day!
Comment #5
mkalkbrennerI already released 7.x-1.2 and backported the fix to 6.x-3.x. I just waited for your feedback.
Currently the code makes no sense at all. The code and the comment is copied from function apachesolr_index_node_solr_document() in apachesolr.index.inc.
For every language specific document apachesolr_multilingual creates, we have to call the registered indexing callbacks, p.e. apachesolr_attachments registers such a callback. The API of apachesolr allows these callbacks to directly modify a document and to additionally return multiple new documents. But I don't know a single implementation of such a callback that makes use of the feature to return multiple new documents.
Right now I have not started to deal with this "feature" provided by apachesolr but I know that I might have to in the future. Therefor I decided to already collect these documents but not to handle them yet. From my point of view it makes no sense to increase the complexity by implementing a "future use-case".
What's your opinion?
In general it's a good idea to increase the number of tests! I'm looking forward to see your contribution and promise to integrate them quickly.
Comment #6
eiriksmI agree. In fact, I would almost say it makes sense to not even invoke the hook. Seems like a "waste of CPU cycles" :)
I took a look at the tests last week to extend them with this case. However they were a bit strange in the way that they require another couple of modules, without even explicitly saying so anywhere. And they live in the confgen module as of now. However, I will see if I can make a more general test regarding indexing specifically if I get the time one of these days.