Fix process order of negated context conditions

Patch works - Thanks a lot for sharing!

If i should experience any issues with this modification later own, I'll post it here. Otherwise you can assume that it works without any issues.

here is a minor issue, the signature of the overridden get_contexts method is not the same, as of the base class. simply add the $value parameter..

results in a strict warning
Strict warning: Declaration of context_condition_context::get_contexts() should be compatible with that of context_condition::get_contexts() in _registry_check_code() (Zeile 3024 von C:\xampp\htdocs\Inostudio\11freunde_website\includes\bootstrap.inc).

Fixed the strict warning by adding the missing $value method parameter (unused).

Works. Thanks for the work!

Just for the note.

If you have custom context condition that load *after* context_condition_context plugin, make sure to lower weight of the module that provides that plugin.

I'm having this issue with the D6 version. Will this patch also work with context for D6?

Works good for me and I'm putting this patch into production!

Committed in e240e6c. Please reopen if you're got a D6 patch (or can confirm that this one will also work in D6).

The commited patch doesn't work for negated context conditions with wildcards. Here is a patch (against 7.x-3.x) that handles them too.

NOTE that this solution still doesn't work for dependency trees of depth >= 3.

Example:

4 contexts:

• a always active
• b depends on a
• c depends on b
• d depends on ~c

Result:

1. c is checked first (activates nothing), then checking b and d are checked (activates b and d)
2. second pass: checks c (activates c).

So all four contexts are set active although their conditions are conflicting.

A solution that works for all combinations of positive and negative context conditions is non-trivial and would have to deal with circles in the dependency graph.

… and condext_condition_context_all does not even support negations.

Here is a more complete solution to this problem. It uses the original idea of calculating "to be activated"-contexts incrementally until there is no change. It uses a temporary list of "to be activated"-contexts so it can deselect previously selected contexts. This patch also includes circle detection.

Accidentally attached the wrong patch. Here's the right one.

@torotil
I have tried your patch on dependable contexts 5-7 children deep and it did not work as expected. Could you please test it yourself?

I've tried another approach - reassessing already active contexts. Patch attached.

@alex: Thanks for testing my patch. Could you point out an exact case where my patch doesn't work?

Could you please test it yourself?

What leads you to the assumption that I haven't done that? We're using #13 in production on several sites for months now.

I've tried another approach - reassessing already active contexts.

How does your approach deal with circles? How is it different from #15?

Example for a circle …

A -> ~B
B -> ~C
C -> ~A

I've taken a closer look at #15 and I'm certain that it won't work. It's nothing but a two-pass solution again:

1. it activates all contexts that have matching conditions.
2. it deactivates contexts that have any context set that's negated

That's algorithmically wrong. It's guaranteed to fail with negated chains like I've posted above. And it's result depends on the order in which it evaluates contexts.

Could someone else please go over #13?

Ok I've found a bug after all: the +-operator with arrays doesn't work as expected with numeric IDs.

I've also adjusted the issue summary to reflect the current state of this issue.

I've also written tests for the depency resolution. I've simply added the examples from #10 and #17 as tests. The tests fail without patches as well as with #13 and #15.

I've updated the summary again. Is anybody willing to review this patch? It even has unit-tests.

Thanks for all your work on this. I will try and find time next week to do a review.

Sorry, I thought I had a case of this problem, but I don't, so I can't test your code for its effectiveness. FWIF, it looks to me like it would do the job. I do have one small remark about the coding style. Drupal Coding standards state:

Functions and variables should be named using lowercase, and words should be separated with an underscore.

You add several variables that do not comply to this, e.g. $activeOld, $activeNew and $knownStates. You also add a member function calculateActiveContexts which - although nog against Drupal coding standards istself - goes against the naming convention within context, where member functions follow a similar naming to regular functions and variables.

Attached is a new patch that is functionally identical to #20 but with coding-style issues fixed.

Those tests are broken in the plain 7.x-3.x branch too.