ConditionManager::evaluate() should not negate results itself

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

As a bug this will need a test case

MR created from the latest patch ... for anyone to contrib.

+ Added missing test file - it got lost in re-rolling/updating patches
+ Add negated condition for core/tests/Drupal/KernelTests/Core/Entity/EntityBundleConditionTest.php

But dunno what tests could be created that would fail for current core ... before those updates.

Tests appear to have been added so removing that tag.

Can the issue summary be updated, specially the proposed solution. Seems to be open ended.

issue summary updated

11.x is the latest development branch

Issue summary had been updated so removing that tag.

Test coverage seems good so think this is good for committer review too.

There are surely backward compatibility issues here. Contrib and custom plugins will need updating to handle isNegated internally, now the ConditionManager doesn't do it for them?

So you think the safer bet would be to deprecate isNegated from ConditionManager?

i think we're still Need Review, as there is no "clear" ToDo.

imho, i can't see why we need to "deprecate" isNegated.
i don't think $this->configuration['negate'] is an "universal" config/option for all condition plugin.

As a reminder: the issue is to (re)move isNegated usage from ConditionManager to Condition plugin level + make sure is properly used.

Maybe we could have this return $condition->isNegated() ? !$result : $result; in the ConditionPluginBase .. similar with buildConfigurationForm implementations and usage for condition plugins.

@longwave thoughts on

Maybe we could have this return $condition->isNegated() ? !$result : $result; in the ConditionPluginBase .. similar with buildConfigurationForm implementations and usage for condition plugins.

Hiding patches as work appears to be in the MR.

@vasike can the MR be updated to 11.x please maybe with return $condition->isNegated() ? !$result : $result;

Wondering if a possible way forward here is to add NegatableConditionInterface which signifies that a condition allows negation and will handle it internally, and then we can deprecate it from ConditionManager?

new MR for 11.x available.
This moves isNegated checking to ConditionPluginBase new "helper" evaluateIsNegated

But still not sure.

Maybe @longwave has a point with #57 suggestions ... but still i can't see "the ideal" solution.

However, for now ... Needs Review

sorry, But
Reminder: This is a Bug, so i think this should be fixed (already).

And if the fix is not ideal. then create follow-up ticket to chase the perfect solution of Negated Conditions.

http://codcontrib.hank.vps-private.net/search?text=configuration%5B%27ne...

There are multiple pages of plugins out there that are using the same negated, so we have to keep BC in mind here, I think I agree with having an interface to mark them as being able to do it themselves + a deprecation is needed here.

larowlan → closed merge request !3999

Yes, I think #57 is the best way forward here

I have client projects with lots of condition plugins that would need to be communicated this change.

With #57 we can trigger a deprecation error if the condition plugin doesn't implement the new interface, which gives us a way to eventually consolidate them.

Updated the MR - NegatableConditionInterface and NegatableConditionBase added and related updates.

a review it would be nice

With #57 we can trigger a deprecation error if the condition plugin doesn't implement the new interface, which gives us a way to eventually consolidate them.

I don't see the deprecation

@smustgrave i didn't say i added deprecation.
just the interface and the "base class"

and i asked for a review ... for those things ...

maybe there are other things ... to make them "nice" ... before the final touch

left some comments

@smustgrave Type hints added. thanks
But unfortunately, it started failing for spellcheck ... i thought it was the new class name(s) - but not sure.
also not sure what are the "related" updates on CI ... "any help will be helpful"

Seems you got bit by 🐛 Spell-checking job fails with "Argument list too long" when too many files are changed Active there's a workaround post on there.

rebased again and 11.x branch as well

and cleaned-up rebase

@smustgrave / @andypost you're my brain saviours
thanks ... cleaned up my last changes. it looks "green" ... even it tried to "do it red"
then ... let's review

Believe all feedback has been addressed so +1 from me but leave in review for some additional eyes to avoid ping ponging around.

Been a few days and don't want the issue to stale so going to mark.

I'm triaging RTBC issues → .

Ah, a 9 year old issue. I like seeing the older issues getting near completion.

I read the IS and the comments. I didn't find any unanswered questions. However the proposed resolution does not match the direction taken in #57. This is more that a refactor, it should include that an interface is added. And also a CR.

I made comments in the MR that need to be addressed.

Changing to NW

@quietone thanks a lot for the reviews (issue and MR/code) and their feedback

Update the MR trying to cover all of threads ... it looks nicer ... at least

However, i expected a feedback about #57 changes i added ... like a confirmation ... so we could update the issue summary ... add change record ...
But, i saw no one.

Maybe @longwave, as "initiator" could check it.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

solved MR conflicts.

After looking at the new approach I'm still not entirely convinced this is worthwhile, and this only feels like a stopgap; somehow we have to later remove non-NegatableConditionInterfaces, but also we probably want to allow conditions to not actually support negation (like in the boolean case where it makes no sense), but not sure how to achieve that.

This is ultimately a framework manager decision so let's tag them for input.

Like #78 I'm not sure whether this gets us to the eventual end point and it doesn't look like much of an improvement on its own, if that's known, can follow-up issues be opened with an explanation?

I would be useful if the issue summary and a change record were fleshed out too, so marking needs work for that.