Files: 
CommentFileSizeAuthor
#28 1817672-28.patch17.07 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,161 pass(es).
[ View ]
#28 interdiff.txt962 bytesjibran
#26 1817672-26.patch17.03 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,758 pass(es).
[ View ]
#26 interdiff.txt10.69 KBjibran
#25 1817672-25.patch19.86 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,188 pass(es).
[ View ]
#25 interdiff.txt13.59 KBjibran
#23 interdiff.txt4.31 KBjibran
#23 1817672-23.patch14.61 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 55,858 pass(es).
[ View ]
#21 1817672-21.patch12.27 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 55,490 pass(es), 21 fail(s), and 54 exception(s).
[ View ]
#20 interdiff.txt11.09 KBjibran
#20 1817672-16.patch12.27 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 57,498 pass(es), 20 fail(s), and 54 exception(s).
[ View ]
#16 1817672-16.patch6.49 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,591 pass(es).
[ View ]
#13 db_query_instead_of_db_select-1817672-13.patch6.47 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 56,607 pass(es).
[ View ]
#13 interdiff.txt3.03 KBbdgreen
#10 db_query_instead_of_db_select-1817672-10.patch7.87 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 55,759 pass(es).
[ View ]
#10 interdiff.txt4.25 KBbdgreen
#8 drupal-1817672-8.patch6.48 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 51,360 pass(es).
[ View ]
#5 drupal-1817672-5.patch9.19 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1817672-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 1817672.patch2.83 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 3,149 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.83 KB
PASSED: [[SimpleTest]]: [MySQL] 3,149 pass(es).
[ View ]

+++ 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?

Title:Use db_query instead of db_selectUse db_query() instead of db_select() where appropriate
Project:Views» 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).

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

Status:Needs work» Needs review
StatusFileSize
new9.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1817672-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new6.48 KB
PASSED: [[SimpleTest]]: [MySQL] 51,360 pass(es).
[ View ]

Rerolled now.

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.

StatusFileSize
new4.25 KB
new7.87 KB
PASSED: [[SimpleTest]]: [MySQL] 55,759 pass(es).
[ View ]

Issues corrected ...

Status:Needs work» Needs review

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.

Status:Needs work» Needs review
StatusFileSize
new3.03 KB
new6.47 KB
PASSED: [[SimpleTest]]: [MySQL] 56,607 pass(es).
[ View ]

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

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!

Status:Reviewed & tested by the community» Needs work

Needs reroll

Status:Needs work» Needs review
StatusFileSize
new6.49 KB
PASSED: [[SimpleTest]]: [MySQL] 57,591 pass(es).
[ View ]

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'];

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

@jibran

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

StatusFileSize
new12.27 KB
FAILED: [[SimpleTest]]: [MySQL] 57,498 pass(es), 20 fail(s), and 54 exception(s).
[ View ]
new11.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().

StatusFileSize
new12.27 KB
FAILED: [[SimpleTest]]: [MySQL] 55,490 pass(es), 21 fail(s), and 54 exception(s).
[ View ]

Same patch for testing.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new14.61 KB
PASSED: [[SimpleTest]]: [MySQL] 55,858 pass(es).
[ View ]
new4.31 KB

Forgot some a lot off use statements.

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.

StatusFileSize
new13.59 KB
new19.86 KB
PASSED: [[SimpleTest]]: [MySQL] 57,188 pass(es).
[ View ]

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

StatusFileSize
new10.69 KB
new17.03 KB
PASSED: [[SimpleTest]]: [MySQL] 57,758 pass(es).
[ View ]

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

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.

StatusFileSize
new962 bytes
new17.07 KB
PASSED: [[SimpleTest]]: [MySQL] 57,161 pass(es).
[ View ]

Done.

Status:Needs review» Reviewed & tested by the community

Nice! One less db query needed in the code.

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

Looks good. thanks jibran!

Changing the title to move with the times.

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.