DBTNG Example needs more examples

A condition based on a subquery

A start ;)

I probably won't do many more but needed to learn to do this and found it missing in example.module

Setting version.

I'd actually love to see simpler examples to copy and paste. :-)

Mentioning this one #1028168: Create a page to show results from database including a pager. in case someone gets excited. :-)

I have patched filter form for the listing page; a new easy to learn stuff in this module.

Kindly review.
Thanks!

Also review the new patch containing "Ordered List" example.

Thanks!

1. +++ b/dbtng_example/dbtng_example.module
   @@ -1,4 +1,5 @@
   +
   

   Remove this linebreak

2. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +585,43 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +/*
   

   Doxygen should be /**

+++ b/dbtng_example/dbtng_example.module
@@ -574,6 +585,43 @@ function dbtng_example_form_update_submit($form, &$form_state) {
+    // Make a table for them.
+    $header = array(t('Id'), t('uid'), t('Name'), t('Surname'), t('Age'));
+    $output .= theme('table', array('header' => $header, 'rows' => $rows));

This is not an ordered list, but a table.

+  $select = db_select('dbtng_example', 'ex');
+  $select->fields('ex', array('pid', 'uid', 'name', 'surname', 'age'));
+  // 'ex.name' is the field on which to order.
+  // Legal values for sorting are "ASC" and "DESC". Any other value will be converted to "ASC".
+  $select->orderBy('ex.name', 'ASC');
+  // generate the result in object format.
+  $result = $select->execute()->fetchAll();

This query will create an ordered list.

corrected the patch as mentioned in #16 and attached the same.

Kindly review.

Thanks!

I think the example would be better if it is called "Ordering results" or something similar as that is what the example is about.

1. +++ b/dbtng_example/dbtng_example.module
   @@ -344,6 +344,10 @@ function dbtng_example_help($path) {
   +      $output = t('Generate an ordered list of all the records in the database. ');
   +      $output .= t('The records will be displayed in ascending order for column "Name".');
   

   Extraneous whitespace after database..

   It seems to me that this string should be on one line in one t() call.

2. +++ b/dbtng_example/dbtng_example.module
   @@ -388,6 +392,12 @@ function dbtng_example_menu() {
   +    'access callback' => TRUE,
   

   I know the internet is littered with examples using TRUE as an access callback. Using TRUE as an access callback is however dangerous in production code and has been known to cause security issues. It should therefore be prevented in any code you write to ensure you never accidentally leave access to menu items unchecked. Try to always use a genuine access callback (even if it's just checking if a user has access to the admin pages or permission to view content for example).

3. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +584,43 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   + * This function queries the database and retrives all the records in an ascending order.
   

   This comment exceeds 80 characters.

4. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +584,43 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   + * SELECT ex.pid AS pid, ex.uid AS uid, ex.name AS name, ex.surname AS surname, ex.age AS age
   

   This comment exceeds 80 characters.

5. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +584,43 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  $output = '';
   

   This should be declared closer to where it is used. Besides that, this menu callback should actually return a render array instead of a string.

6. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +584,43 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  $select = db_select('dbtng_example', 'ex');
   +  $select->fields('ex', array('pid', 'uid', 'name', 'surname', 'age'));
   +  // 'ex.name' is the field on which to order.
   +  // Legal values for sorting are "ASC" and "DESC". Any other value will be converted to "ASC".
   +  $select->orderBy('ex.name', 'ASC');
   +  // generate the result in object format.
   +  $result = $select->execute()->fetchAll();
   

   If you extract this section into it's own method named something like dbtng_example_execute_ordered_select_query, then the dev reading the example would immediately know where to look for the actual Database API example.

7. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +584,43 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  $select->fields('ex', array('pid', 'uid', 'name', 'surname', 'age'));
   

   I'm not sure about the schema of the dbtng_example table, but it seems to me like you are selecting all fields in the table.

   If that is the intention, you can just do the following:

     $select->fields('ex');
   

   Or even better

     $select = db_select('dbtng_example', 'ex')->fields('ex');
   

   That way you can direct the reader's focus on the $select->orderBy('ex.name', 'ASC') call.

8. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +584,43 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  // Legal values for sorting are "ASC" and "DESC". Any other value will be converted to "ASC".
   

   This comment exceeds 80 characters.

9. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +584,43 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  if ($result) {
   +    $rows = array();
   +    foreach ($result as $row) {
   +      // Sanitize the data before handing it off to the theme layer.
   +      $rows[] = array_map('check_plain', (array) $row);
   +    }
   +    // Make a table for them.
   +    $header = array(t('Id'), t('uid'), t('Name'), t('Surname'), t('Age'));
   +    $output .= theme('table', array('header' => $header, 'rows' => $rows));
   +  }
   +  else {
   +    drupal_set_message(t('No records found.'));
   +  }
   +  return $output;
   

   You could also extract this into a function dbtng_example_render_resultset_as_table to make sure that the reader does not have to filter out information that is not relevant to the database API.

@legolasbo: The mentioned modifications have been made in the new patch and also attached the interdiff.

Kindly review.
Thanks!

1. +++ b/dbtng_example/dbtng_example.module
   @@ -346,8 +346,7 @@
   +      $output = t('Generate a list of database records in Ordering format. The records will be displayed in ascending order for column "Name".');
   

   'Ordering' should be 'ordering'

2. +++ b/dbtng_example/dbtng_example.module
   @@ -395,7 +394,7 @@
   +    'access arguments' => array('access dbtng example ordered list'),
   

   This is too specific. 'access content' or 'access administration pages' will do.

3. +++ b/dbtng_example/dbtng_example.module
   @@ -403,6 +402,22 @@
   + * Implements hook_permission().
   + *
   + * This hook can supply permissions that the module defines, so that they can
   + * be selected on the user permissions page and used to grant or restrict
   + * access to actions the module performs.
   + */
   +function dbtng_example_permission() {
   +  return array(
   +    'access dbtng example ordered list' => array(
   +      'title' => t('Permission to view an Ordered List'),
   +      'description' => t('Give permission to access an Ordered List.'),
   +    ),
   +  );
   +}
   +
   +/**
   

   This can be removed if you use one of the permissions mentioned above

4. +++ b/dbtng_example/dbtng_example.module
   @@ -588,39 +603,46 @@
   + * This function queries the database and retrives all the records in an
   + * ascending order.
   

   /s/retrives/retrieves
   /s/in an ascending/in ascending

5. +++ b/dbtng_example/dbtng_example.module
   @@ -588,39 +603,46 @@
   + * SELECT ex.pid AS pid, ex.uid AS uid, ex.name AS name, ex.surname AS surname,
   

   Maybe explain that the code below will result in the following query

6. +++ b/dbtng_example/dbtng_example.module
   @@ -588,39 +603,46 @@
     *
   

   Excess blank comment line

7. +++ b/dbtng_example/dbtng_example.module
   @@ -588,39 +603,46 @@
   -  $output = '';
   -
   -  $select = db_select('dbtng_example', 'ex');
   -  $select->fields('ex', array('pid', 'uid', 'name', 'surname', 'age'));
   +  $dbtng_example_execute_ordered_select_query = db_select('dbtng_example', 'ex')->fields('ex');
      // 'ex.name' is the field on which to order.
   -  // Legal values for sorting are "ASC" and "DESC". Any other value will be converted to "ASC".
   -  $select->orderBy('ex.name', 'ASC');
   +  // Legal values for sorting are "ASC" and "DESC". Any other value will be
   +  // converted to "ASC".
   +  $dbtng_example_execute_ordered_select_query->orderBy('ex.name', 'ASC');
      // generate the result in object format.
   -  $result = $select->execute()->fetchAll();
   +  $dbtng_example_result = $dbtng_example_execute_ordered_select_query->execute()->fetchAll();
    
   -  if ($result) {
   

   I think you misunderstood me here. I meant something as follows:

   $result = dbtng_example_execute_ordered_select_query();
   
   foreach ($result as $row) {
     .. Generate the table ..
   }
   

8. +++ b/dbtng_example/dbtng_example.module
   @@ -588,39 +603,46 @@
   +    foreach ($dbtng_example_result as $row) {
          // Sanitize the data before handing it off to the theme layer.
          $rows[] = array_map('check_plain', (array) $row);
        }
   -    // Make a table for them.
   -    $header = array(t('Id'), t('uid'), t('Name'), t('Surname'), t('Age'));
   -    $output .= theme('table', array('header' => $header, 'rows' => $rows));
   +    $output .= dbtng_example_render_resultset_as_table($rows);
   

   The foreach should actually be inside of the dbtng_example_render_resultset_as_table() function

@legolasbo: Modified the code as mentioned and also attached the interdiff.

Kindly review.
Thanks!

Review of the interdiff:

1. +++ b/dbtng_example/dbtng_example.module
   @@ -602,17 +586,30 @@
   +  $output = '';
   +  if ($result) {
   +    $output .= dbtng_example_render_resultset_as_table($result);
   +  }
   +  else {
   +    drupal_set_message(t('No records found.'));
   +  }
   +  return $output;
   

   Checking wether or not there is something to render should be the task of dbtng_example_render_resultset_as_table. The entire method could be as simple as:

   dbtng_example_ordered_list() {
    $result = dbtng_example_execute_ordered_select_query();
   dbtng_example_render_resultset_as_table($result);
   }
   

   Pretty self-descriptive no? You could even change the function's docblock to "Menu callback."

2. +++ b/dbtng_example/dbtng_example.module
   @@ -620,27 +617,22 @@
   +  // return the result
   +  return $dbtng_example_result;
   

   This comment is redundant. return $dbtng_example_result; almost literally states 'return the result'

Review of the entire patch (without previous comments from the interdiff)

+++ b/dbtng_example/dbtng_example.module
@@ -574,6 +583,58 @@ function dbtng_example_form_update_submit($form, &$form_state) {
+  $output = theme('table', array('header' => $header, 'rows' => $rows));
+  return $output;

If you render the to html here, the output can't be changed anymore. If you however return a render array it can still be altered by the theme layer if required.

i.e.:

return [
  '#theme' => 'table',
  '#header' => $header,
  '#rows' => $rows,
];

The method can then be renamed to something like 'dbtng_example_convert_resultset_to_table_render_array()' which would make it self-describing.

Corrections are made in the updated patch with interdiff.

Kindly review.
Thanks!

1. +++ b/dbtng_example/dbtng_example.module
   @@ -344,6 +344,9 @@ function dbtng_example_help($path) {
   +      $output = t('Generate a list of database records in ordering format. The records will be displayed in ascending order for column "Name".');
   

   "Generate a list of database records ordered by the 'name' column in ascending order." reads better I think.

2. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,62 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   + * Render an ordered list of entries in the database.
   

   Menu callback demonstrating how to retrieve ordered results from the database.

3. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,62 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  return dbtng_example_render_resultset_as_table($result);
   

   return dbtng_example_convert_resultset_to_table_render_array($result); 

4. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,62 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  $rows = array();
   +  if($result) {
   +    foreach ($result as $row) {
   +      // Sanitize the data before handing it off to the theme layer.
   +      $rows[] = array_map('check_plain', (array) $row);
   +    }
   +  }
   

   This should live in a method called dbtng_example_convert_resultset_to_table_rows() and be called from dbtng_example_convert_resultset_to_table_render_array()

This is also missing tests that prove that everything works as expected.

Corrections are made as mentioned in #28

Kindly review.
Thanks!

1. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,63 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +/**
   + * This function demonstrates retrieving of ordered results from the database
   + */
   

   This docblock is missing an @return annotation.

2. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,63 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +/**
   + * This function queries the database and retrieves all the records in
   + * ascending order.
   + *
   + * The code below will result in the following query
   + * SELECT ex.pid AS pid, ex.uid AS uid, ex.name AS name, ex.surname AS surname,
   + * ex.age AS age
   + * FROM {dbtng_example} ex
   + * ORDER BY ex.name ASC
   + */
   

   this docblock is missing a @return annotation.

3. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,63 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  $dbtng_example_execute_ordered_select_query = db_select('dbtng_example', 'ex')->fields('ex');
   

   Maybe you should add a comment explaining the 'ex'. Or even better replace all 'ex' with $tableAlias.

4. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,63 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  // 'ex.name' is the field on which to order.
   

   Maybe this comment should also explain what 'ex' and 'name' are exactly.

5. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,63 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  // generate the result in object format.
   

   I'm not sure what you mean this comment.

6. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,63 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  return $dbtng_example_result;
   

   An empty line above the return statement more clearly separates the 2 logical steps querying and returning data.

7. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,63 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +/**
   + * This function renders a resultset to table rows
   + */
   +function dbtng_example_render_resultset_to_table_rows($result) {
   

   This function does not render anything, it just converts the resultset into table rows.

   'dbtng_example_convert_resultset_into_table_rows'

   Besides that, the docblock is missing @param and @return annotations.

8. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,63 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +      $rows[] = array_map('check_plain', (array) $row);
   

   Doesn't this result in a pass by reference warning?

9. +++ b/dbtng_example/dbtng_example.module
   @@ -574,6 +583,63 @@ function dbtng_example_form_update_submit($form, &$form_state) {
   +  $rows = dbtng_example_render_resultset_to_table_rows($result);
   

   You can inline this variable.

Added annotations as mentioned in #31

Kindly review.
Thanks!

1. +++ b/dbtng_example/dbtng_example.module
   @@ -601,22 +602,31 @@
   -  $dbtng_example_execute_ordered_select_query = db_select('dbtng_example', 'ex')->fields('ex');
   -  // 'ex.name' is the field on which to order.
   +  $dbtng_example_execute_ordered_select_query = db_select('dbtng_example', 'dbtng_example')->fields('dbtng_example');
   +  // 'dbtng_example.name' is the field on which to order where 'dbtng_example'
   +  // is a table alias and 'name' is a column name
   

   this doesn't improve the example. Something like the pseudocode below explains what is going on without the need for comments.

   $tableName = 'dbtng_example';
   $tableAlias = 'ex';
   .. = $db_select($tableName, $tableAlias)->fields($tableAlias)
   

2. +++ b/dbtng_example/dbtng_example.module
   @@ -601,22 +602,31 @@
   +function dbtng_example_converts_resultset_to_table_rows($result) {
   

   /s/converts/convert

3. +++ b/dbtng_example/dbtng_example.module
   @@ -629,9 +639,15 @@
     * This function renders array for table 'dbtng_example'
   

   This comment is incorrect. the function converts a resultset into a render array.

corrections are made as mentioned in #33.

Kindly review.
Thanks!

Hi,

Attached is the patch for delete based on multiple condition.

Hi rFay,

Please find the revised patch attached with the instructions on the link.

Thanks,
Manoj Bisht

Rescoping.

This is all good stuff. Moving to 8.x-1.x because that's current.

File an issue per item, and when it's in, we can backport to 7.x-1.x as needed in those child issues.

Moving to new examples meta

Let's move this to 4.0.x, which is for ^9.4 || ^10