Visibility incorrectly returns false on OR logic when context missing from a different condition

Thank you for reporting the bug, I'm sorry we left that bug in the module and didn't catch it.

FYI the correct status for a new issue is "Active"; "needs work" is for when a solution has been proposed via a patch or merge request but it needs further work.

Hey Folks,

I have the same issue. My environment is as follows:

* Drupal 10.4.2
* Block visibility group 2.0.4

Thanks for the code provided by @debra-v

It works on my side, and I uploaded a patch regarding the proposed code.

So I changed the status from active to needs review. :)

#5 works

#5 fixes broken logic conditions for me, +1 RTBC

The code differs significantly from the 2.1.x branch. Adding tests would help ensure this bug is fixed in both branches.

@cobenash, @proweb.ua, @ccjjmartin – would any of you be able to write a test to prevent regressions? A merge request would be preferred, but I appreciate the patch!

Moved to MR, edited existing test to use two different contexts so that the AND/OR conditions are properly evaluated / tested.

Test fails without patch: https://git.drupalcode.org/issue/block_visibility_groups-3503194/-/jobs/...

Test passes with patch: https://git.drupalcode.org/issue/block_visibility_groups-3503194/-/jobs/...

Wow, that was quick! Were you already working on the test, @b_sharpe?

I always hesitate to modify an existing test instead of adding a new one since there’s a risk of losing valuable coverage. It’s a bit tricky to tell if anything important is lost here, but seeing the red/green tests is definitely encouraging! Can you alleviate that worry?

Ha, was actually just coincidence, I needed this to fix a prod instance today.

I would normally agree about a new test over editing; however, in this case the edit made more sense as the old test is for testing both AND/OR (testMultipleConditions()) and would have caught this issue in the first place had there been more than one context so I figured the edit made more sense.

This issue is attempting to fix the broken logic in the 2.0.4 version of the module, which I can verify is a real bug. However, the proposed change here will be in conflict with the direction taken in the 2.1.x version of the module, see this issue: https://www.drupal.org/project/block_visibility_groups/issues/2864027 ✨ Allow choosing more than 1 visibility group per block Needs work

There is a backport patch to the 2.0.x version of the module available there that modifies the same lines of code the proposed patch here is modifying. I believe the other patch fixes this issue as well and knowing it was already merged in the 2.1.x version, that in theory is the direction the maintainers was to head. Ideally someone who understands the code in this patch would review the affected code in the already merged branch to verify if the issue here was fixed or not.

Never mind, it fixed itself.

I tested this with a condition that has two optional contexts, and if one or more of them didn't have a value it prevented the block from being displayed, which kinda defeats the purpose of the contexts being set as not required...

Maybe instead of FALSE it gets set to the inverse of the context definition's required flag?