Hello,

mysql_wait_timeout is a simple module which aims at setting different values for MySQL's "wait_timeout" parameter depending on what Drupal is about to do: regular execution (= generate a HTTP response), cron job or Drush command.
It is more simple than db_tweaks, which is Drupal 6-only and covers more MySQL settings but without achieving the single goal of wait_timeout.

More details about this module can be found on its project page : https://www.drupal.org/sandbox/xavier_g/2364137

The code can be cloned this way:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/xavier_g/2364137.git mysql_wait_timeout

Alas, I must confess I never reviewed any sandbox project so far.
Thanks in advance for your review.

CommentFileSizeAuthor
#18 patch_undefined_variables.txt434 bytescllamas

Comments

PA robot’s picture

PA robot commented

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

PA robot’s picture

PA robot commented
Status: Active » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxxavier_g2364137git

I'm a robot and this is an automated message from Project Applications Scraper.

xavier_g’s picture

xavier_g commented
Status: Needs work » Active
xavier_g’s picture

xavier_g commented

I have realized that Drupal 7 allows setting "init_commands" when defining the $databases array in settings.php:

<?php
// By default, do not enforce MySQL's wait_timeout
$mysql_wait_timeout = NULL;

// TODO compute best value for $mysql_wait_timeout here
/*
 * This is clearly the best place to enforce MySQL's wait_timeout (especially
 * compared to hook_init()): there is no need to check we are dealing with
 * MySQL and the command is executed almost immediately after the MySQL
 * connection was established.
 * We should be able to handle the same cases as the .module file:
 *   - default: obviously, we can handle it in settings.php
 *   - drush: we can check if function_exists('drush_main')
 *   - cron: we can look for cron.php in $_SERVER or dive into Drush functions
 *     to see how to retrieve the current Drush command
 */

if (!is_null($mysql_wait_timeout)) {
  // we could iterate over databases and check their
  // "driver" attribute instead of treating only default/default
  $databases['default']['default']['init_commands'] = array(
    'SET SESSION wait_timeout = ' . $mysql_wait_timeout,
  );
}

I will dive into this approach and determine if, in the end, this module is really needed.

xavier_g’s picture

xavier_g commented

I finally decided to convert the D7 module into a D6 one (because Drupal 6 does not provide the "init_commands" mechanism and thus still benefit from the initial approach. The D7 module itself was replaced with a simple file to be included in settings.php.

xavier_g’s picture

xavier_g commented
Status: Active » Needs review
Chetan Sharma’s picture

Chetan Sharma commented

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
12 | WARNING | Variable $conf is undefined.

remove $conf from if condition and keep it outside from the condition thanks.

http://pareview.sh/pareview/httpgitdrupalorgsandboxxavierg2364137git

xavier_g’s picture

xavier_g commented

Regarding #7: I am afraid none of this makes any sense to me...

For the record, we are speaking of the following piece of code, meant to be included into settings.php:

// Initialize $conf, just in case.
if (!isset($conf)) {
  $conf = array();
}
  • "12 | WARNING | Variable $conf is undefined." -- of course it isn't, at least when looking at the file without any clue on its include context; that's why I am testing whether it is actually defined on runtime using isset() -- isn't that a false positive from pareview? Perhaps induced by a recent improvement? (as I do not recall pareview reporting anything when I submitted this module...)
  • "remove $conf from if condition" -- err... and what would remain in that condition?
rajeev_drupal’s picture

rajeev_drupal commented
Assigned: Unassigned » rajeev_drupal

Reviewing this

rajeev_drupal’s picture

rajeev_drupal commented
Issue tags: +#DCM2015

Automated Review

Below issues are found in pareview.sh

DrupalPractice has found some issues with your code, but could be false positives.
FILE: /var/www/drupal-7-pareview/pareview_temp/wait_timeout.inc.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
12 | WARNING | Variable $conf is undefined.
----------------------------------------------------------------------

Time: 17ms; Memory: 3Mb

Manual Review

<

Individual user account
Yes: Follows follow the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements. / No: List of security issues identified.]
Coding style & Drupal API usage
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
  1. Config Link is not available in .info file

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Assigned: rajeev_drupal » Unassigned

Unassigned.So that other will take a look at it.Thanks for the review.

tgates’s picture

tgates commented

Because this project is MySQL specific, I suggest that you put MySQL in the project name.

The D7 file wait_timeout.inc.php assumes that $databases['default']['default'] is a MySQL database. However Drupal support other databases as well, the 'init_commands' won't work on some non-MySQL databases. You can test for the presence of MySQL by looking for $databases['default']['default']['driver'] to be "mysql".

mojiferous’s picture

mojiferous
he/him
commented

Manual Review

1. You can probably quiet down the undefined $conf by using a ternary like $conf = (isset($conf)) ? $conf : array();
2. Line 15 can probably be just if(empty($databases)){ since empty will return true for a null or unassigned variable.

chandrasekhar539’s picture

chandrasekhar539 commented

Automated Review

everything fine

Manual Review

Individual user account
Yes: Follows follow the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements. / No: List of security issues identified.]

Chetan Sharma’s picture

Chetan Sharma commented

You can use ternary operator rather than if condition
if (!isset($conf)) {
$conf = array();
}

Example : $conf = (isset($conf)) ? $conf : array();

monojnath’s picture

monojnath commented

Hi

