D7 recently got the ability to add query comments onto generated queries. Since Views generates queries, I was wondering if it'd be possible to add support for a configurable query comment into Views as well.

It would almost certainly help with query performance debugging on the sql server end.

Comments

dawehner’s picture

Version: 7.x-3.x-dev » 6.x-3.x-dev

I think this should be a setting on the query settings page(which is not ready yet)

In Drupal6 views generates the query, so it should be possible to develop the same feature there, too.

dawehner’s picture

Okay the query setting is ready so this can be developed.

Letharion’s picture

Assigned: Unassigned » dawehner

@dereine
So this goes to the task queue?

dawehner’s picture

Assigned: dawehner » Unassigned
Category: feature » task

Exactly.

cafuego’s picture

Assigned: Unassigned » cafuego

Okay, I am working on a patch.

cafuego’s picture

Status: Active » Needs review
StatusFileSize
new3.17 KB

Ok, you should probably wait for #1105848: Unsafe query comments possible via UI to be addressed before applying this :-)

Attached patch provided a textfield in the SQL Settings, allowing the user to enter a query comment. This is then prepended to the SQL statement, so for instance MySQL can write it to its slow_query_log if required.

dawehner’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev

So this is a patch again 7.x-3.x

cafuego’s picture

Oh sorry, yes :-)

dawehner’s picture


+    // Convert the query comment from display to query settings.
+    $query_comment = $this->get_option('query_comment');
+    if (!empty($query_comment)) {
+      $query_settings = $this->get_option('query');
+      $query_settings['options']['query_comment'] = $query_comment;
+      $this->set_option('query', $query_settings);
+      // Clear the values
+      $this->set_option('query_comment', NULL);
+    }

query_comment is not there in d7 nor d6. Why do we have to convert the options then?

cafuego’s picture

Is that not the bit of code that pulls the comment from the UI form and sticks it in the query object to be generated? I'm not particularly familiar with the views codebase, so I basically followed where DISTINCT is set and added query_comment just after that.

dawehner’s picture

Status: Needs review » Needs work

The destinct part was part of the main display configuration before, so the options had to be converted.

So needs work.

cafuego’s picture

Righto, I can just remove that section then - that makes life a tad easier. Thanks :-) I shall do a new patch shortly.

cafuego’s picture

Status: Needs work » Needs review
StatusFileSize
new2.4 KB

Re-rolled the patch with the superfluous code block removed.

dawehner’s picture

If you use this->options i don't get this part :)


   /**
+   * A query comment.
+   */
+  var $query_comment = '';
+
+  /**
    * Constructor; Create the basic query object and fill with default values.
    */
   function init($base_table = 'node', $base_field = 'nid', $options) {
@@ -139,6 +144,15 @@ class views_plugin_query_default extends views_plugin_query {
   }
 
   /**
+   * Set the query comment.
+   */
+  function set_query_comment($value = '') {
+    if (!empty($value)) {
+      $this->query_comment = $value;
+    }
+  }
dawehner’s picture

Status: Needs review » Needs work

So

cafuego’s picture

StatusFileSize
new1.68 KB

Oh I see. Again a matter of replicating the distinct option for the query_comment - which of course wasn't there before. Removed in the attached patch. So as far as I can tell I now have left:

  • Option definition
  • Option form field
  • Code that inserts the option if present

By my reckoning removing any more will break the functionality, so that should be it for the patch ;-)

cafuego’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Needs work

Please reupload your patch, i can't look at it.

cafuego’s picture

I'm off in the country with pretty crap internet (and no git repo) at the moment. Shall re-upload when I get back home.

cafuego’s picture

Status: Needs work » Needs review
StatusFileSize
new1.68 KB

Re-attached patch.

cafuego’s picture

Aight, core (D7 and D8) are patched as per #1105848: Unsafe query comments possible via UI

dawehner’s picture

Status: Needs review » Fixed

Patch worked fine.Thanks for all the reroles.

Commited to 7.x-3.x

Status: Fixed » Closed (fixed)

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