Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.This patch will make this module work with a multi-domain site using the Domain Access module.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | top_searches_domains.patch | 3.41 KB | bob.hinrichs |
| top_searches_domains.patch | 2.85 KB | bob.hinrichs |
Comments
Comment #1
z.stolar CreditAttribution: z.stolar commentedThanks! 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...
Comment #2
bob.hinrichs CreditAttribution: bob.hinrichs commentedYou 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?
Comment #3
z.stolar CreditAttribution: z.stolar commentedIm 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?
Comment #4
bob.hinrichs CreditAttribution: bob.hinrichs commentedActually..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).
Comment #5
bob.hinrichs CreditAttribution: bob.hinrichs commentedHi 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.
Comment #6
z.stolar CreditAttribution: z.stolar commentedThe 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?
Comment #7
bob.hinrichs CreditAttribution: bob.hinrichs commentedYes that makes sense, add domain id as part of the q index. To confirm, in the patch, execute this in the update?
Yes, I can maintain the patch. thanks!
Comment #8
bob.hinrichs CreditAttribution: bob.hinrichs commentedHere is a new one with the proper indexing. Module is in production and patch is tested as well.