-
Notifications
You must be signed in to change notification settings - Fork 12
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
OEL-1057: Fix issue with menu preprocess overriding core keys. #167
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plese, check if we can only have one commit instead of 2.
d798d71
to
d2fc42a
Compare
includes/menu.inc
Outdated
'label' => $item['title'], | ||
'path' => $item['url']->toString(), | ||
'attributes' => new Attribute(['class' => $class]), | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@escuriola I see some repetition here vs above.
And I wonder, where is this 'trigger' value used, and which array keys does it expect?
In the menu.html.twig
, I see we are directly calling the pattern.
In pattern-navigation.html.twig
we are sending the items directly to bcl-navigation.html.twig
.
Here they go to bcl-dropdown.html.twig
which indeed uses the 'trigger' part.
The values within 'trigger' are then sent to bcl-link.html.twig
or bcl-button.html.twig
. At least 'link' expects 'path' and 'label', whereas 'button' only expects 'label'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now where the 'trigger' key is used.
Imo this is a bit weird: By setting $pattern_item['trigger']
, some other keys within $pattern_item
such as $pattern_item['label']
no longer have any effect.
Trying to clean this up would be out of scope in this issue. So let's keep it as it is.
includes/menu.inc
Outdated
$variables['items'][$key]['items'][$key_sub_item] = $sub_item_menu; | ||
$variables['items'][$key]['items'][$key_sub_item]['label'] = $sub_item['title']; | ||
$variables['items'][$key]['items'][$key_sub_item]['path'] = $sub_item['url']->toString(); | ||
$variables['items'][$key]['items'][$key_sub_item]['attributes'] = new Attribute(['class' => $subclass]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@escuriola
This change seems to be pointless.
The original Drupal menu item does not have any 'items' key that we were destroying. 'items' is a completely new key that is only understood by bcl-navigation.
includes/menu.inc
Outdated
]; | ||
$variables['items'][$key] = $item_with_submenu; | ||
$variables['items'][$key] += $item_with_submenu; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@escuriola
Overall I feel we have a lot of redundant juggling of variables here:
We are having local variables like $item
from the foreach, $items
from outside the foreach, $item_with_submenu
, and in the old code we had $menu_item
. But then we are writing each value to $variables['items'][$key]
.
We have two options that would simplify this:
- By-reference
foreach ($variables['items'] as $key => &$item) {
, then write everything directly to$item
. Perhaps we could call it$item_ref
to remind the reader that this is by-reference. - Don't use by-reference, write everything to
$item
, and in the end set$variables['items'][$key] = $item;
.
Also, instead of local vars like $item_with_submenu
with later +=
, we could directly call $item += [...]
.
I have a more general concern with the entire rewriting of variables here, but I will address this in another comment.
A general concern I have with this entire rewriting mechanism: LSP for templates and patternsIn my opinion, we should treat patterns and Drupal theme template overrides in the same way we treat method overrides in OOP code: We should respect the parameter signature of the parent implementation. Liskov Substitution Principle (just adding this to easily find this post later). The variables sent to the template should have roughly the same array structure as the variables that are sent to the respective core template. In preprocess we can do some minor changes, but we should not fundamentally change the structure of the array. Not doing this leads to problems with other components (modules) that try to inject their own templates - as we have seen here. With this PR we are trying to work around this problem by building a mixed structure that is understood both by our templates and by core templates. This mixed structure has lots of duplication, e.g. now we have Theme hook signature vs template signatureIn Drupal it is a little bit more complicated than that. The signature of a theme hook is any variables we sent to the theme hook, e.g. from a render element. The goal would be to respect the spirit of LSP both for the theme hook signature and for the template signature. Perhaps the LSP-ish rules are a bit vague for templates and theme hooks, but we should at least not completely rewrite these Patterns vs BCLBtw the same applies to patterns vs bcl templates: The variables received by a pattern variant should always be the same as for the main variant, and as defined in the pattern definition (yml file). The variables sent to the BCL template can have a different structure. The rewriting has to happen in the pattern template, possibly with the help of a twig function. SolutionThe boundary from "Drupal menu theme hook" to "navigation pattern" is in the menu template itself, where we include the pattern. To not repeat this in every menu template, we could write a twig function So in {{ pattern('navigation', {
items: items|drupal_menu_to_navigation_pattern
}) }} This twig function could then do a very clean conversion, and would not need to support a duplicate structure. Next stepsDon't rewrite everything yet. Perhaps we want to deliver a quick fix that keeps most of the logic in the preprocess function. |
I talked with @brummbar . He rightly suggested we should use a separate variable for the items that are sent to the pattern. |
d9650b8
to
f3eae53
Compare
7c0b426
to
0eadef2
Compare
includes/pattern.inc
Outdated
@@ -161,3 +163,46 @@ function oe_bootstrap_theme_preprocess_pattern_featured_media(&$variables) { | |||
function oe_bootstrap_theme_preprocess_pattern_inpage_navigation(&$variables): void { | |||
$variables['inpage_navigation_id'] = $variables['attributes']['id'] ?? Html::getUniqueId('bcl-inpage-navigation'); | |||
} | |||
|
|||
/** | |||
* Implements hook_preprocess_menu(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest "Impelements hook_preprocess_HOOK() for 'pattern_navigation'.". But look at other functions how we do it there.
includes/menu.inc
Outdated
'path' => $item['url']->toString(), | ||
'attributes' => new Attribute(['class' => $class]), | ||
'path' => $item['url'] instanceof Url ? $item['url']->toString() : '', | ||
'attributes' => new Attribute(['class' => $item['in_active_trail'] ? ['active'] : []]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long line.
I see that this was previously done with a local variable $class
.
In my previous review I mentioned that we do too much "juggling with variable". But I don't think I meant this one.
Easiest fix is to simply introduce line breaks, like so:
'attributes' => new Attribute([
'class' => $item['in_active_trail'] ? ['active'] : [],
]),
We can debate whether or not we should use a local variable here, and whether we should omit the 'attributes' altogether if we are not adding any class. But it seems good enough to me with just line breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the approach, doing this in a pattern preprocess would have been wrong architecturally because the pattern preprocess and pattern template should expect the values as defined in the yml file.
resolves #156 |
91a2038
to
e05213a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still ok after rebase.
Jira issue(s):