This patch will make this module work with a multi-domain site using the Domain Access module.

CommentFileSizeAuthor
#8 top_searches_domains.patch3.41 KBbob.hinrichs
top_searches_domains.patch2.85 KBbob.hinrichs

Comments

z.stolar’s picture

z.stolar commented
Status: Active » Postponed (maintainer needs more info)

Thanks! I didn't test the patch, but by reading it, it looks fine.
However - Is that the way other modules hook with domain access? Isn't it done with another sub module?
The reason I ask is that if DA is not installed/activated, then we have unneeded column and queries running...

bob.hinrichs’s picture

bob.hinrichs commented

You are correct. It sets up a column that is not used if you are not running multiple domains. I'm not sure how to make this work for multiple domains without it though. This may be beyond the scope of what you want to do, but probably the way to do it would be to create a sub-module for running this feature with domain access. This probably means creating hooks for your functions that interact with the database. Might not be too hard though?

z.stolar’s picture

z.stolar commented

Im not objecting the idea as a matter of principe. Im all for standards, and if thats the way other modules interact with DA, then this module can do it too. Do you already have this patch working on production somewhere?

bob.hinrichs’s picture

bob.hinrichs commented

Actually..um..hate to say, I have encountered a problem with it, having to do with a warning when it tries to insert a term that was searched on another site--the 'q' field is configured as a key. I haven't had time to try fixing it, it might be a simple matter of changing the field. I'll write in when I can fix it.

In terms of the correct way to do it, e.g. just have this extra column that is unused unless DA is in play, or some more sophisticated approach, I guess it's a little complicated to decide: 1) it is probably a very small % of people using it with DA this way, so adding the extra stuff makes it slightly more complex and will probably affect performance a bit. However, 2) changing the original module to have a hook-based approach (which also has a few performance consequences) and spinning this off into a sub-module will take more work, and I'm sure we're all pretty busy. A third option might be to just keep this as a patch (once I can get it to work properly).

bob.hinrichs’s picture

bob.hinrichs commented

Hi there Z!

I removed the index from the 'q' field and it eliminates the sql warnings (ALTER TABLE `top_searches` DROP INDEX `q`). A question for you is, since this is a requirement to using the domain id column, whether this compromises the module too much in terms of performance or data integrity, to use the DA-capable code as a native part of the module? If so, then it rules that out and we can just make this an optional patch, or use the more complex route of a submodule.

z.stolar’s picture

z.stolar commented

The index is there for a purpose - it makes things faster. Instead of dropping it, I would add the DA column to it...
However - maybe we should, at this point, use a patch which adds the DA column to the index (and adds the column itself), and of course - takes DA into account when inserting/reading.

We can publish the patch on the main page of the project, and specify it's compatibility to the main branch. Will you maintain the patch?

bob.hinrichs’s picture

bob.hinrichs commented

Yes that makes sense, add domain id as part of the q index. To confirm, in the patch, execute this in the update?

ALTER TABLE `top_searches` DROP INDEX `q`, ADD INDEX `q` (`q` ASC, `domain_id` ASC) ;

Yes, I can maintain the patch. thanks!

bob.hinrichs’s picture

bob.hinrichs commented
StatusFileSize
new top_searches_domains.patch3.41 KB

Here is a new one with the proper indexing. Module is in production and patch is tested as well.