Modernize ForumTest: use entity queries instead of DB queries

How can a clean up task be a major?

Thanks so much, @voleger! And I'm sorry that this didn't receive the reviews it deserved :(

Great work here!

But, there's a problem with scope:

1. +++ b/core/modules/forum/tests/src/Functional/ForumTest.php
   @@ -369,15 +464,17 @@ public function editForumVocabulary() {
   -    $this->assertResponse(200);
   ...
   +    $this->assertSession()->statusCodeEquals(200);
   ...
   -    $this->assertEqual($current_vocabulary->label(), $edit['name'], 'The name was updated');
   -    $this->assertEqual($current_vocabulary->getDescription(), $edit['description'], 'The description was updated');
   +    $this->assertEquals($current_vocabulary->label(), $edit['name'], 'The name was updated');
   +    $this->assertEquals($current_vocabulary->getDescription(), $edit['description'], 'The description was updated');
   

   Please revert changes like these. They're out of scope.

2. +++ b/core/modules/forum/tests/src/Functional/ForumTest.php
   @@ -627,52 +778,60 @@ private function verifyForums(EntityInterface $node, $admin, $response = 200) {
   -      $forum_tid = db_query("SELECT tid FROM {forum} WHERE nid = :nid AND vid = :vid", [
   -        ':nid' => $node->id(),
   -        ':vid' => $node->getRevisionId(),
   -      ])->fetchField();
   -      $this->assertTrue($forum_tid == $this->rootForum['tid'], 'The forum topic is linked to a different forum');
   +      $forum_tid = $this->database->select('forum', 'f');
   +      $forum_tid->fields('f', ['tid']);
   +      $forum_tid->condition('nid', $node->id());
   +      $forum_tid->condition('vid', $node->getRevisionId());
   +      $forum_tid = $forum_tid->execute()->fetchField();
   

   And keep only changes like these.

   That's what the issue title describes, that is the scope of this issue!

That's SO much better! So much simpler :) I wanted to RTBC, but found one things I wasn't sure about yet, and one nit:

1. +++ b/core/modules/forum/tests/src/Functional/ForumTest.php
   @@ -396,7 +397,7 @@ public function editForumVocabulary() {
   -   * @return \Drupal\Core\Database\StatementInterface
   +   * @return array|\Drupal\Core\Database\StatementInterface
   
   @@ -427,18 +428,30 @@ public function createForum($type, $parent = 0) {
   -    return $term;
   +    /** @var \Drupal\taxonomy\Entity\Term $term */
   +    return [
   +      'tid' => $tid,
   +      'name' => $term->getName(),
   +    ];
   

   Why this change?

2. +++ b/core/modules/forum/tests/src/Functional/ForumTest.php
   @@ -633,10 +646,10 @@ private function verifyForums(EntityInterface $node, $admin, $response = 200) {
   +      $forum_tid = $this->container->get('database')->select('forum', 'frm');
   +      $forum_tid->condition('nid', $node->id());
   +      $forum_tid->condition('vid', $node->getRevisionId());
   +      $forum_tid = $forum_tid->execute()->fetchField();
   

   Nit: we can chain all of this:

   $forum_tid = $this->container->get(…)
     ->select(…)
     ->condition(…)
     ->condition(…)
     ->execute()
     ->fetchField();
   

I'm confused. #7 doesn't address #6. It re-introduces out-of-scope changes?

EDIT: I was hoping to RTBC #5 when #6 was addressed!

The #9 patch is for your composer.lock, that must've been an accident :)

Alright, #14's patch contains what #9 described, and is actually a reroll of #5 in response to my review in #6.

+++ b/core/modules/forum/tests/src/Functional/ForumTest.php
@@ -633,10 +645,13 @@ private function verifyForums(EntityInterface $node, $admin, $response = 200) {
-      $forum_tid = db_query("SELECT tid FROM {forum} WHERE nid = :nid AND vid = :vid", [
-        ':nid' => $node->id(),
-        ':vid' => $node->getRevisionId(),
-      ])->fetchField();
+      $forum_tid = $this->container
+        ->get('database')
+        ->select('forum')
+        ->condition('nid', $node->id())
+        ->condition('vid', $node->getRevisionId())
+        ->execute()
+        ->fetchField();

The issue title says "convert DB queries to entity queries", but that's not possible for this one.

Still, it's better to use the @database service than db_query(), which was deprecated in Drupal 8.0.x already.

+++ b/core/modules/forum/tests/src/Functional/ForumTest.php
@@ -266,7 +266,8 @@ public function testAddOrphanTopic() {
-    $nid_count = db_query('SELECT COUNT(nid) FROM {node}')->fetchField();
+    $nid_count = $this->container->get('entity_type.manager')
+      ->getStorage('node')->getQuery()->count()->execute();

We should add accessCheck(FALSE) to this query and the other ones. It doesn't make any difference to the test because there's no acces control on the queries, but every query that's not rendering should do this and it's a common mistake in non-test code.

#27: TIL!

+++ b/core/modules/forum/tests/src/Functional/ForumTest.php
@@ -427,18 +432,30 @@ public function createForum($type, $parent = 0) {
-    $parent_tid = db_query("SELECT t.parent_target_id FROM {taxonomy_term__parent} t WHERE t.entity_id = :tid", [':tid' => $tid])->fetchField();
+    $tid = $term->id();
+    $parent_tid = $taxonomy_term_storage->loadParents($tid);
+    $parent_tid = empty($parent_tid) ? 0 : array_shift($parent_tid)->id();

This is a problem as well - loadParents() doesn't do accessCheck(FALSE) or have an option to specify either way. Nor does getChildren() or getTree(). Have opened #2938848: TermStorage::loadParents()/getChildren()/getTree() need an option to specify accessCheck(FALSE) to discuss that further, it might be enough to just have the follow-up though, or should these use raw entity queries?

IMHO the follow-up is enough: part of the scope of #2938848 should be to look at all tests using this and update them to have accessCheck(FALSE). There's lots of tests that use this already. Besides, that issue will have to evaluate every existing call in core and considering updating them to use accessCheck(FALSE).

Using a raw entity query here just increases the maintenance burden.

I think & hope you'll agree with this assessment.

So … back to RTBC.

This applies cleanly to 8.6.x, so I've committed/pushed to that branch. It doesn't apply to 8.5.x, so marking this fixed against 8.6.x - nothing really stops us backporting to 8.5.x but it can also wait for the minor release IMO.