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

Seperate navigation tree cache for each host (Case 166581) #37

Closed
wants to merge 5 commits into from

Conversation

MalteWunsch
Copy link
Member

@MalteWunsch MalteWunsch commented Jan 8, 2024

This allows us to populate navigation trees on dedicated preview servers with host names like "preview.webfactory.de" under different visibility rules.

Technically speaking, at least two issues could be considered as BC breaks:

  • the changed location of the tree cache file
  • the changed constructor parameters of TreeFactory

But I'd like to argue that for practical reasons, both of them should be consivered as internal implementation details. The TreeFactory class/service was never intended to be overwritten, as suggested by the private properties ranging back to 2017. => I would tag a release as a minor version.

MalteWunsch and others added 3 commits January 8, 2024 18:56
As suggested by the private properties ranging back to 2017, this class/service was never intended to be overwritten. We should state that clearly on a syntactical level.
This allows us to populate navigation trees on dedicated preview servers with host names like "preview.webfactory.de" under different visibility rules.
@MalteWunsch MalteWunsch marked this pull request as ready for review January 8, 2024 18:09
@MalteWunsch MalteWunsch requested a review from mpdude January 8, 2024 18:09
@MalteWunsch MalteWunsch marked this pull request as draft January 8, 2024 18:34
@MalteWunsch MalteWunsch marked this pull request as ready for review January 9, 2024 10:33
@MalteWunsch
Copy link
Member Author

@mpdude Seems to work as intented and I don't know which ghost I was hunting yesterday.

Fabian and I like this approach because it is encapsulated inside the bundle without a BC (arguably 👆).

With the alternative "only one tree cache with modified PageBuildDirector or an additional BuildDirector", we would need to consider the different tree structure in the blocks in src/Resources/views/Navigation/navigationBlocks.html.twig (or the overriding Twig template in the project respectively).

A downside of this approach is that in edge cases with projects with multiple host names (e.g. www.youthpass.eu and demo.youthpass.eu) the tree is cached once per host name - but this seems to be negigible (e.g. for GBA, the tree cache is ~220 kb).

@MalteWunsch MalteWunsch requested review from mpdude and removed request for mpdude January 9, 2024 10:43
@mpdude
Copy link
Member

mpdude commented Mar 26, 2024

In the project where we actually implemented the "preview" solution, we chose to use different Kernel instances for different modes of operation – as suggested in https://symfony.com/doc/current/configuration/multiple_kernels.html.

That gives complete separation of all types of caches, including the HTTP level, navigation tree, routing etc. So, I think this PR should be closed.

@MalteWunsch MalteWunsch closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants