search

neos / flow-development-collection

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

MVC Dispatcher is emitted even in CLI Dispatcher (Slack Conversation) #1892

Open sorenmalling opened 4 years ago

sorenmalling commented 4 years ago

Søren Malling:rainbowsheep: 3:12 PM I seem to have found a change in FLOW 6.0 I can't find mentioned in the changelog or other docs. When the Neos\Flow\Security\Context class is injected with @Flow\Inject I get the following exeption in the CLI

Cannot build object "Neos\Flow\Security\Authentication\TokenAndProviderFactoryInterface" because constructor injection is not available in the compile time Object Manager.
Refactor your code to use setter injection instead. Configuration source:
automatically registered class. Build stack:
Lelesys\SuperUser\Service\SuperUserService, Neos\Flow\Security\Context

What changed on this subject? (edited)

Christian Müller 3:16 PM nothing, you try to use it during compile time, that was never allowed the refactoring just split things, so while before Security\Context was probably not ossible at compile time, now it's the TokenAndProviderFactoryInterface

Søren Malling:rainbowsheep: 3:19 PM Can you enlighten me a bit? where Compile time kick in when i run the ./flow command and this Lelesys\SuperUser\Service\SuperUserService (which is @Flow\Scope("singleton") ) is injected in a Web controller and mentioned in a signalSlot dispatcher registration in Package.php. No exception before upgrading from Flow 5 to Flow 6 so trying to track down the real issue

Christian Müller 3:21 PM interesting yeah signal slot might be the issue compile time is a "run" of flow where proxies are build so no injection available etc. calling flow surely can trigger a compiletime "run" if the caches are empty

Søren Malling:rainbowsheep: 3:24 PM Got it! $dispatcher->connect('Neos\Flow\Mvc\Dispatcher', 'afterControllerInvocation', 'Lelesys\SuperUser\Service\SuperUserService', 'appendLogoutLink');`

This registration caused it Didn't think about the signal slot dispatcher was a issue :slightly_smiling_face: Thanks!

Christian Müller 3:25 PM that is weird though if you can I would further track that, because why would the MVC Dispatcher be instantiated in a compile time (CLI) request? there must be some reason for that since 6.0 CLI has a separate dispatcher so the MVC dispatcher is not even needed for a CLI request

Søren Malling:rainbowsheep: 3:27 PM The MVC dispatcher i used in the Cli Requesthandler - could that be why ?

Christian Müller 3:27 PM yes why would you do that

Søren Malling:rainbowsheep: 3:27 PM Never mind, it was a separate Cli Dispatcher (mistaked it for the Mvc\Dispatcher class) Here is exception log

8 Neos\Flow\ObjectManagement\CompileTimeObjectManager::get("Neos\Flow\Security\Authentication\TokenAndProviderFactoryInterface")
7 Neos\Flow\ObjectManagement\CompileTimeObjectManager::get("Neos\Flow\Security\Context")
6 Neos\Flow\ObjectManagement\CompileTimeObjectManager::get("Lelesys\SuperUser\Service\SuperUserService")
5 Neos\Flow\SignalSlot\Dispatcher::dispatch("Neos\Flow\Mvc\Dispatcher", "afterControllerInvocation", array|3|)
4 Neos\Flow\Cli\Dispatcher::emitAfterControllerInvocation(Neos\Flow\Cli\Request, Neos\Flow\Cli\Response, Neos\Flow\Command\CoreCommandController)
3 Neos\Flow\Cli\Dispatcher::dispatch(Neos\Flow\Cli\Request, Neos\Flow\Cli\Response)
2 Neos\Flow\Cli\CommandRequestHandler::handleRequest()
1 Neos\Flow\Core\Bootstrap::run()

Line 5 uses mentiones the Mvc\Dispatcher (edited) new messages

Christian Müller 3:30 PM ah, damn I thinkn I know where that comes from for backwards compatibility we dispatcch the "Neos\Flow\Mvc\Dispatcher", "afterControllerInvocation" event AFTER a cli request was handled and I guess that triggers something for the super user thingy

Søren Malling:rainbowsheep: 3:33 PM That could be. I made it by wrapping the signalSlot registration with `

if (PHP_SAPI !== 'cli') {

kitsunet commented 4 years ago

Ok, just for clarification

backwards compatibility we dispatcch the "Neos\Flow\Mvc\Dispatcher", "afterControllerInvocation" event AFTER a cli request was handled

This happened before the change as well, we just continue doing it despite it not being strictly necessary or rather now being "wrong". So that is not the root cause of the issue. Rather the refactoring of the security context seems to make it not injectable during compile time anymore (which doesn't make much sense anyway) but apparently some class here (SuperUserService) is triggered via above signal and then wants a security context injected, which results in the error.

sorenmalling commented 4 years ago

@kitsunet

Can you help me with a conclusion? I don't believe the CLI/Dispatcher should call the Mvc/Dispatchers "afterControllerInvocation", right?

This signal is currently responsible of persisting, eventually based upon a httpRequest - what will we "lose" from removing this?

albe commented 4 years ago

Can you help me with a conclusion? I don't believe the CLI/Dispatcher should call the Mvc/Dispatchers "afterControllerInvocation", right?

Correct, it was done for backwards compatibility and should be changed in next major.

This signal is currently responsible of persisting, eventually based upon a httpRequest - what will we "lose" from removing this?

Nothing, we should just probably change it to Neos\Flow\SignalSlot\Dispatcher::dispatch("Neos\Flow\Cli\Dispatcher", "afterControllerInvocation", ...) so you can properly act depending on Cli or Mvc use case.

But the real cause is probably that some things should just not happen inside a compile time run... maybe triggering exactly those signals? That can be checked via the $runlevel inside the CommandRequestHandler. Needs further investigation I guess.

albe commented 4 years ago

Should we take a look at this for the upcoming major?

sorenmalling commented 4 years ago

Should we take a look at this for the upcoming major?

Yes, I'm open for a call after next week or on slack untill then 👍

albe commented 4 years ago

Related to #1244 - if we have a different signal for CLI than MVC, the if condition in the slot for persisting can be avoided.

sorenmalling commented 4 years ago

Related to #1244 - if we have a different signal for CLI than MVC, the if condition in the slot for persisting can be avoided.

Sounds like a good solution - after all it's two different contexts (prepping for wedding later today (not mine) - so can only comment not review)

fcool commented 3 years ago

Related and as least the BUG part is solved in #2528

New Slots for the different dispatchers are the next logical step.

mhsdesign commented 8 months ago

So the legacy layer will be removed via https://github.com/neos/flow-development-collection/pull/3296 so i guess that will allow us to close this thread.