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

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
16.25 KB

Here's a start.

tim.plunkett’s picture

Here are some more.

tim.plunkett’s picture

More work, still some left.

tim.plunkett’s picture

tim.plunkett’s picture

63 down, 11 to go.

tim.plunkett’s picture

FileSize
109.39 KB

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

tim.plunkett’s picture

FileSize
112.06 KB

Removed:
testModuleInvokeAllDuringLoadFunction()
testMenuLoadArgumentsInheritance()

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

tim.plunkett’s picture

I think I found the last couple fails.

dawehner’s picture

  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

dawehner’s picture

+++ 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.

tim.plunkett’s picture

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.

dawehner’s picture

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.

tim.plunkett’s picture

FileSize
11.58 KB
123.87 KB

Ugh, stupid fixes needed in menu.inc.

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

tim.plunkett’s picture

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

tim.plunkett’s picture

FileSize
998 bytes
132.59 KB
tim.plunkett’s picture

tim.plunkett’s picture

FileSize
16.61 KB
133.55 KB

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.

tim.plunkett’s picture

FileSize
27.73 KB

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

tim.plunkett’s picture

FileSize
1003 bytes
134.53 KB

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

dawehner’s picture

+++ 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.

tim.plunkett’s picture

Okay, switching them back.

ParisLiakos’s picture

  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

tim.plunkett’s picture

FileSize
141.23 KB

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.

ParisLiakos’s picture

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

tim.plunkett’s picture

FileSize
142.87 KB

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

tim.plunkett’s picture

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

29/30 uploads give me a 500. yay.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

a lot better indeed. Thanks!

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

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

Testbot fluke.

webchick’s picture

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

tim.plunkett’s picture

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

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

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