Files: 
CommentFileSizeAuthor
#36 interdiff.txt4.08 KBtim.plunkett
#36 non-test-non-form-2089635-36.patch120.6 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,088 pass(es).
[ View ]
#28 non-test-non-form-2089635-28.patch120.41 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,164 pass(es).
[ View ]
#28 interdiff.txt3.88 KBtim.plunkett
#26 non-test-non-form-2089635-26.patch120.25 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,792 pass(es).
[ View ]
#26 interdiff.txt22.01 KBtim.plunkett
#25 interdiff.txt1.71 KBtim.plunkett
#23 interdiff.txt576 byteslarowlan
#23 page-callbacks.patch98.3 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 59,154 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#20 drupal8.nontest-noforms.2089635-20.patch98.3 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 59,203 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#20 interdiff.txt485 bytesdisasm
#19 drupal8.nontest-noforms.2089635-18.patch98.31 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#19 interdiff.txt1.92 KBdisasm
#15 non-test-non-form-2089635-15.patch98.25 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,790 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#15 interdiff.txt22.34 KBtim.plunkett
#13 interdiff.txt707 bytestim.plunkett
#13 non-test-non-form-2089635-13.patch90.47 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,150 pass(es), 2,201 fail(s), and 1,174 exception(s).
[ View ]
#11 non-test-non-form-2089635-11.patch90.54 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,763 pass(es), 373 fail(s), and 35 exception(s).
[ View ]
#11 interdiff.txt15.51 KBtim.plunkett
#9 non-test-non-form-2089635-9.patch77.06 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,778 pass(es), 90 fail(s), and 72 exception(s).
[ View ]
#9 interdiff.txt19.58 KBtim.plunkett
#7 interdiff.txt24.47 KBtim.plunkett
#7 non-test-non-form-2089635-7.patch65.15 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,655 pass(es), 154 fail(s), and 4 exception(s).
[ View ]
#5 non-test-non-form-2089635-5.patch48.78 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,800 pass(es), 36 fail(s), and 12 exception(s).
[ View ]
#5 interdiff.txt3.41 KBtim.plunkett
#3 interdiff.txt5.54 KBtim.plunkett
#3 non-test-non-form-2089635-3.patch45.37 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch non-test-non-form-2089635-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 non-test-non-form-2089635-1.patch44.02 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,426 pass(es), 498 fail(s), and 170 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new44.02 KB
FAILED: [[SimpleTest]]: [MySQL] 58,426 pass(es), 498 fail(s), and 170 exception(s).
[ View ]

Got about halfway done, going out to dinner. I know I broke at least one thing...

Anything with an access check or logic in the hook_menu is still non-trivial.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new45.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch non-test-non-form-2089635-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.54 KB

Okay, I'm back. Here are some attempted text fixes for the last run, while I work on the rest.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.41 KB
new48.78 KB
FAILED: [[SimpleTest]]: [MySQL] 58,800 pass(es), 36 fail(s), and 12 exception(s).
[ View ]

LOL I did not mean to RTBC.
Here's one I did since then.

StatusFileSize
new65.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,655 pass(es), 154 fail(s), and 4 exception(s).
[ View ]
new24.47 KB

StatusFileSize
new19.58 KB
new77.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,778 pass(es), 90 fail(s), and 72 exception(s).
[ View ]

StatusFileSize
new15.51 KB
new90.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,763 pass(es), 373 fail(s), and 35 exception(s).
[ View ]

More fixes.

StatusFileSize
new90.47 KB
FAILED: [[SimpleTest]]: [MySQL] 56,150 pass(es), 2,201 fail(s), and 1,174 exception(s).
[ View ]
new707 bytes

Don't refactor after running tests without rerunning the tests.

StatusFileSize
new22.34 KB
new98.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,790 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

Fix menu_get_object, add @deprecated, and remove accidental changes to symfony

Assigned:tim.plunkett» Unassigned
Status:Needs review» Needs work

Three of the remaining failures are related to search queries with / in them.
For example,

Drupal\search\Tests\SearchNumberMatchingTest->testNumberSearching()
0: node title shown (search found the node) in search for number 12/34/56789

I'm not sure how to best resolve that.

Noooo idea about Drupal\translation\Tests\TranslationTest.

If anyone wants to pitch in, feel free.

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
new98.31 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

This should fix search. Need to specify that the param can contain odd characters in requirements: http://symfony.com/doc/current/cookbook/routing/slash_in_parameter.html

Also, default search was taking plugin_id as param, it should be taking just keys and passing NULL as plugin_id.

Content translations a bit trickier, because none of the callbacks are in that module.

StatusFileSize
new485 bytes
new98.3 KB
FAILED: [[SimpleTest]]: [MySQL] 59,203 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

forgot to fix path in search.routing.yml.

per #9 tackling content_translation in a separate issue: #2090717: Convert content_translation routes to interim wrapper controller..

