Problem/Motivation

The test class in core/modules/node/tests/src/Functional/NodeRevisionPermissionsTest.php uses a regular database queries instead of using an entity queries.

Proposed resolution

Replace the regular database queries with an entity queries.

Remaining tasks

TBD

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

TBD

CommentFileSizeAuthor
#9 3151990-9.patch1.01 KBdaffie
#2 3151990-2.patch2.53 KBdaffie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Active » Needs review
FileSize
3151990-2.patch2.53 KB

Patch changes the db queries to entity queries.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
Status: Needs review » Reviewed & tested by the community

Verified this patch on local. Looks good to me.

pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
Issue tags: +Bug Smash Initiative
pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
Issue tags: -Bug Smash Initiative

  • catch committed b5cc26a on 9.1.x 
    Issue #3151990 by daffie, Hardik_Patel_12: Replace the database queries...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/node/tests/src/Functional/NodeRevisionPermissionsTest.php
@@ -107,10 +107,13 @@ public function testNodeRevisionAccessAnyType() {
     $node_revision_access = \Drupal::service('access_check.node.revision');
-    $connection = \Drupal::database();
+    $vids = \Drupal::entityQuery('node')
+      ->allRevisions()
+      ->condition('nid', $revision->id())
+      ->execute();

Is this wasn't a test I'd have said we should do $query->execute() in the condition, but given it is a test there's no reason to optimise.

Committed/pushed to 9.1.x, thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Fixed » Needs work

I missed this prior to commit, but both queries here need ->accessCheck(FALSE) since they're querying what's in the database as opposed to what can be shown to the current user.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs work » Needs review
FileSize
3151990-9.patch1.01 KB

Adding the requested ->accessCheck(FALSE) to both EntityQueries.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
Assigned: Unassigned » Hardik_Patel_12
Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
Assigned: Hardik_Patel_12 » Unassigned

Sorry, trying to assign other issue

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC , accessCheck is added to both queries as suggested by @catch.

  • catch committed d7334ba on 9.1.x 
    Issue #3151990 by daffie, Hardik_Patel_12, catch: Replace the database...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed d7334ba and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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