- ๐ต๐ฑPoland sayco Toruล
First of all, I think the ticket needs to be updated with the information that the fromInternalUri is now part of the Url class, not the UrlHelper.
The other thing about this (before we apply any changes in the code) is that the implementation of the function is wrongly designed. It breaks the SRP, as itโs not only responsible for creating the internal URI, but also for validation against the external URI. I think this is the wrong approach.
I know it requires a lot of changes, but the fromInternalUri should be responsible for building the internal URI from whatever is given, or it should return a null value or raise an exception. Whatever is using the function should be responsible for handling the exception or null value, checking what the given value is, and taking appropriate actions depending on the outcome. It shouldnโt be the responsibility of the fromInternalUri method to know about other dependencies and all the use cases.
We also should avoid adding any silent caches in the code. At the very least, logging with debug level should be added. Otherwise, itโs hard to guess that anything unexpected happened in the code.
- ๐ฉ๐ชGermany donquixote
Quick idea,
would it make sense to return a markup object with cacheable metadata, instead of a render element?$markup = $renderer->renderRoot($element); assert($markup instanceof MarkupInterface); assert($markup instanceof CacheableDependencyInterface);
- ๐ฆ๐นAustria mvonfrie
It seems that my comment #2996410-14: Review Drupal\Core\Menu\StaticMenuLinkOverrides::saveOverrides for performance improvements โ belongs more to this issue which I just found after posting there. Repeating and crosslinking the issues.
When changing an override on the test system and exporting config I observe constant changes in the order of keys per overridden menu item. Especially
weight
andenabled
always change their position between local dev and test environment.Could this be related to the line
$all_overrides[$id] = $definition + $this->loadOverride($id);
as well?
Furthermore, in my opinion the config should only include what actually can be overridden, aka filter
$expected with Drupal\Core\Menu\MenuLinkBase::$overrideAllowed
(if applicable).