Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
44.02 KB

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.

tim.plunkett’s picture

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

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

tim.plunkett’s picture

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

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

tim.plunkett’s picture

tim.plunkett’s picture

tim.plunkett’s picture

More fixes.

tim.plunkett’s picture

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

tim.plunkett’s picture

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

disasm’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
98.31 KB

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.

disasm’s picture

forgot to fix path in search.routing.yml.

disasm’s picture

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

larowlan’s picture

FileSize
98.3 KB
576 bytes

Should fix search

Status: Needs review » Needs work

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

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
FileSize
1.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.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -FormInterface
FileSize
22.01 KB
120.25 KB

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...
dawehner’s picture

  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

tim.plunkett’s picture

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I assume this won't break the tests.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

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

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

The testbot had an apache segfault.
Requeueing.

webchick’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

Crosspost.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
120.6 KB
4.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.

larowlan’s picture

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.

webchick’s picture

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. :)

Gábor Hojtsy’s picture

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