Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Assigned: Unassigned » damiankloip

I'll knock this one off the list.

InternetDevels’s picture

As this issue hasn`t been fixed yet we are going to work on it today during Code Sprint UA

InternetDevels’s picture

Assigned: InternetDevels » Unassigned
Status: Active » Needs review
Issue tags: +CodeSprintUA
FileSize
2.96 KB

Patch attached.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#3 looks good
due to CodeSprintUA - bot overloaded and patch successfully run on dev server via all tests
If bot green - RTBC

Status: Reviewed & tested by the community » Needs work
Issue tags: -WSCCI-conversion, -CodeSprintUA

The last submitted patch, drupal-convert_menu_login_callback-1987734-3.patch, failed testing.

RoSk0’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion, +CodeSprintUA
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This was just a random test error.

ramlev’s picture

Assigned: Unassigned » ramlev
ramlev’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.04 KB

Rerolled the patch to work on 8.x

juampynr’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/TestControllers.phpundefined
@@ -15,6 +15,13 @@
 class TestControllers {

Missing documentation blocks in this file, plus the class should implement ControllerInterface.

See https://drupal.org/node/1953346 for an example.

+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/TestControllers.phpundefined
@@ -15,6 +15,13 @@
+   * Returns page to be used as a login path.

This needs a docblock.

juampynr’s picture

Ignore my comment above.

I wonder why this is not a controller and instead is a POPO. Shouldn't this implement ControllerInterface?

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community

I think this is fine, as you only really need ControllerInterface if you have dependencies you need to inject.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/tests/modules/menu_test/menu_test.moduleundefined
@@ -314,8 +314,7 @@ function menu_test_menu() {
   $items['menu_login_callback'] = array(
     'title' => 'Used as a login path',
-    'page callback' => 'menu_login_callback',
-    'access callback' => TRUE,
+    'route_name' => 'menu_login_callback',
   );

let's remove the whole menu item here...

juampynr’s picture

Status: Needs work » Needs review
FileSize
3.14 KB
708 bytes

Here you are.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect. Potentially you dropped the "()" but I don't care at all.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 35ab9ae and pushed to 8.x. Thanks!

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