Break router.builder dependency

You could use setter injection in the proxy to get route.builder in there so that it can provide actual rebuild functionality for router provider.

I think this is valuable just for disentanglement reasons.

The advantage of the state / rebuild service is that it can decide to either inject or lazily load the services, but we can always replace the implementation without having to change all services using that.

So +1 to that.

Now this is critical because #2351777: Do not depend on event subscribers for security: Replace AccessRouteSubscriber with build-in checks depends on it: that patch fails because of a circular dependency "router.builder -> access_manager -> router.route_provider -> router.builder"

Now this no longer needs a performance review because of #5 :) catch indicated he wanted ContainerAware which made this stupid simple. Since we always need to call rebuildIfNeeded() in a request, I opted to inject state because it always will be needed.

This can't be real ;)

Odd, I thought that's automated.

Typehinting with an object? Tsk, tsk.

The missing return in getCollectionDuringRebuild broke it. For good measure, added to rebuild to.

This patch allows the passing of access_manager into router.builder.real. That's all.

RTBC, the proxy is a good step to unblock other issues and get some lazyness into the route system.

We can always replace with a lazy proxy from #1973618: DIC: Lazy instantiation of service dependencies (ProxyManager for "proxy services") later on - when / if that landed.

This unblocks a circular dependency.

Rerolled.

I'm still a bit uneasy about this proxy compared to the smaller self-contained service we could inject both into RouterBuilder and other things that currently take RouterBuilder as a dependency. Is that really much harder to implement? I know it's an API change but RouterBuilder could also proxy through to those methods, keeping a BC layer no?

Had brief discussion with alexpott and he had similar concerns so it doesn't feel like just me.

My concerns have translated into a patch. That said I'm not sure how either patch really breaks the dependency chain router.builder -> access_manager -> router.route_provider -> router.builder.

Fixed ThemeController

RTBC - Looks great to me.

It makes a lot of sense to keep the state separate from the actual builder.

Please also be aware of: #356399: Optimize the route rebuilding process to rebuild on write

But I think we get this one in first and then follow-up on #356399: Optimize the route rebuilding process to rebuild on write as we can then nicely couple the isRebuildNeeded() into the isRebuildReallyNeeded parts more cleanly.

Thanks!

The problem here is that access manager depends on route provider depends on route.builder which then can't depend on access manager. That's the chain we tried to break and this patch didn't break that. Mine did because access manager depends on route provider depends on route.builder and then route.builder.real can get access manager freely.

Edit: the chain breaking happens the moment route.builder.real is not constructor injected but via the container aware way.

AccessManager only uses the injected route provider for checkNamedRoute().

checkNamedRoute() isn't used by route.builder.

So we should also be able to split that up to break the circular dependency.

Right, let's do that. I will split access_manager in two by introducing a loadcheck private service both halves rely on. This will break the backwards compatibility of AccessManagerInterface . Are we cool with that? Notably setChecks will be removed which of course is not used by anyone except the route builder.

#28 sounds like we get to clearly delineated services, with no circular dependencies, and without using either ContainerAware or EventDispatcher (which is on top of ContainerAware anyway) tricks to solve them, which sounds great.

Committed/pushed to 8.0.x, thanks!

See you in the follow-ups.