Follow-up from #1971384: [META] Convert page callbacks to controllers.

73 instances of 31 callbacks. Something to do on the plane!

Files: 
CommentFileSizeAuthor
#44 page-callback-2091691-44.patch141.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,770 pass(es).
[ View ]
#44 interdiff.txt1.03 KBtim.plunkett
#41 page-callback-2091691-41.patch142.87 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,772 pass(es).
[ View ]
#42 interdiff.txt16.31 KBtim.plunkett
#39 page-callback-2091691-39.patch141.23 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,709 pass(es).
[ View ]
#37 interdiff.txt10.12 KBtim.plunkett
#37 page-callback-2091691-37.patch127.8 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,710 pass(es).
[ View ]
#35 page-callback-2091691-32.patch134.53 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,840 pass(es).
[ View ]
#35 interdiff.txt1003 bytestim.plunkett
#31 interdiff-from-17.txt27.73 KBtim.plunkett
#30 page-callback-2091691-30.patch133.55 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,805 pass(es), 19 fail(s), and 0 exception(s).
[ View ]
#30 interdiff.txt16.61 KBtim.plunkett
#29 page-callback-2091691-29.patch135.03 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,772 pass(es).
[ View ]
#29 interdiff.txt7.67 KBtim.plunkett
#27 page-callback-2091691-27.patch132.59 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,650 pass(es), 85 fail(s), and 4 exception(s).
[ View ]
#27 interdiff.txt998 bytestim.plunkett
#25 page-callback-2091691-25.patch131.62 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,069 pass(es), 346 fail(s), and 44 exception(s).
[ View ]
#25 interdiff.txt8.91 KBtim.plunkett
#23 page-callback-2091691-23.patch123.87 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,696 pass(es), 21 fail(s), and 1 exception(s).
[ View ]
#23 interdiff.txt11.58 KBtim.plunkett
#17 page-callback-2091691-17.patch114.48 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,417 pass(es), 87 fail(s), and 14 exception(s).
[ View ]
#17 interdiff.txt7.38 KBtim.plunkett
#13 page-callback-2091691-13.patch112.06 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,414 pass(es), 48 fail(s), and 1 exception(s).
[ View ]
#11 page-callback-2091691-11.patch109.39 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,600 pass(es), 185 fail(s), and 1 exception(s).
[ View ]
#9 page-callback-2091691-9.patch80.73 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,672 pass(es), 197 fail(s), and 3 exception(s).
[ View ]
#7 page-callback-2091691-7.patch53.07 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,826 pass(es), 42 fail(s), and 33 exception(s).
[ View ]
#5 page-callback-2091691-5.patch48.88 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,177 pass(es), 393 fail(s), and 29 exception(s).
[ View ]
#3 test-non-form-2091691-3.patch25.38 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,639 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
#1 test-non-form-2091691-1.patch16.25 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 59,047 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new16.25 KB
FAILED: [[SimpleTest]]: [MySQL] 59,047 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Here's a start.

StatusFileSize
new25.38 KB
FAILED: [[SimpleTest]]: [MySQL] 58,639 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Here are some more.

StatusFileSize
new48.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,177 pass(es), 393 fail(s), and 29 exception(s).
[ View ]

More work, still some left.

StatusFileSize
new53.07 KB
FAILED: [[SimpleTest]]: [MySQL] 58,826 pass(es), 42 fail(s), and 33 exception(s).
[ View ]

StatusFileSize
new80.73 KB
FAILED: [[SimpleTest]]: [MySQL] 58,672 pass(es), 197 fail(s), and 3 exception(s).
[ View ]

63 down, 11 to go.

StatusFileSize
new109.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,600 pass(es), 185 fail(s), and 1 exception(s).
[ View ]

Okay, this is all of them, but will still have failures.

StatusFileSize
new112.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,414 pass(es), 48 fail(s), and 1 exception(s).
[ View ]

Removed:
testModuleInvokeAllDuringLoadFunction()
testMenuLoadArgumentsInheritance()

Neither of these are relevant anymore, they do not work with routes at all.

StatusFileSize
new7.38 KB
new114.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,417 pass(es), 87 fail(s), and 14 exception(s).
[ View ]