StatusFileSize
new98.3 KB
FAILED: [[SimpleTest]]: [MySQL] 59,154 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new576 bytes

Should fix search

Status:Needs review» Needs work

The last submitted patch, page-callbacks.patch, failed testing.

Assigned:Unassigned» tim.plunkett
StatusFileSize
new1.71 KB

Thanks so much for that, @disasm and @larowlan!
Here's the aggregated interdiff of your changes, in case anyone is curious.

I think I figured out the last fail, will have a patch soon.

Status:Needs work» Needs review
Issue tags:-FormInterface
StatusFileSize
new22.01 KB
new120.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,792 pass(es).
[ View ]

Since @disasm got #2090717: Convert content_translation routes to interim wrapper controller. working already, merging that in.
This also now includes translation_node_overview.

So, the leftovers are:

  • user_admin_account and node_admin_nodes (blocked by the views issue but otherwise trivial)
  • tracker_page, which is tricky because of tracker/%user_uid_optional, and because people wanted it to be a view...

  1. +++ b/core/modules/aggregator/aggregator.module
    @@ -142,13 +142,6 @@ function aggregator_menu() {
    -    'title' => 'OPML feed',
    +++ b/core/modules/aggregator/aggregator.routing.yml
    @@ -137,3 +137,11 @@ aggregator.categorize_feed_form:
    +aggregator.opml_page:
    +  path: '/aggregator/opml/{cid}'
    +  defaults:

    Let's ensure to add the title.

  2. +++ b/core/modules/aggregator/aggregator.module
    @@ -142,13 +142,6 @@ function aggregator_menu() {
    -    'type' => MENU_CALLBACK,
    +++ b/core/modules/aggregator/aggregator.routing.yml
    @@ -137,3 +137,11 @@ aggregator.categorize_feed_form:
    +    _content: '\Drupal\aggregator\Controller\AggregatorController::opmlPage'

    This one does not care about a drupal html page, so it should use _controller.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentInterfaceTest.php
    @@ -53,7 +53,7 @@ function testCommentInterface() {
    -    $this->drupalGet('node/' . $this->node->id() . '/' . $comment->id());
    +    $this->drupalGet('node/' . $this->node->id());
    +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.php
    @@ -68,7 +68,7 @@ function testThreadedCommentView() {
    -    $this->drupalGet('node/' . $this->node->id() . '/' . $comment->id());
    +    $this->drupalGet('node/' . $this->node->id());

    Just wondering whether the comment ID maybe cared about the pager before?

  4. +++ b/core/modules/contact/contact.module
    @@ -80,31 +80,22 @@ function contact_menu() {
       $items['contact'] = array(
         'title' => 'Contact',
    ...
         'title' => 'Contact category form',
    ...
         'title' => 'Contact',

    This should be also copied to the routing file.

  5. +++ b/core/modules/dblog/lib/Drupal/dblog/Routing/RouteSubscriber.php
    @@ -0,0 +1,73 @@
    +class RouteSubscriber implements EventSubscriberInterface {
    ...
    +  public function routes(RouteBuildEvent $event) {
    +    $collection = $event->getRouteCollection();
    +    if ($this->moduleHandler->moduleExists('search')) {

    Wasn't the approach in the issue way simpler?

  6. +++ b/core/modules/node/node.routing.yml
    @@ -12,6 +12,28 @@ node.page_edit:
    +node.add_page:
    +  path: '/node/add'
    ...
    +
    +node.add:
    +  path: '/node/add/{node_type}'
    +  defaults:

    At least node/add could have a title.

  7. +++ b/core/modules/node/node.routing.yml
    @@ -12,6 +12,28 @@ node.page_edit:
    +    _controller: '\Drupal\node\Controller\NodeController::addPage'
    ...
    +    _controller: '\Drupal\node\Controller\NodeController::add'

    I can't see a reason why this should be not _content.

  8. +++ b/core/modules/node/node.routing.yml
    @@ -19,6 +41,22 @@ node.delete_confirm:
    +    _controller: '\Drupal\node\Controller\NodeController::revisionOverview'
    +  requirements:
    +    _access_node_revision: 'view'
    +
    ...
    +    _controller: '\Drupal\node\Controller\NodeController::revisionShow'

    _content

StatusFileSize
new3.88 KB
new120.41 KB
PASSED: [[SimpleTest]]: [MySQL] 59,164 pass(es).
[ View ]

The issue for dblog_top('search') (admin/reports/search) is way easier because it completely bypasses the module_exists :) It just always provides the route, which is wrong.

-   $this->drupalGet('node/' . $this->node->id() . '/' . $comment->id()); did absolutely nothing anyway.

I addressed all of the other issues.

Status:Needs review» Reviewed & tested by the community

I assume this won't break the tests.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, non-test-non-form-2089635-28.patch, failed testing.

Status:Needs work» Reviewed & tested by the community

The testbot had an apache segfault.
Requeueing.

Status:Reviewed & tested by the community» Needs work

A-W-E-S-O-M-E! :D

I tried really hard to get through this whole patch but ran smack into babysitting duty, so only was able to give a cursory overview to the last, say, 2/3. Here's what I found so far though.

+++ b/core/includes/menu.inc
@@ -1027,9 +1028,23 @@ function menu_item_route_access(Route $route, $href, &$map) {
+
+  // Attempt to inspect the route object.
+  $request = \Drupal::request();
+  if ($request->attributes->has(RouteObjectInterface::ROUTE_OBJECT)) {
+    $path = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)->getPath();
+    $parts = explode('/', ltrim($path, '/'));
+    if (isset($parts[$position]) && $parts[$position] == '{' . $type . '}') {
+      return $router_item['map'][$position];
+    }

Yeah, wow. No idea what's happening here but that seems crazy. We need to document this, along with a @todo that tells us how to clean this up after.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -114,6 +114,8 @@ function template_preprocess_aggregator_item(&$variables) {
+ * @deprecated Use \Drupal\aggregator\Controller\AggregatorController::opmlPage()
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
@@ -346,4 +346,12 @@ public function sources() {
+   * @todo Remove aggregator_opml().

Cross-referenced @deprecated and @todo! That's great!

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockAdminController.php
@@ -0,0 +1,26 @@
+use Symfony\Component\HttpFoundation\Request;

Why does this "use" Request? It's not referenced below.

+++ b/core/modules/block/lib/Drupal/block/Controller/BlockAdminController.php
@@ -0,0 +1,26 @@
+class BlockAdminController extends ControllerBase {

That sounds a bit too generic, like it would contain methods for all block admin-related thingies. Better as BlockDemoController or so?

In IRC, corrected: "BlockController."

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentInterfaceTest.php
@@ -53,7 +53,7 @@ function testCommentInterface() {
     // Check comment display.
-    $this->drupalGet('node/' . $this->node->id() . '/' . $comment->id());
+    $this->drupalGet('node/' . $this->node->id());
     $this->assertText($subject_text, 'Individual comment subject found.');
     $this->assertText($comment_text, 'Individual comment body found.');
+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.php
@@ -68,7 +68,7 @@ function testThreadedCommentView() {
     // Check comment display.
-    $this->drupalGet('node/' . $this->node->id() . '/' . $comment->id());
+    $this->drupalGet('node/' . $this->node->id());
     $this->assertText($comment_subject, 'Individual comment subject found.');
     $this->assertText($comment_text, 'Individual comment body found.');

Hm. That seems to have changed the nature of that test? Before it was testing for the comment permalink/canonical URL/whatever worked, not just that the comment was on the node page.

Berdir found #296486: TestingParty08: viewing an individual comment where the tests were added which confirms this. So either we should put this capability back (if we can track down why it was done and the reason remains valid) or we should remove these tests entirely.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactPageController.php
@@ -0,0 +1,34 @@
+class ContactPageController {

This one is "ContactPageController" and does not extend ControllerBase.

We should probably convert these all in a consistent way.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
@@ -0,0 +1,44 @@
+class ContentTranslationController {

and here is ModuleController, no base class once again.

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.php
@@ -319,4 +319,28 @@ protected function buildFilterQuery() {
+  /**
+   * @todo Remove dblog_top().
+   */
+  public function pageNotFound() {
...
+  /**
+   * @todo Remove dblog_top().
+   */
+  public function accessDenied() {
...
+  /**
+   * @todo Remove dblog_top().
+   */
+  public function search() {

The @todos need to be updated here.

Status:Needs work» Reviewed & tested by the community

That drupalGet() does nothing because that functionality has been removed in #350984: Kick comment rendering out of node module, the tests for it were added only three months before that in #296486: TestingParty08: viewing an individual comment and after that was removed, simply pointed to the normal node page and happily testing the output there.

Status:Reviewed & tested by the community» Needs work

Crosspost.

Status:Needs work» Needs review
StatusFileSize
new120.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,088 pass(es).
[ View ]
new4.08 KB

Those three dblog methods all call through to dblog_top(), that's not a copy/paste mistake.

Yes, @Berdir's code anthropology is correct, but I don't think we should remove the coverage. It's still good to assert that the comments are displayed below the node when they're supposed to be.

I resolved the other points @webchick made.

EDIT: The interdiff didn't include the changes to BlockAdminController, which was renamed to BlockController, and the stray use Symfony\Component\HttpFoundation\Request; was removed.

Status:Needs review» Reviewed & tested by the community

Issues addressed from #33
There's a few that don't use ControllerBase (because they don't need to yet).
Happy to keep the test for the comment rendering on node/% even though comment-permalinks are of a different format now.

Status:Reviewed & tested by the community» Fixed

Awesome. Can't find any other complaints, and this is a HUGE step forward in removing hook_menu() and friends. Let's go!

Committed and pushed to 8.x. YEAHH!

1 down, 3 to go. :)

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