OEL-2002: Preserve Drupal links. #270

drishu · 2022-09-12T16:59:29Z

Jira issue(s):

D8AGE-???

drishu · 2022-09-14T14:13:31Z

includes/menu.inc

-        'class' => $item['in_active_trail'] ? ['active'] : [],
-      ]),
-    ];
+  $menu_preprocess = \Drupal::classResolver(MenuPreprocess::class);


I dont think this is actually needed with this class, we could just instantiate with new

templates/patterns/navigation/navigation.ui_patterns.yml

patches/npm/@openeuropa+bcl-theme-default+0.24.1.patch

drishu

One more thing, I want to add a new test for it, or change the link pattern render test where we cover all possible scenarios of urls, anchors, external links, full internal urls, uris, url objects whatever else.

brummbar · 2022-09-27T17:03:00Z

patches/npm/@openeuropa+bcl-theme-default+0.24.1.patch

+{% if _icon_markup is defined %}
+  {%- set _label -%}
+    {%- if _icon_position == 'before'  -%}
+      {{ _icon_markup|raw }}


Is the |raw needed? I tried locally and it seems it wasn't. When we set rendered html to a variable in a twig file, it gets set as safe markup.

templates/overrides/menu/menu-local-tasks.html.twig

templates/patterns/icon/pattern-icon.html.twig

tests/src/PatternAssertion/LinkPatternAssert.php

modules/oe_bootstrap_theme_helper/src/TwigExtension/TwigExtension.php

brummbar · 2022-10-04T16:15:38Z

modules/oe_bootstrap_theme_helper/tests/src/Kernel/TwigExtensionTest.php

+            'id' => 'id-bar',
+          ],
+        ],
+      ],


I think we miss a scenario with external url.

brummbar · 2022-10-04T16:20:50Z

tests/src/PatternAssertion/BasePatternAssert.php

