Problem/Motivation

setExecutableManager() is on \Drupal\Core\Condition\ConditionInterface.
There is a base class, \Drupal\Core\Condition\ConditionPluginBase, which extends \Drupal\Core\Executable\ExecutablePluginBase.

It's only called by ConditionManager.

However, ExecutablePluginBase provides the implementation, which is backwards.

Proposed resolution

Move the method (and protected property) to the correct class.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#1 2385429-setExecutableManager-1.patch2.05 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2385429-setExecutableManager-1.patch2.05 KB
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

It's concerning that our existing tests don't catch this. Would adding tests here be a possibility?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Well it's not broken per se.

ConditionPluginBase extends ExecutablePluginBase. Having setExecutableManager() on the parent doesn't hurt, but since only ConditionPluginBase::execute() uses it, I figured we could move it.

Xano queued 1: 2385429-setExecutableManager-1.patch for re-testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs review » Reviewed & tested by the community

I think the biggest problem that this patch solves isn't even that only conditions use this, but that it's ConditionInterface that defines setExecutableManager(), but ExecutablePluginBase that implements it.

alexpott’s picture

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

No other type plugin other than Conditions (including stuff in contrib) is already using this or would find it useful?

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

The only usage of this method is ConditionManager setting itself on condition plugins. This looks like a pattern that we should not copy elsewhere, but that in the future should be replaced by proper dependency injection.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

@alexpott: does #7 explain the situation sufficiently, or is there something else you'd like us to do?

Xano queued 1: 2385429-setExecutableManager-1.patch for re-testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs review » Reviewed & tested by the community

RTBC'ing to 'assign' to @alexpott.

alexpott’s picture

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

I agree that the implementation is backwards. There are no docs to inherit :)

Committed abb22d7 and pushed to 8.0.x. Thanks!

  • alexpott committed abb22d7 on 8.0.x 
    Issue #2385429 by tim.plunkett: setExecutableManager() is implemented on...

Status: Fixed » Closed (fixed)

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

Filed follow-up #2996431: Fix docs for ConditionPluginBase