neos / flow-development-collection

The unified repository containing the Flow core packages, used for Flow development.
https://flow.neos.io/
MIT License
138 stars 188 forks source link

BUG: `@Flow\Inject` `$throwableStorage` is not injected in my Controller #2928

Open mhsdesign opened 2 years ago

mhsdesign commented 2 years ago

Is there an existing issue for this?

Current Behavior

The ThrowableStorage doesnt get injected in a controller which extends ActionController

<?php

class MyController extends ActionController
{
  /**
   * @Flow\Inject
   * @var ThrowableStorageInterface
   */
  protected $throwableStorage;

  public function myAction()
  {
    \Neos\Flow\var_dump($this->throwableStorage);
    die();
  }
}

Expected Behavior

The ThrowableStorage is injected

Steps To Reproduce

See above code

Environment

- Flow: since 6.3? tested on 8.0
- PHP: nevermind

Anything else?

The issue is, that the ActionController already defines an inject method for this property.

    /**
     * @var ThrowableStorageInterface
     */
    private $throwableStorage;

    /**
     * Injects the throwable storage.
     *
     * @param ThrowableStorageInterface $throwableStorage
     * @return void
     */
    public function injectThrowableStorage(ThrowableStorageInterface $throwableStorage)
    {
        $this->throwableStorage = $throwableStorage;
    }

this was done sometime with Flow 6.3 via https://github.com/neos/flow-development-collection/pull/2685

and flows behaviour is correct, if there is an injector/setter - ignore the flow inject annotation. (Flow cannot know that the injector above will only fills in a private property)

https://flowframework.readthedocs.io/en/8.1/TheDefinitiveGuide/PartIII/ObjectManagement.html#property-injection

If a setter method exists for the same property, it has precedence.

the question is - why do we really need to use an injector to a private property in the ActionController?

related issue: https://github.com/neos/neos-development-collection/issues/3858

Workarounds:

kdambekalns commented 2 years ago

why do we really need to use an injector to a private property in the ActionController?

Because @Flow\Inject cannot work on a private property, but making it protected would lead to clashes with subclasses already having a $throwableStorage

The "proper solution" would be to make the $throwableStorage protected so subclasses don't even need to inject their own. But this might make it a breaking change?!

mhsdesign commented 2 years ago

or rename the setter and the private property should do too (non breaking)

kdambekalns commented 2 years ago

or rename the setter and the private property should do too (non breaking)

Yes, but to have the ThrowableStorage readily available like the Logger would make sense. This mess is the result of adding the former without realizing it would be very useful to controllers in general…

mhsdesign commented 2 years ago

My personal opinion: I don't particular like inheritance and reusing protected services from above - it just feels dirty.

so I'm okay with the idea to make it private somehow (we could even use constructor injection ...)

on the other hand - if i like it or not - inheritance is already the pattern to use and one cannot dodge it - so it wouldnt harm adding yet another property. (things will never be really encapsulated with inheritance - but thats how we do things for now)