+    $this->assertElementExists($selector, $crawler);
+    $class = $crawler->filter($selector)->attr('class');
+
+    self::assertNotNull($class, \sprintf(


Nit: no backslashes.

brummbar · 2022-10-04T16:33:04Z

tests/src/PatternAssertion/LinkPatternAssert.php

+  protected function assertBaseElements(string $html, string $variant): void {
+    $crawler = new Crawler($html);
+
+    $this->assertElementExists('a', $crawler);


We should probably do 'body > a' to check that it's first level.

brummbar · 2022-10-04T16:33:49Z

tests/src/PatternAssertion/LinkPatternAssert.php

+   *   The crawler.
+   */
+  protected function assertIcon(string $expected, Crawler $crawler): void {
+    $svg = $crawler->filter('svg');


We should body > a > svg to check that it's rendered inside.

brummbar · 2022-10-04T16:34:51Z

tests/src/PatternAssertion/LinkPatternAssert.php

+   */
+  protected function assertSettings(array $expected, Crawler $crawler): void {
+    if (!empty($expected['disabled'])) {
+      $this->assertElementHasClass('disabled', 'a', $crawler);


Actually we don't need this method at all. Since we know there's only one a tag, we can simply assert that a.disabled exists.

brummbar · 2022-10-04T16:35:08Z

tests/src/PatternAssertion/LinkPatternAssert.php

+      $this->assertElementAttribute('true', 'a', 'aria-disabled', $crawler);
+    }
+    else {
+      $this->assertElementNotHasClass('disabled', 'a', $crawler);


This the same as above, but asserting that doesn't exist.

brummbar · 2022-10-04T17:30:51Z

src/MenuPreprocess.php

+  public function prepareLocalTasks(array $local_tasks): array {
+    $links = [];
+    foreach ($local_tasks as $link) {
+      if ($link['#access']->isForbidden()) {


Core handles this differently: see build/core/lib/Drupal/Core/Render/Renderer.php:226.
Basically access is granted only if access is set to allowed, or to true.
In our case we will always have objects (\Drupal\Core\Menu\LocalTaskManager::getTasksBuild()).

brummbar · 2022-10-04T17:32:06Z

tests/src/Kernel/MenuLocalTasksTest.php

+          ],
+          '#active' => FALSE,
+          '#weight' => 0,
+          '#access' => $access_forbidden,


Let's add a case where access is neutral. Also in that case the access should be prevented.

drishu changed the title ~~Oel 2002~~ OEL-2002: Preserve Drupal links. Sep 12, 2022

drishu force-pushed the OEL-2002 branch 4 times, most recently from b83a263 to b438c8e Compare September 14, 2022 13:57

drishu commented Sep 14, 2022

View reviewed changes

drishu commented Sep 15, 2022

View reviewed changes

drishu force-pushed the OEL-2002 branch 2 times, most recently from 4549cd7 to 1b23ac4 Compare September 26, 2022 14:35

brummbar requested changes Sep 27, 2022

View reviewed changes

drishu commented Sep 28, 2022

View reviewed changes

modules/oe_bootstrap_theme_helper/src/TwigExtension/TwigExtension.php Show resolved Hide resolved

brummbar requested changes Oct 4, 2022

View reviewed changes

drishu force-pushed the OEL-2002 branch 5 times, most recently from 8da1459 to 39d8812 Compare October 7, 2022 06:51

drishu added 13 commits October 7, 2022 09:53

OEL-2002: Presereve drupal links in menu preprocess.

6f91baf

OEL-2002: Add bcl_link twig function.

900bc3a

OEL-2002: Account for access in menu_local_tasks.

850c081

OEL-2002: Remove duplicate local tasks template.

6a95171

OEL-2002: Move local actions template into menu folder.

6aa2ec8

OEL-2002: Account for string urls.

41aec0b

OEL-2002: Add BC for label and path.

af1792e

OEL-2002: Add icon support for links.

d1c12c2

OEL-2002: Move empty url bailout earlier.

41d6313

OEL-2002: Fix broken navigation link.

fc58709

OEL-2002: Rework bcl breadcrump template to use drupal links.

d97c834

OEL-2002: Fix submenu not appearing.

215dcad

OEL-2002: Move BCL parameter processing back to template.

c5c4ba6

drishu added 26 commits October 7, 2022 09:53

OEL-2002: Prevent cluttering in menu preprocessing.

403a70e

OEL-2002: Use drupal protocols in path exceptions.

5992bdf

OEL-2002: Adapt test to local tasks changes.

ae6a3f9

OEL-2002: Account for hashtag links.

e8b6649

OEL-2002: Wrap title override with condition.

e91b468

OEL-2002: Add extra_classes to menu link preprocess.

340a9f2

OEL-2002: Implement Url::fromInternalUri into bclLink.

aa8cc63

OEL-2002: Remove 4th bclLink parameter from patch.

46d5f81

OEL-2002: Add css class helpers to BasePatternAssert.

9e39b7f

OEL-2002: Add LinkPatternAssert to markup rendering test.

6f3817e

OEL-2002: Add TwigExtension test.

45864aa

OEL-2002: Fix 'none' shown instead of '/'.

9c92b65

OEL-2002: Fix NotFoundHttpException in markup rendering test.

c9cf981

OEL-2002: Remove redundant raw filter.

bad8ed6

OEL-2002: Assert equal instead of identical to prevent false negatives.

eae6e27

OEL-2002: Restore menu local task titles.

746ccfa

OEL-2002: Follow core behaviour for links with string paths.

ff3cce0

OEL-2002: Add attributes to icons.

eeaf894

OEL-2002: Move all menu preprocessing into utility class.

31dc910

OEL-2002: Remove useless whitespace in patch.

ac803e7

OEL-2002: Let php resolve sprintf namespace.

37bce25

OEL-2002: Check link is fist level in pattern assert.

b489479

OEL-2002: Assert element order in link icon assert.

aa1c78c

OEL-2002: Streamline link testing.

cf0cc0e

OEL-2002: Ensure neutral link does not pass in local tasks.

c1f36aa

OEL-2002: Add external link test.

ffc7cab

drishu force-pushed the OEL-2002 branch from 39d8812 to ffc7cab Compare October 7, 2022 06:54

brummbar approved these changes Oct 7, 2022

View reviewed changes

brummbar merged commit f5b0c71 into 1.x Oct 12, 2022

brummbar deleted the OEL-2002 branch October 12, 2022 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OEL-2002: Preserve Drupal links. #270

OEL-2002: Preserve Drupal links. #270

drishu commented Sep 12, 2022

drishu Sep 14, 2022 •

edited

Loading

drishu left a comment

brummbar Sep 27, 2022

brummbar Oct 4, 2022

brummbar Oct 4, 2022

brummbar Oct 4, 2022

brummbar Oct 4, 2022

brummbar Oct 4, 2022

brummbar Oct 4, 2022

brummbar Oct 4, 2022

brummbar Oct 4, 2022

OEL-2002: Preserve Drupal links. #270

OEL-2002: Preserve Drupal links. #270

Conversation

drishu commented Sep 12, 2022

drishu Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

drishu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drishu Sep 14, 2022 •

edited

Loading