I tried to use your module.
But, it seems to lack the basic documention for the project.
Please visit "https://www.drupal.org/node/2190239".
Also I have seen some issues logged by "http://pareview.sh/pareview/httpgitdrupalorgsandboxxavierg2364137git".
And its a simple .inc file.
So it would be great if you can create your own .module file that would include the .inc file when enabled. :)

fabian.fernandes_30’s picture

fabian.fernandes_30 commented

Hi xavier_g,
good work

there are couple of issues that are simple fixes,

Review of the 7.x-1.x branch (commit e3fc4f4):

DrupalPractice has found some issues with your code, but could be false positives.
FILE: /var/www/drupal-7-pareview/pareview_temp/wait_timeout.inc.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
15 | WARNING | Variable $databases is undefined.
15 | WARNING | Variable $databases is undefined.
20 | WARNING | Variable $conf is undefined.
----------------------------------------------------------------------

cllamas’s picture

cllamas commented
StatusFileSize
new patch_undefined_variables.txt434 bytes

I've modified the code to fix the undefined variables warnings. (patch attached)

Chetan Sharma’s picture

Chetan Sharma commented

if (isset($databases) && empty($databases)) {
return 1;
}
Hi cllamas,
this condition will never execute.

patrickscheffer’s picture

patrickscheffer commented
Status: Needs review » Needs work

Hi! Great idea to address this MySQL problem in a seperate script. You're right it needs no complete module.

The automated tests gave still some issues:

This one is easy to fix.
24 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, colons, or question marks

I guess the following can be ignored as the Drupal Best Practices does not take script including into account.
15 | WARNING | Variable $databases is undefined.
15 | WARNING | Variable $databases is undefined.
20 | WARNING | Variable $conf is undefined.

However, as Chetan Sharma above noted: the first if statement is incorrect. A variable can never be 'isset' and 'empty' at the same time. I think the empty check would be enough as that one also contains an isset. Check it out here.

xavier_g’s picture

xavier_g commented

Hey guys! Long time no see :')

So, let's review the reviews...

tgates:

Because this project is MySQL specific, I suggest that you put MySQL in the project name.

You can test for the presence of MySQL by looking for $databases['default']['default']['driver'] to be "mysql".

Two excellent suggestions in a single comment! THAT is a review! I will apply these advices :)

Mojiferous:
Working around pareview with a ternary? Ok, I guess it is worth a try...
empty() instead of isset() && empty() makes sense too (as "empty() does not generate a warning if the variable does not exist" according to the PHP documentation), I will apply that advice.

monojnath:
I believe you forgot to read README.txt :)

PatrickScheffer:

I guess the following can be ignored as the Drupal Best Practices does not take script including into account.

Ah! Some common sense, at least! Thank you, thank you so much.

A variable can never be 'isset' and 'empty' at the same time.

Never say never -- consider this:

<?php
$databases = '';
print isset($databases) ? 'isset ' : 'not set ';
print empty($databases) ? 'empty' : 'not empty';

print "\n";
$databases = array();
print isset($databases) ? 'isset ' : 'not set ';
print empty($databases) ? 'empty' : 'not empty';

print "\n";
$databases = FALSE;
print isset($databases) ? 'isset ' : 'not set ';
print empty($databases) ? 'empty' : 'not empty';

It will output:

isset empty
isset empty
isset empty

So, here is what I am going to do: I will take these suggestions into account so that mysql_wait_timeout checks it is dealing with a MySQL database and I will strive to reduce the pareview output as much as possible. However, as some of you understood, this is not intended to be a real, complete module (that's more of a common configuration snippet that ought to be standardized) and at some point, it would be wise to ignore the output of the almighty automated review tools :)
Thanks to all of you for the reviews.

xavier_g’s picture

xavier_g commented
Title: [D7] wait_timeout » [D7] mysql_wait_timeout
Issue summary: View changes
xavier_g’s picture

xavier_g commented

And done. Alas, The automated review still cries about undefined variables:

FILE: /var/www/drupal-7-pareview/pareview_temp/mysql_wait_timeout.inc.php
-------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 3 LINES
-------------------------------------------------------------------------
18 | WARNING | Variable $databases is undefined.
23 | WARNING | Variable $databases is undefined.
28 | WARNING | Variable $conf is undefined.
28 | WARNING | Variable $conf is undefined.
-------------------------------------------------------------------------

Again, I suggest we simply ignore those warnings.

patrickscheffer’s picture

patrickscheffer commented

Is your code ready for review? If so, please update your status to Needs review so we know it's our turn to do something.

xavier_g’s picture

xavier_g commented
Status: Needs work » Needs review
patrickscheffer’s picture

patrickscheffer commented
Status: Needs review » Reviewed & tested by the community

Alright, I've confirmed the requested changes are processed so I'll update the status to RTBC.

If you'd like to get rid of those annoying warnings in the automated review you can wrap those pieces of code in ignore tags:

// @codingStandardsIgnoreStart
$conf = isset($conf) ? $conf : array();
// @codingStandardsIgnoreEnd

Of course, you don't have to do this, but maybe you're as paranoid as me and must have a clean test result. ;-)

mlncn’s picture

mlncn commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution! Congratulations, you are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to a full project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Status: Fixed » Closed (fixed)

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

xavier_g’s picture

xavier_g commented

PatrickScheffer: thanks for the @codingStandardsIgnore advice, I have just applied it.
mlncn: thanks a lot :)