I think I found the last couple fails.

  1. +++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
    @@ -84,7 +84,9 @@ public function content(Request $request, $_content) {
         if (!is_array($page_content)) {
           $page_content = array(
    -        '#markup' => $page_content,
    +        'main' => array(
    +          '#markup' => $page_content,
    +        ),

    Wait, are you sure about that change?

  2. +++ b/core/modules/block/custom_block/tests/modules/custom_block_test/custom_block_test.module
    @@ -92,10 +92,7 @@ function custom_block_test_menu() {
       $items['custom-block/%custom_block'] = array(
         'title callback' => 'entity_page_label',
         'title arguments' => array(1),
    +++ b/core/modules/block/custom_block/tests/modules/custom_block_test/custom_block_test.routing.yml
    +++ b/core/modules/block/custom_block/tests/modules/custom_block_test/custom_block_test.routing.yml
    @@ -0,0 +1,6 @@
    @@ -0,0 +1,6 @@
    +custom_block_test.custom_block_view:
    +  path: '/custom-block/{custom_block}'
    +  defaults:
    +    _entity_view: 'custom_block'
    +  requirements:
    +    _entity_access: 'custom_block.view'
    diff --git a/core/modules/content_translation/content_translation.local_tasks.yml b/core/modules/content_translation/content_translation.local_tasks.yml
    diff --git a/core/modules/content_translation/content_translation.local_tasks.yml b/core/modules/content_translation/content_translation.local_tasks.yml
    new file mode 100644
    new file mode 100644
    index 0000000..151c584

    So is there a reason why we don't have a title callback on the route?

  3. +++ b/core/modules/block/custom_block/tests/modules/custom_block_test/custom_block_test.routing.yml
    --- /dev/null
    +++ b/core/modules/content_translation/content_translation.local_tasks.yml
    +++ b/core/modules/content_translation/content_translation.local_tasks.yml
    +++ b/core/modules/content_translation/content_translation.local_tasks.yml
    @@ -0,0 +1,3 @@
    @@ -0,0 +1,3 @@
    +content_translation.local_tasks:
    +  derivative: 'Drupal\content_translation\Plugin\Derivative\ContentTranslationLocalTasks'
    +  weight: 100

    This seems to be out of scope?

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.php
    @@ -555,57 +555,6 @@ protected function menuLoadRouter($router_path) {
       /**
    -   * Tests inheritance of 'load arguments'.
    -   */
    -  function testMenuLoadArgumentsInheritance() {

    nice!!

  5. +++ b/core/modules/system/tests/modules/menu_test/menu_test.module
    @@ -197,181 +169,130 @@ function menu_test_menu() {
    +    // Title gets completely ignored. Good thing, too.
    +    'title' => 'Bike sheds full of blue smurfs',
    +    'title callback' => 'menu_test_title_callback',
    +    // If '4' is not in quotes, the argument becomes arg(4).
    +    'title arguments' => array('Example title', '4'),

    We can't convert this to a title callback on the route?

  6. +++ b/core/modules/system/tests/modules/menu_test/menu_test.routing.yml
    @@ -159,3 +199,340 @@ menu_test.optional_placeholder:
    +  # @todo Find a way to use the correct path.
    +  #path: "/menu-test/ -._~!$'"()*@[]?&+%#,;=:%23%25%26%2B%2F%3Féøïвβ中國書۞"

    WOW

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Plugin/Derivative/ContentTranslationLocalTasks.php
@@ -0,0 +1,91 @@
+        $path = '/' . str_replace($entity_info['menu_path_wildcard'], '{entity}', $entity_info['menu_base_path']);
+        $routes = $this->routeProvider->getRoutesByPattern($path)->all();
+        if (empty($routes)) {
+          continue;
+        }
+

It would be great to add a todo that entities should not deal with paths but rather set the route names.

18.1
See drupal_set_page_content(), the fallback uses 'main', and our route based one was written wrong.

18.2
I've opened #2102391: Provide a _title_callback replacement for entity_page_label() for entity_page_label()

18.3
We had test coverage for this (see the fails in #15) so I converted it

18.5
I don't actually touch those lines, just indenting them...

19
That will be fixed by #1810350: Remove menu_* entity keys and replace them with entity links, so I'm not too worried.

That will be fixed by #1810350: Remove menu_* entity keys and replace them with entity links, so I'm not too worried.

Well at least the patch you have pointed just adds even more paths to the entity definitionm, anyway.

StatusFileSize
new11.58 KB
new123.87 KB
FAILED: [[SimpleTest]]: [MySQL] 58,696 pass(es), 21 fail(s), and 1 exception(s).
[ View ]

Ugh, stupid fixes needed in menu.inc.

Also, git bisect + simpletest is depressing, but it works.

StatusFileSize
new8.91 KB
new131.62 KB
FAILED: [[SimpleTest]]: [MySQL] 58,069 pass(es), 346 fail(s), and 44 exception(s).
[ View ]

I went down the rabbit hole there, but @dawehner had a good suggestion.

StatusFileSize
new998 bytes
new132.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,650 pass(es), 85 fail(s), and 4 exception(s).
[ View ]

StatusFileSize
new7.67 KB
new135.03 KB
PASSED: [[SimpleTest]]: [MySQL] 58,772 pass(es).
[ View ]

StatusFileSize
new16.61 KB
new133.55 KB
FAILED: [[SimpleTest]]: [MySQL] 58,805 pass(es), 19 fail(s), and 0 exception(s).
[ View ]

Okay, while that passed, I think its not ideal.
I went that way because I couldn't get the local tasks to work, but I think I figured it out.

Also switching from $entity to $_entity as we do everywhere else when it's an internal param.

StatusFileSize
new27.73 KB

For anyone following along, here's the interdiff from the last reviewed patch (#17)

StatusFileSize
new1003 bytes
new134.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,840 pass(es).
[ View ]

Ugh, I broke d.o. Missed one change.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Routing/RouteSubscriber.php
@@ -42,9 +42,12 @@ public function routes(RouteBuildEvent $event) {
       $route = new Route(
-        "$entity_type/manage/{" . $entity_type . '}',
-        array('_content' => '\Drupal\entity_test\Controller\EntityTestController::testEdit', 'entity_type' => $entity_type),
-        array('_permission' => 'administer entity_test content')
+        "$entity_type/manage/{_entity}",
+        array('_content' => '\Drupal\entity_test\Controller\EntityTestController::testEdit'),
+        array('_permission' => 'administer entity_test content'),
+        array('parameters' => array(
+          '_entity' => array('type' => 'entity:' . $entity_type),
+        ))

This change is kind of odd as I kind of expect that both usecases are valid.

StatusFileSize
new127.8 KB
PASSED: [[SimpleTest]]: [MySQL] 58,710 pass(es).
[ View ]
new10.12 KB

Okay, switching them back.

  1. +++ b/core/includes/menu.inc
    @@ -1994,14 +1994,15 @@ function menu_local_tasks($level = 0) {
    +    if ($router_item) {
         // Get all tabs (also known as local tasks) and the root page.
    @@ -2170,15 +2171,16 @@ function menu_local_tasks($level = 0) {
         $data['tabs'] += $tabs;
    +    }

    so we added an if, lets add the indentation too

  2. +++ b/core/modules/forum/forum.module
    @@ -148,11 +148,11 @@ function forum_menu() {
    +  if (in_array($route_name, array('forum.index', 'forum.page'))) {

    win!

  3. +++ b/core/modules/system/tests/modules/menu_test/menu_test.routing.yml
    @@ -166,3 +206,340 @@ menu_test.optional_placeholder:
    +  # @todo Find a way to use the correct path.
    +  #path: "/menu-test/ -._~!$'"()*@[]?&+%#,;=:%23%25%26%2B%2F%3Féøïвβ中國書۞"

    i guess this is not blocking

  4. +++ b/core/modules/system/tests/modules/system_test/system_test.module
    @@ -3,84 +3,10 @@
    -  $items['system-test/auth'] = array(
    -    'page callback' => 'system_test_basic_auth_page',
    -    'access callback' => TRUE,
    -    'type' => MENU_CALLBACK,
    -  );

    i cant see this one added as route. removed deliberately?

  5. +++ b/core/modules/update/tests/modules/update_test/update_test.module
    @@ -18,22 +18,6 @@ function update_test_system_theme_info() {
    -function update_test_menu() {

    awesomeness

StatusFileSize
new141.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,709 pass(es).
[ View ]

1) The diff is now going to look insane, be warned :)

3) I don't think it should be.

4) The usage of that callback was removed back in #1862398: [meta] Replace drupal_http_request() with Guzzle, so I just removed the callback

No interdiff because its just the indentation fix.

Status:Needs review» Reviewed & tested by the community

yeah, well this identation change made that patch unreviewable:P
i am glad i reviewed it on its previous form

lets get this in quickly, unless bot disagress

StatusFileSize
new142.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,772 pass(es).
[ View ]

Actually, I find that incredibly hard to understand, so I'm splitting it out.

The interdiff is against #37.

Actually, due to #2103877: Patch uploads are failing I can't upload it. will as soon as I can

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new16.31 KB

29/30 uploads give me a 500. yay.

Status:Needs review» Reviewed & tested by the community

a lot better indeed. Thanks!

StatusFileSize
new1.03 KB
new141.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,770 pass(es).
[ View ]

In IRC, @alexpott pointed out that until #2084463: Convert contextual links to a plugin system similar to local tasks/actions happens, we can't make those changes to content_translation_menu(). Restored that.

Here is a "reverse review" of the patch, highlighting some oddities. Most of this is at the top of the patch, the end of the patch is pretty standard.

  1. +++ b/core/includes/menu.inc
    @@ -1983,202 +1978,30 @@ function menu_local_tasks($level = 0) {
    -    if (!$router_item || !$router_item['access']) {
    +    if ((!$route_name && !$router_item) || ($router_item && !$router_item['access'])) {
    ...
         // Allow modules to dynamically add further tasks.
         $module_handler = \Drupal::moduleHandler();
         foreach ($module_handler->getImplementations('menu_local_tasks') as $module) {
           $function = $module . '_menu_local_tasks';
    -      $function($data, $router_item, $root_path);
    +      $function($data, $route_name);
         }
         // Allow modules to alter local tasks.
    -    $module_handler->alter('menu_local_tasks', $data, $router_item, $root_path);
    +    $module_handler->alter('menu_local_tasks', $data, $route_name);

    These are the only functional changes here. The rest is just moving the GIANT hunk of legacy code into a helper method, because it was very confusing to me which was legacy and which wasn't.

    The top hunk is fixing a bug where the hook_menu_local_tasks/hook_menu_local_tasks_alter weren't being called for plugin-based tasks. It was trying to bail out early when a menu_router item wasn't found, but that's not enough anymore.

    The second hunk was due to huge WTFs with the alters, since *sometimes* menu_get_item() will return the hook_menu() parent of a new routing.yml path. In cleaning that up, we now only pass along the route name.

  2. +++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
    @@ -84,7 +84,9 @@ public function content(Request $request, $_content) {
         if (!is_array($page_content)) {
           $page_content = array(
    -        '#markup' => $page_content,
    +        'main' => array(
    +          '#markup' => $page_content,
    +        ),
           );
         }

    As explained above, this is a bug fix to make it match drupal_set_page_content(). The test for this behavior was only testing page callbacks, and now that its testing routes we found this mistake.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNonNodeTest.php
    @@ -372,7 +372,7 @@ function testCommentFunctionality() {
    -    $this->drupalGet('entity_test_render/manage/' . $new_entity->id() . '/edit');
    +    $this->drupalGet('entity_test_render/manage/' . $new_entity->id());
    +++ b/core/modules/datetime/lib/Drupal/datetime/Tests/DatetimeFieldTest.php
    @@ -112,7 +112,7 @@ function testDateField() {
    -    preg_match('|entity_test/manage/(\d+)/edit|', $this->url, $match);
    +    preg_match('|entity_test/manage/(\d+)|', $this->url, $match);

    These two, and all hunks like them, are more of the same changes we've made elsewhere. This is the MENU_DEFAULT_LOCAL_TASK path, which is never presented as a link in the D7 UI, but is used in test code. They would fall through to the real path in D7, but will 404 in D8.

  4. +++ b/core/modules/content_translation/content_translation.local_tasks.yml
    @@ -0,0 +1,4 @@
    +content_translation.local_tasks:
    +  derivative: 'Drupal\content_translation\Plugin\Derivative\ContentTranslationLocalTasks'
    +  class: 'Drupal\content_translation\Plugin\ContentTranslationLocalTasks'
    +++ b/core/modules/content_translation/lib/Drupal/content_translation/Plugin/ContentTranslationLocalTasks.php
    @@ -0,0 +1,41 @@
    +class ContentTranslationLocalTasks extends LocalTaskDefault {
    +++ b/core/modules/content_translation/lib/Drupal/content_translation/Plugin/Derivative/ContentTranslationLocalTasks.php
    @@ -0,0 +1,92 @@
    +class ContentTranslationLocalTasks extends DerivativeBase implements ContainerDerivativeInterface {

    All of this is needed because content_translation needs to be all things to all entity types. The config_translation module is having some of the same pains, which I was just recently helped with. This is not normal, unless your module is trying to be a generic layer on top of a dozen things, you won't have to write stuff like this

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.php
    @@ -555,57 +555,6 @@ protected function menuLoadRouter($router_path) {
    -  function testMenuLoadArgumentsInheritance() {
    +++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleApiTest.php
    @@ -137,19 +137,6 @@ function testModuleInvokeAll() {
    -  function testModuleInvokeAllDuringLoadFunction() {

    These tests are removed because we're removing support for magic menu loaders (%node => node_load), you need a ParamConverter

  6. +++ b/core/modules/system/tests/modules/ajax_test/ajax_test.module
    @@ -16,34 +16,10 @@
    -  $items['ajax-test/render'] = array(
    -    'title' => 'ajax_render',
    -    'page callback' => 'ajax_test_render',
    -    'access callback' => TRUE,
    -    'type' => MENU_CALLBACK,
    -  );

    MENU_CALLBACKs don't need the leftover hook_menu definitions when converted

  7. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
    @@ -61,9 +61,12 @@ function entity_test_entity_types($filter = NULL) {
    -  // Optionally specify a translation handler for testing translations.
    -  if (\Drupal::state()->get('entity_test.translation')) {
    -    foreach(entity_test_entity_types() as $entity_type) {
    +  foreach (entity_test_entity_types() as $entity_type) {
    +    // Ensure these test entity types use their base path as their edit path.
    +    $info[$entity_type]['menu_edit_path'] = $info[$entity_type]['menu_base_path'];
    +
    +    // Optionally specify a translation handler for testing translations.
    +    if (\Drupal::state()->get('entity_test.translation')) {

    Just switching from doing 1 thing in a loop to 2 things

Status:Reviewed & tested by the community» Needs work
Issue tags:-WSCCI, -FormInterface, -WSCCI-conversion, -Approved API change

The last submitted patch, page-callback-2091691-44.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI, +FormInterface, +WSCCI-conversion, +Approved API change

#44: page-callback-2091691-44.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest264828menu_links' doesn't exist

Testbot fluke.

Status:Reviewed & tested by the community» Fixed

So I had a huge review written out which some combination of Dreditor and Firefox then lost, but it's OK because all of the points I was going to bring up (and then some) were already pre-emptively addressed in #44.

Just one question:

+++ b/core/includes/menu.inc
@@ -2195,6 +2018,189 @@ function menu_local_tasks($level = 0) {
+ * @deprecated Remove once all local tasks/actions are converted to plugins.
+ */
+function _menu_get_legacy_tasks($router_item, &$data, &$root_path) {

What's left to complete this so we can remove these 100+ lines of inscrutable code? :)

Committed and pushed to 8.x. Thanks!!!! :D

Thank you!

I think #2102125: Big Local Task Conversion is the major blocker for that, but possibly also #2102653: Convert test form callbacks to new form controller

Assigned:tim.plunkett» Unassigned

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