Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nick_schuch’s picture

Status: Active » Needs review
FileSize
724 bytes

Here we go!

larowlan’s picture

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTestBasic.php
@@ -48,6 +48,12 @@
+    \Drupal::config('system.theme')->set('admin', 'seven')->save();

Use $this->container->get('config.factory')->get('system.theme')->set('admin', 'seven')->save() instead

nick_schuch’s picture

larowlan’s picture

+++ b/core/modules/tour/tests/tour_test/tour_test.routing.yml
@@ -5,6 +5,13 @@ tour_test_1:
+    _content: '\Drupal\tour_test\Controller\TourTestController::tourTest1'

Should we add the controller just to avoid confusion? Not sure on precedence here.

Other than that, looks ok, I guess the local action could be declared in yml instead of hook_menu() but I'm not sure where thats at yet.

larowlan’s picture

Sorry Drupal\tour_test\Controller\TourTestController::tourTest1 already exists.
As discussed in irc, pwolanin would prefer new local actions use yml instead of hook_menu, makes #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit simpler

nick_schuch’s picture

As per discussion in IRC. We need to use a yml implementation over hook_menu. Here is the reroll.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

unless bot disagrees

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
3.1 KB

Thanks alexpott! Rerolled. Selector change in the TourTest that are not required anymore.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

back we go

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.