evaluate() should always return a boolean

Created on 19 February 2023, about 2 years ago
Updated 21 February 2023, about 2 years ago

Problem/Motivation

See https://www.drupal.org/project/user_permission_condition/issues/3290292#... 📌 Automated Drupal 10 compatibility fixes Fixed

PermissionCondition::evaluate() currently doesn' reliably return a boolean value (if none of the conditions matches).

See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Condition...

Evaluates the condition and returns TRUE or FALSE accordingly.

Steps to reproduce

Proposed resolution

Return a boolean and ensure negate is considered

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Needs review

Version

1.0

Component

Code

Created by

🇩🇪Germany Anybody Porta Westfalica

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

  • Issue created by @Anybody
  • @anybody opened merge request.
  • 🇩🇪Germany Anybody Porta Westfalica
  • Status changed to Needs review about 2 years ago
  • 🇩🇪Germany Anybody Porta Westfalica

    Tried to save us from code duplications. If you like a more simple approach, we can also just add a return value:

        return $this->isNegated() ? FALSE : TRUE;
    

    at the end instead, of course. And keep the existing logic.

    Result should be the same.

  • 🇩🇪Germany geek-merlin Freiburg, Germany

    Yay, code-wise this looks good and clean.
    Code looks good, but it's not tested, and i trust no human brain, not even mine, to grok code execution.
    We have an install and uninstall test, but no unit test for this simple piece of functionality.
    Can you add a test (unit test is enough) or at least test manually?

  • 🇩🇪Germany Anybody Porta Westfalica

    @geek-merlin totally understand and agree ;) The main reason for this issue was to not forget this. I think it's really minor, but will put this on our list for at least a manual test. But don't expect it too soon. :)

  • 🇩🇪Germany geek-merlin Freiburg, Germany

    Good, so we'll tacke this further then.

Production build 0.71.5 2024