Issue summary updated: #42
Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it's updating code to the new API/code style
Issue priority Major because it's a refactoring task
Unfrozen changes Unfrozen because it is a change in automated tests
Prioritized changes The main goal of this issue is removing deprecated code style.
Disruption The change is not disruptive
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

vijaycs85’s picture

Status: Active » Closed (won't fix)

Need to rewrite the whole module to make test sync with current test implementation. For more details, please refer: #1988802: [META] Rewrite test modules in system to provide better unit testing.

ayelet_Cr’s picture

Status: Closed (won't fix) » Active
disasm’s picture

Assigned: Unassigned » disasm
Status: Active » Needs review
FileSize
1.77 MB

converting test_page_test_page to a new controller and updating all the menu hooks that call it.

disasm’s picture

forgot to rebase before creating the diff.

Status: Needs review » Needs work

The last submitted patch, drupal8.system-module.1987868-5.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
12.16 KB

Try again with the correct namespace.

Status: Needs review » Needs work

The last submitted patch, drupal8.system-module.1987868-7.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
5.09 KB
21.17 KB

wrong key on hook_menu entries (route name -> route_name).

disasm’s picture

forgot to rebase

Status: Needs review » Needs work

The last submitted patch, drupal8.system-module.1987868-10.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
7.95 KB
11.91 KB

Moving testPage method to Test Controller added by #2032535: Resolve 'title' using the route and render array.

Status: Needs review » Needs work

The last submitted patch, drupal8.system-module.1987868-12.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
1005 bytes
46.93 KB

attached is a patch that converts drupal_add_js and drupal_set_title to render array.

Status: Needs review » Needs work

The last submitted patch, drupal8.test_page_test.1987868-14.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
527 bytes
46.93 KB

I really need to run phpcs on patches when I work on them after midnight ;-) Fixing two syntax errors.

Status: Needs review » Needs work

The last submitted patch, drupal8.test_page_test.1987868-16.patch, failed testing.

jibran’s picture

Why we have a comment out code in the patch?

disasm’s picture

Status: Needs work » Needs review
FileSize
1004 bytes
12.03 KB

I really messed up that branch last night. This interdiff goes back to #12.

Status: Needs review » Needs work

The last submitted patch, drupal8.test_page_test.1987868-19.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
22.8 KB
564 bytes
12.12 KB

Adding hook_menu back in. Some reason when the entry doesn't exist, no menus show up at all. This is a temporary fix until we figure out what the reason for it is.

Screen Shot 2013-08-31 at 1.34.26 AM.png

Status: Needs review » Needs work

The last submitted patch, drupal8.test_page_test.1987868-21.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
685 bytes
12.13 KB

fixing more tests. If this is green, will open up another issue to resolve the menu issue when no hook_menu exists.

Status: Needs review » Needs work

The last submitted patch, drupal8.test_page_test.1987868-23.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
709 bytes
12.13 KB

Some more test fixes.

xjm’s picture

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

dawehner’s picture

Status: Needs review » Needs work

Missing title on all of them.

  1. +++ b/core/modules/system/tests/modules/test_page_test/lib/Drupal/test_page_test/Controller/Test.php
    @@ -26,4 +26,23 @@ public function renderTitle() {
    +   *
    +   */
    +  public function testPage() {
    

    Missing @return.

  2. +++ b/core/modules/system/tests/modules/test_page_test/test_page_test.module
    @@ -1,26 +1,9 @@
    - */
     function test_page_test_menu() {
       $items['test-page'] = array(
         'title' => 'Test front page',
    -    'page callback' => 'test_page_test_page',
    -    'access callback' => TRUE,
    +    'route_name' => 'test_page_test_page',
         'type' => MENU_CALLBACK,
       );
    
    +++ b/core/modules/system/tests/modules/test_page_test/test_page_test.routing.yml
    @@ -4,3 +4,11 @@ test_page_render_title:
    +
    +test_page_test_page:
    +  pattern: '/test-page'
    +  defaults:
    +    _content: '\Drupal\test_page_test\Controller\Test::testPage'
    +  requirements:
    +    _access: 'TRUE'
    +
    

    This menu callback can be removed entirely. ... also missing title on the routing yml

disasm’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
12.13 KB

removing hook_menu. Adding @return, adding title.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion, -MENU_LOCAL_ACTION

The last submitted patch, drupal8.test_page.1987868-28.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

#28: drupal8.test_page.1987868-28.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion, +MENU_LOCAL_ACTION

The last submitted patch, drupal8.test_page.1987868-28.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
564 bytes
120.47 KB

adding hook_menu back in.

Status: Needs review » Needs work

The last submitted patch, drupal8.test_page.1987868-32.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
12.18 KB

reroll!

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion, -MENU_LOCAL_ACTION

The last submitted patch, drupal8.test_page.1987868-34.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion, +MENU_LOCAL_ACTION

#34: drupal8.test_page.1987868-34.patch queued for re-testing.

dawehner’s picture

+++ w/core/modules/system/tests/modules/test_page_test/test_page_test.module
@@ -1,26 +1,9 @@
   $items['test-page'] = array(
     'title' => 'Test front page',
-    'page callback' => 'test_page_test_page',
-    'access callback' => TRUE,
+    'route_name' => 'test_page_test_page',
     'type' => MENU_CALLBACK,
   );

We could just remove that.

disasm’s picture

sadly, that can't be removed without altering tests: Path from menu_get_item('test-page') is equal to 'test-page'. If you remove it, menu_get_item doesn't know
about test-page.

xjm’s picture

Status: Needs review » Needs work

The last submitted patch, 34: drupal8.test_page.1987868-34.patch, failed testing.

xjm’s picture

Issue summary: View changes
Issue tags: +Needs reroll
valthebald’s picture

Issue summary: View changes

Added beta stage evaluation

valthebald’s picture

Assigned: disasm » valthebald
Issue tags: +#SprintWeekend2015

Working on it as part of #SprintWeekend2015

valthebald’s picture

Status: Needs work » Closed (duplicate)

This issue is already solved by [#2301137]

David Hernández’s picture

Issue tags: -#SprintWeekend2015 +SprintWeekend2015

Hello!

Thank you for working on this issue!

We should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015 with no #.

penyaskito’s picture

@valthebald Wrong issue link?

DamienMcKenna’s picture

Issue tags: -Needs reroll

Removed the "Needs reroll" seeing as the issue was closed.