Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Active » Needs review
FileSize
2.83 KB
dawehner’s picture

+++ b/lib/Views/aggregator/Plugin/views/argument/Fid.phpundefined
@@ -29,10 +29,6 @@ class Fid extends Numeric {
-    $query = db_select('aggregator_feed', 'f');
-    $query->addField('f', 'title');
-    $query->condition('f.fid', $this->value);
-    $result = $query->execute();

Oh this is embarrassing.

+++ b/lib/Views/comment/Plugin/views/field/NodeNewComments.phpundefined
@@ -78,15 +78,8 @@ class NodeNewComments extends Numeric {
+      $query  = 'SELECT n.nid, COUNT(c.cid) as num_comments from {node} n INNER JOIN {comment} c ON n.nid = c.nid LEFT JOIN {history} h ON h.nid = n.nid WHERE n.nid IN :nids AND (c.changed > GREATEST(COALESCE(h.timestamp, :timestamp) AND c.status = :status GROUP BY n.nid';
+      $result = db_query($query, array(':nids' => $nids, ':timestamp' => NODE_NEW_LIMIT, ':status' => COMMENT_PUBLISHED));

It's funny as we replaced them earlier with dbtng code and there are probably other places #1748168: Convert or document all usages of db_query()
Could it be that we should actually basically revert that patch?

xjm’s picture

Title: Use db_query instead of db_select » Use db_query() instead of db_select() where appropriate
Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
Status: Needs review » Needs work

Closed #1817608: replace the query in NodeNewComments with a static one as a duplicate of this issue. See the comments from DamZ and chx there.

We'll want to split the aggregator change off and move it into the patch to add that integration (once it's posted).

Damien Tournoud’s picture

(copied from the other issue)

This query (both versions of it) doesn't seem to make any sense. At the very very minimum, it is missing a constraint on h.uid.

I don't think there is a way to do this query properly (we want to count the number of comments posted to a list of nodes after the time the user last seen each node), other then with an ugly correlated scalar subquery:

SELECT nid, (SELECT COUNT(*) FROM comment c WHERE c.changed > GREATEST(COALESCE(h.timestamp, :timestamp), :timestamp)) count FROM node n LEFT JOIN history h ON h.nid = n.nid AND h.uid = :uid

So I just suggest we do like the forum module does in forum_get_topics().

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.19 KB

This query (both versions of it) doesn't seem to make any sense. At the very very minimum, it is missing a constraint on h.uid.

Oh right, the new version (dbtng) is missing the constraint, the old
one had it.

This patch basically reverts the changes done in #1748168: Convert or document all usages of db_query() for every
place where it has been helpful.

dawehner’s picture

Issue tags: -VDC

#5: drupal-1817672-5.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, drupal-1817672-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.48 KB

Rerolled now.

dcam’s picture

Status: Needs review » Needs work
Issue tags: +Novice

I found a few issues, two of them just code style problems. They seem like good things for a Novice to fix.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeNewComments.phpundefined
@@ -79,16 +79,15 @@ function pre_render(&$values) {
+      $result = db_query("SELECT n.nid, COUNT(c.cid) as num_comments FROM {node} n INNER JOIN {comment} c ON n.nid = c.nid
+        LEFT JOIN {history} h ON h.nid = n.nid AND h.uid = :h_uid WHERE n.nid IN (:nids)
+        AND c.changed > GREATEST(COALESCE(h.timestamp, :timestamp), :timestamp) AND c.status = :status GROUP BY n.nid  ", array(
+          ':status' => COMMENT_PUBLISHED,
+          ':h_uid' => $user->uid,
+          ':nids' => $nids,
+          ':timestamp' => HISTORY_READ_LIMIT,
+        ));

There's a double space at the end of the query argument, right after "GROUP BY n.nid".

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Permissions.phpundefined
@@ -58,14 +58,8 @@ function pre_render(&$values) {
+      $result = db_query("SELECT u.uid, u.rid, rp.permission FROM {role_permission} rp INNER JOIN {users_roles} u ON u.rid = rp.rid WHERE u.uid IN (:uids) AND rp.module IN (:modules) ORDER BY rp.permission",
+        -        array(':uids' => $uids, ':modules' => array_keys($modules)));

It looks like this one was copied and pasted from another patch, probably the issue mentioned in #2 that converted queries from db_query to db_select. After the second line was pasted, however, the "- " wasn't removed.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Roles.phpundefined
@@ -48,12 +48,7 @@ function pre_render(&$values) {
+      $result = db_query("SELECT u.uid, u.rid FROM {users_roles} u WHERE u.uid IN (:uids) AND u.rid in (:rids)", array(':uids' => $uids, ':rids' => array_keys($roles)));

The second "in" isn't uppercase.

bdgreen’s picture

Issues corrected ...

bdgreen’s picture

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Permissions.phpundefined
@@ -55,14 +55,8 @@ function pre_render(&$values) {
-      $query = db_select('role_permission', 'rp');
-      $query->join('users_roles', 'u', 'u.rid = rp.rid');
-      $query->fields('u', array('uid', 'rid'));
-      $query->addField('rp', 'permission');
-      $query->condition('u.uid', $uids);
-      $query->condition('rp.module', array_keys($modules));
-      $query->orderBy('rp.permission');
-      $result = $query->execute();
+      $result = db_query("SELECT u.uid, u.rid, rp.permission FROM {role_permission} rp INNER JOIN {users_roles} u ON u.rid = rp.rid WHERE u.uid IN (:uids) AND rp.module IN (:modules) ORDER BY rp.permission",
+                array(':uids' => $uids, ':modules' => array_keys($modules)));

I'm sorry, I wasn't very clear. The extra spaces in front of the array argument need to be deleted too, not just the '-'. It should be indented two spaces from the start of the previous line.

#10 also adds several extra code style changes. Another more experienced contributor might want to comment on it, but I think we generally try to keep patches on topic and not add extra items, even if they seem necessary. The reasons why are that it can make patches harder to review by confusing the issue and if they did end up being committed it increases the chances that other patches will need to be rerolled. I would open up another issue to address them.

bdgreen’s picture

Status: Needs work » Needs review
FileSize
3.03 KB
6.47 KB

@dcam acknowledged ;) Rerolled from #8 and added changes (only) from #9, updated with #12 -- hope that's OK?

dcam’s picture

Status: Needs review » Reviewed & tested by the community

#13 looks good to me! It reverts several queries that were originally changed to db_select() in #1748168: Convert or document all usages of db_query(). Thanks, bdgreen!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs reroll

jibran’s picture

Status: Needs work » Needs review
FileSize
6.49 KB

Conflict

++<<<<<<< HEAD
 +    $results = db_select('node_field_revision', 'npr')
 +      ->fields('npr', array('vid', 'nid', 'title'))
 +      ->condition('npr.vid', $this->value)
 +      ->execute()
 +      ->fetchAllAssoc('vid', PDO::FETCH_ASSOC);
++=======
+     $results = db_query("SELECT nr.vid, nr.nid, nr.title FROM {node_revision} nr WHERE nr.vid IN (:vids)", array(':vids' => $this->value))->fetchAllAssoc('vid', PDO::FETCH_ASSOC);
++>>>>>>> 13

Fixed

--- a/core/modules/node/lib/Drupal/node/Plugin/views/argument/Vid.php
+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument/Vid.php
@@@ -25,11 -25,7 +25,7 @@@ class Vid extends Numeric
    function title_query() {
      $titles = array();

-     $results = db_select('node_field_revision', 'npr')
-       ->fields('npr', array('vid', 'nid', 'title'))
-       ->condition('npr.vid', $this->value)
-       ->execute()
-       ->fetchAllAssoc('vid', PDO::FETCH_ASSOC);
 -    $results = db_query("SELECT nr.vid, nr.nid, nr.title FROM {node_revision} nr WHERE nr.vid IN (:vids)", array(':vids' => $this->value))->fetchAllAssoc('vid', PDO::FETCH_ASSOC);
++    $results = db_query("SELECT npr.vid, npr.nid, npr.title FROM {node_field_revision} npr WHERE npr.vid IN (:vids)", array(':vids' => $this->value)      $nids = array();
      foreach ($results as $result) {
        $nids[] = $result['nid'];
dawehner’s picture

In a perfect world all this classes would get the database connection injected via the create() method.

jibran’s picture

dawehner’s picture

@jibran

This is already in, so why do we have to wait?

jibran’s picture

FileSize
12.27 KB
11.09 KB

Trying to make a perfect world.

  • We should create a meta for all views plugins.
  • There is no service to remove db_or().
jibran’s picture

FileSize
12.27 KB

Same patch for testing.

Status: Needs review » Needs work

The last submitted patch, 1817672-21.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
14.61 KB
4.31 KB

Forgot some a lot off use statements.

dawehner’s picture

That's nearly ready to fly now!

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/argument/UserUid.phpundefined
@@ -20,15 +22,35 @@
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Connection $database) {

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeNewComments.phpundefined
@@ -22,6 +24,29 @@
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Connection $database) {

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument/Vid.phpundefined
@@ -17,7 +19,28 @@
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Connection $database) {

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/argument_validator/User.phpundefined
@@ -26,6 +28,29 @@
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Connection $database) {

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Permissions.phpundefined
@@ -22,6 +24,29 @@
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Connection $database) {

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Roles.phpundefined
@@ -22,6 +24,29 @@
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Connection $database) {

Let's document the new parameter as well.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeNewComments.phpundefined
@@ -76,16 +101,15 @@ function pre_render(&$values) {
+      $result = $this->database->query("SELECT n.nid, COUNT(c.cid) as num_comments FROM {node} n INNER JOIN {comment} c ON n.nid = c.nid
+        LEFT JOIN {history} h ON h.nid = n.nid AND h.uid = :h_uid WHERE n.nid IN (:nids)
+        AND c.changed > GREATEST(COALESCE(h.timestamp, :timestamp), :timestamp) AND c.status = :status GROUP BY n.nid", array(

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument/Vid.phpundefined
@@ -25,11 +48,7 @@ class Vid extends Numeric {
+    $results = $this->database->query("SELECT npr.vid, npr.nid, npr.title FROM {node_field_revision} npr WHERE npr.vid IN (:vids)", array(':vids' => $this->value))->fetchAllAssoc('vid', PDO::FETCH_ASSOC);

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Permissions.phpundefined
@@ -55,14 +80,8 @@ function pre_render(&$values) {
+      $result = $this->database->query("SELECT u.uid, u.rid, rp.permission FROM {role_permission} rp INNER JOIN {users_roles} u ON u.rid = rp.rid WHERE u.uid IN (:uids) AND rp.module IN (:modules) ORDER BY rp.permission",

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Roles.phpundefined
@@ -45,12 +70,7 @@ function pre_render(&$values) {
+      $result = $this->database->query("SELECT u.uid, u.rid FROM {users_roles} u WHERE u.uid IN (:uids) AND u.rid IN (:rids)", array(':uids' => $uids, ':rids' => array_keys($roles)));

Let's remove the double quotes.

jibran’s picture

FileSize
13.59 KB
19.86 KB

Thanks @dawehner for the review. Fixed #24. Added doxygen for create method as well.

jibran’s picture

FileSize
10.69 KB
17.03 KB

Removed doxygen for create beacuse of \Drupal\Core\Plugin\ContainerFactoryPluginInterface.

dawehner’s picture

Awesome!!

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/argument_validator/User.phpundefined
@@ -120,10 +154,7 @@ function validate_argument($argument) {
-      $query = db_select('users_roles', 'u');
-      $query->addField('u', 'rid');
-      $query->condition('u.uid', $account->uid);
-      $result = $query->execute();
+      $result = $this->database->query('SELECT rid FROM {users_roles} WHERE uid = :uid', array(':uid' => $account->uid));
       foreach ($result as $role) {
         $account->roles[] = $role->rid;

We seriously should do foreach ($account->getRoles() as $rid) instead.

jibran’s picture

FileSize
962 bytes
17.07 KB

Done.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice! One less db query needed in the code.

damiankloip’s picture

Title: Use db_query() instead of db_select() where appropriate » replace db_select() with an injected database connection in views plugins

Looks good. thanks jibran!

Changing the title to move with the times.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed db0104f and pushed to 8.x. Thanks!

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