Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Attribute driven routes are ignored in production context #3400

Closed
1 task done
bwaidelich opened this issue Oct 11, 2024 · 3 comments · Fixed by #3401
Closed
1 task done

BUG: Attribute driven routes are ignored in production context #3400

bwaidelich opened this issue Oct 11, 2024 · 3 comments · Fixed by #3401

Comments

@bwaidelich
Copy link
Member

bwaidelich commented Oct 11, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Routes from attributes are not detected

Expected Behavior

Routes from attributes should be respected and listable via ./flow routing:list

Steps To Reproduce

Add Route attributes to some MVC action and configure the AttributeRoutesProviderFactory as described

Environment

- Flow: 9.0@dev

Anything else?

I could pin point the issue down to this line

$annotatedClasses = $this->reflectionService->getClassesContainingMethodsAnnotatedWith(Flow\Route::class);
that returns an empty array in production context, probably because it is invoked before the ReflectionService is fully initialized.

Related: #2059

@kitsunet
Copy link
Member

kitsunet commented Oct 11, 2024

This shoudn't be affected by the ConfigurationManager availability I think?

This is super obscure but I think the problem is that we just do not load method annotation data in production. This is sensible for performance, and we can fix this issue here by using compile static, which we should do anyways for better production performance, BUT fact is, we should have the same behavior in Development and instead only decide between runtime and compiletime behavior.

Atm. \Neos\Flow\Reflection\ReflectionService::initialize has a special handling for production with frozen caches, and I skimmed over this for a bit because I thought we don't freeze anymore, turns out, the reflection service automatically freezes it's own caches for production, so this triggers basically always for production. And in this code branch we just do not load the necessary reflection data (together with below we should probably except method calls that try to access this while not in compiletime or better yet, split the reflectionservice between one for compiletime and one for runtime like the object manager does).

I know from experience that this trickery is necessary as the ram usage for this is huge.
But as suggested to make this not only fail in production we should change \Neos\Flow\Reflection\ReflectionService::hasFrozenCacheInProduction to not check for production but for compiletime, trick is that the bootstrap does not expose this state atm, and we need to provide it to the reflectionservice first to test against. Should all be non breaky but a bit complicated. I will have a look. Short term I would really just change this to work with compile static to fix the bug.

Oh and while I can have a look at a PR tomorrow, for sake of clarity as this is a bit of a hidden feature I guess,
this is a good example of compile static usage: \Neos\Flow\Cli\CommandManager::getCommandControllerMethodArguments this method will be executed in compiletime and then overwritten in the proxy with one that hardcoded returns whatever was returned during compiletime, cutting off all dependencies used inside the method call and thus never having to use the unavailable reflection data in production.

@kitsunet
Copy link
Member

The PR should fix the immediate bug, I will open additional PRs with some ideas about the wider topic of runtime (production) reflection data etc.

@kitsunet
Copy link
Member

#3402 for the underlaying issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants