The current search block appears on all pages, not just the assigned search page, when there are two search form blocks. I believe this is because the check for visibility (current_search_check_visibility in current_search.block.inc) is being done incorrectly and ignore whenever there is already a cached version of the value, regardless of the cached version of the file.

I think the right thing to do is to store whether the block is visible in the static cache. I've done this by just adding a second cached value there, "is_visible", in addition to the searcher.

I've attached a patch that resolves the problem against dev for me and I think seems sound. Please let me know if you think Im off track or if changes are needed.

Thanks!

CommentFileSizeAuthor
#9 multiple-current-search-block-1668980-9.patch4.63 KBcpliakas
#8 multiple-current-search-block-1668980-8.patch2.96 KBcpliakas
#1 facetapi-fix_for_multiple_search_blocks-1668980-1.patch5.02 KBsnyderp

Comments

snyderp’s picture

snyderp commented
StatusFileSize
new facetapi-fix_for_multiple_search_blocks-1668980-1.patch5.02 KB

Here is a patch for the above problem against the 7.x-1.x-dev branch. Hope it helps!

cpliakas’s picture

cpliakas commented
Status: Active » Needs review

snyderp,

Thanks for the contribution. Marking it as needs review so the test bot can hack at it.

Also, could you provide steps to reproduce the bug? I am not clear on the conditions causing the issue.

Thanks!
Chris

snyderp’s picture

snyderp commented

Hi Chris,

The issue comes up when you have to search blocks on the same page. The hook_block_view hook gets called on the first one, and there is no statically cached info for the block, so the if ($map === NULL is true and we get to ask if the block should be visible.

For the second check though, $map is no longer null (it has keys for both search blocks) so we never end up asking if the block should be visible. The structure of the control statements causes this to fall through to being visible.

The patch fixes this so we check and cache if a block should be visible on a per request / block basis, instead of just once.

Hope that helps! Please let me know if I can clarify further!

Pete

cpliakas’s picture

cpliakas commented

Thanks, Pete. Regarding the two search blocks, is this configured via Search API?

snyderp’s picture

snyderp commented

No, they're just standard search block forms (so, one using the standard, default, search module block, and the other another programmatically inserted version of the same)

cpliakas’s picture

cpliakas commented

I gotcha now. Let me see if I can replicate.

cpliakas’s picture

cpliakas commented
Assigned: Unassigned » cpliakas
Priority: Normal » Critical
Status: Needs review » Needs work

I can't replicate the exact issue, but I can confirm this is definitely broken. I am going to take this one because there is some cleanup I want to do here based on legacy code.

Thanks for reporting,
Chris

cpliakas’s picture

cpliakas commented
Status: Needs work » Needs review
StatusFileSize
new multiple-current-search-block-1668980-8.patch2.96 KB

The attached patch refactors the code a bit. Let me know if it resolved your issue or not. I also want to add some tests for the Current Search Blocks module to prevent issues like this from popping up.

cpliakas’s picture

cpliakas commented
StatusFileSize
new multiple-current-search-block-1668980-9.patch4.63 KB

Re-rolled patch against HEAD, added tests building on the patch committed at #1671154: Add tests for the Current Search Blocks module.

cpliakas’s picture

cpliakas commented
Status: Needs review » Fixed

I am confident this change will fix the problem, and I want to get this in. Please feel free to re-open if it doesn't fix the issue. Committed to 7.x versions of Facet API.

Thanks,
Chris

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.