ConditionManager::evaluate() should not negate results itself

and the behaviour should b covered with tests

I'd better change the docs for negated... but +1 to approach
implementation details of isNegated() should be able to access config and do some custom logic but return !empty($this->configuration['negate']) by default

1. +++ b/core/lib/Drupal/Core/Condition/ConditionInterface.php
   @@ -54,6 +54,9 @@
   +   * @deprecated Deprecated as of 8.0.0. Condition negation is an implementation
   +   *   detail and should not be relied upon.
   ...
      public function isNegated();
   

   needs a point about removal

2. +++ b/core/lib/Drupal/Core/Condition/ConditionManager.php
   @@ -68,8 +68,11 @@ public function createInstance($plugin_id, array $configuration = array()) {
   +    if (!($condition instanceof ConditionInterface)) {
   +      throw new \InvalidArgumentException(sprintf('%s must be an instance of %s, but %s was given.', '$condition', '\Drupal\Core\Condition\ConditionInterface', get_class($condition)));
   

   scope-creep?

3. +++ b/core/modules/language/src/Plugin/Condition/Language.php
   @@ -143,7 +143,8 @@ public function evaluate() {
   +    return $this->configuration['negate'] ? !$result : $result;
   
   +++ b/core/modules/system/src/Plugin/Condition/CurrentThemeCondition.php
   @@ -109,7 +109,9 @@ public function evaluate() {
   +    return $this->configuration['negate'] ? !$result : $result;
   
   +++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
   @@ -148,15 +148,18 @@ public function evaluate() {
   +    return $this->configuration['negate'] ? !$result : $result;
   

   Why not configuration accessed directly?

   return $this->isNegated() ? ..
   will make more sense

+++ b/core/lib/Drupal/Core/Condition/ConditionManager.php
@@ -68,8 +68,11 @@ public function createInstance($plugin_id, array $configuration = array()) {
+    if (!($condition instanceof ConditionInterface)) {
+      throw new \InvalidArgumentException(sprintf('%s must be an instance of %s, but %s was given.', '$condition', '\Drupal\Core\Condition\ConditionInterface', get_class($condition)));

Isn't that a usecase of an assertioN?

See #2536560: Runtime Assertion unit and functional testing

1. +++ b/core/lib/Drupal/Core/Condition/ConditionManager.php
   --- a/core/modules/language/src/Plugin/Condition/Language.php
   +++ b/core/modules/language/src/Plugin/Condition/Language.php
   
   +++ b/core/modules/language/src/Plugin/Condition/Language.php
   --- a/core/modules/system/src/Plugin/Condition/CurrentThemeCondition.php
   +++ b/core/modules/system/src/Plugin/Condition/CurrentThemeCondition.php
   
   +++ b/core/modules/system/src/Plugin/Condition/CurrentThemeCondition.php
   --- a/core/modules/system/src/Plugin/Condition/RequestPath.php
   +++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
   

   Shouldn't we add some form of test coverage for all those fixes?

2. +++ b/core/tests/Drupal/Tests/Core/Condition/ConditionManagerTest.php
   @@ -0,0 +1,80 @@
   +  public function testConstruct() {
   +    $namespaces = new ArrayObject();
   +    $this->sut = new ConditionManager($namespaces, $this->cache, $this->moduleHandler);
   +  }
   

   Mh, so this method doesn't have any assertions ... isn't there something we could check for?

do we have the patch for core 8.7.x? As current one doesn't apply & 8.7 still has this negate issue

Rerolled for 8.9 (which is incidentally the same as 8.7 in this case)

I'm not sure what to do with isNegated(). It's not that it needs to go away, as it is important to the implementations. But it shouldn't be on the interface. For now I just turned the @deprecated into a doc comment.

+++ b/core/tests/Drupal/Tests/Core/Condition/ConditionManagerTest.php
@@ -0,0 +1,80 @@
+/**
+ * @file
+ * Contains \Drupal\Tests\Core\Condition\ConditionManagerTest.
+ */

should be removed

Clean-up and fix test but not sure the test is right

It seems this doesn't cover the EntityBundle conditions
@see https://www.drupal.org/project/drupal/issues/3255496 - closed as it seems part of this.

Addressed Drupal CS issue of patch #29, keeping the status needs work for comment #36.

I have fixed CS error. please review it

2 updates for the latest patch

1. use $this->isNegated(), instead of $this->configuration['negate']

2. Updated also for Entity Bundle condition, as specified on #36

2 more updates:

1. We don't need the "initial" $this->isNegated() condition, besides the ones about condition configuration check.

2. UserRole condition updated.

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 and/or NegatableConditionBase which signifies that a condition allows negation and will handle it internally, and then we can deprecate it from ConditionManager? But if most conditions allow negation this is still quite a change for them all to make.

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.

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 #3401988: Spell-checking job fails with "Argument list too long" when too many files are changed 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.