Improve the detection of pretty URLs usage #6507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Same as @simoheinonen |
|
I think PR can solve #6506 issue as well in one go |
stof
reviewed
Nov 6, 2024
src/Resources/config/services.php
Outdated
| @@ -203,6 +204,7 @@ | |||
| ->set(AdminRouteGenerator::class) | |||
| ->arg(0, tagged_iterator(EasyAdminExtension::TAG_DASHBOARD_CONTROLLER)) | |||
| ->arg(1, tagged_iterator(EasyAdminExtension::TAG_CRUD_CONTROLLER)) | |||
| ->arg(2, service('cache.system')) | |||
There was a problem hiding this comment.
you should probably create a dedicated cache pool inheriting its configuration from cache.system instead of using cache.system directly. this is how core Symfony components work (allowing them to avoid having to care about conflicting keys between the caches of different component, as the key only needs to be unique inside the cache pool)
There was a problem hiding this comment.
See https://github.com/Incenteev/hashed-asset-bundle/blob/v1.5.0/src/Resources/config/cache.xml#L35 for an example of how to do it in a third-party bundle
There was a problem hiding this comment.
Thanks! I did that change.
|
@simoheinonen this looks like a separate issue (even though it is indeed related to pretty URLs), so I suggest opening a dedicated issue. |
It has something to do with this PR. With 4.14.1 I at least get the page open with a broken navi (which this PR tries to fix) but with this PR I get the error. |
|
Ok, I was able to narrow it down to this: doesn't work: public function configureMenuItems(): iterable
{
yield MenuItem::linkToDashboard('Home', 'fa fa-home');
yield MenuItem::linkToCrud('Orders', 'fas fa-box-open', Order::class);
}works: public function configureMenuItems(): iterable
{
// yield MenuItem::linkToDashboard('Home', 'fa fa-home');
yield MenuItem::linkToCrud('Orders', 'fas fa-box-open', Order::class);
} |
javiereguiluz
force-pushed
the
pretty_url_smart_detect
branch
from
2e35b1f
to
16c0f13
Compare
Kocal
added a commit
to symfony/ux
that referenced
this pull request
Nov 29, 2024
This PR was squashed before being merged into the 2.x branch. Discussion ---------- [Icons] use dedicated cache pool | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT based on `@stof` comment on EasyAdminBundle repository EasyCorp/EasyAdminBundle#6507 (comment) > you should probably create a dedicated cache pool inheriting its configuration from cache.system instead of using cache.system directly. this is how core Symfony components work (allowing them to avoid having to care about conflicting keys between the caches of different component, as the key only needs to be unique inside the cache pool) Commits ------- 62f7c03 [Icons] use dedicated cache pool
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #6499.
The bug: if you enable pretty URLs but visit a page that doesn't use them (e.g. a Symfony route embedded in a EasyAdmin dashboard), then all URLs of the dashboard (menus, actions, etc.) use ugly URLs.
Why: because we detected if pretty URLs should be used based on the current request. This is wrong.
How to solve this:
We could create a config option
easyadmin.use_pretty_urls: true... but that would require creating a config file (that soon will be useless because EasyAdmin 5 will only use pretty URLs). It goes a bit against DX, so I don't like that solution.A good technical solution would be to detect if our custom route loader is enabled and it generated the admin routes. Ideally in a Compiler Pass. Sadly, this doesn't work. I asked in Symfony Slack and smart folks like @stof confirmed that this can't work.
So, I ended up with the following solution:
3.1) The custom route loader now dumps all the generated routes using Symfony's cache component
3.2) We detect if pretty URLs are used by checking if that cached item exists and it's not empty
3.3) Indirectly, this improves a lot the performance of finding the route name for a given tuple of Dashboard + CRUD controller + Action. I was going to do this change in the future, but doing it now will help us solve this issue.
I tested in my apps and everything worked as expected, so I'll try to merge this very soon. Thanks.