This is a sister issue of #2996431: Fix docs for ConditionPluginBase and together they make very hard to read / follow the condition plugin system. When the execute() method of a condition plugin is called it calls the condition plugin manager which in turn calls back the evaluate() method of the same plugin. Both of these steps are mistyped: the manager property typehint is incorrect as the issue notes and this issue is about ConditionManager::execute() calling evaluate() but that method doesn't exist on ExecutableInterface, the only reason this never blows up is because everyone calls this via the aforementioned ConditionPluginBase::execute() with said plugin as the argument and that's not only an ExecutableInterface but also it's a ConditionInterface. The attached simple patch merely codifies this. Altering the interface is much harder in light of BC policies and this is enough (once the sister issue at least gets the typehint fixed).

CommentFileSizeAuthor
#4 3065586_4.patch1.12 KBghost of drupal past
#4 interdiff.txt886 bytesghost of drupal past
#2 3065586.patch808 bytesghost of drupal past

Comments

Charlie ChX Negyesi created an issue. See original summary.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented
StatusFileSize
new 3065586.patch808 bytes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/lib/Drupal/Core/Condition/ConditionManager.php
@@ -71,8 +71,11 @@ public function createInstance($plugin_id, array $configuration = []) {
+    throw new \LogicException("This manager object can only execute condition plugins");

Let's throw a 

   * @throws \Drupal\Core\Executable\ExecutableException
   *   If the plugin could not be executed.

As that way we conform to the already documented interface. Otherwise this looks good and although this theoretically changes BC because a fatal is now an exception we don't really make any promises about that AND the ExecutableException is already on the interface.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented
StatusFileSize
new interdiff.txt886 bytes
new 3065586_4.patch1.12 KB

Thanks for the review.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented
Related issues: +#2996431: Fix docs for ConditionPluginBase
bircher’s picture

bircher
Location 🇨🇿
commented
Status: Needs review » Reviewed & tested by the community

That makes sense.
I have no concerns about BC for going from a fatal to an exception since both should be avoided and potentially found in a development environment. In addition this change will make it easier to understand and reason about it in case that happens.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 91bbb61c14 to 8.8.x and 89e91f6c15 to 8.7.x. Thanks!

Backported as this is a bugfix and the BC implications are minimal.

  • alexpott committed 91bbb61 on 8.8.x 
    Issue #3065586 by Charlie ChX Negyesi, alexpott: ConditionManager::...

  • alexpott committed 89e91f6 on 8.7.x 
    Issue #3065586 by Charlie ChX Negyesi, alexpott: ConditionManager::...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.