[D8] Captcha Questions

Automated Review

Review of the 8.x-1.x branch (commit 3191536):

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: ...a_questions_dblog/src/Controller/CaptchaQuestionsDblogController.php
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  --------------------------------------------------------------------------
   30 | WARNING | \Drupal calls should be avoided in classes, use
      |         | dependency injection instead
   53 | WARNING | \Drupal calls should be avoided in classes, use
      |         | dependency injection instead
  --------------------------------------------------------------------------
  
  
  FILE: ...s/pareviewsh/pareview_temp/src/Form/CaptchaQuestionsSettingsForm.php
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 6 WARNINGS AFFECTING 6 LINES
  --------------------------------------------------------------------------
    51 | WARNING | \Drupal calls should be avoided in classes, use
       |         | dependency injection instead
    60 | WARNING | \Drupal calls should be avoided in classes, use
       |         | dependency injection instead
   131 | WARNING | \Drupal calls should be avoided in classes, use
       |         | dependency injection instead
   134 | WARNING | \Drupal calls should be avoided in classes, use
       |         | dependency injection instead
   218 | WARNING | \Drupal calls should be avoided in classes, use
       |         | dependency injection instead
   257 | WARNING | \Drupal calls should be avoided in classes, use
       |         | dependency injection instead
  --------------------------------------------------------------------------
  
  
  FILE: /root/repos/pareviewsh/pareview_temp/captcha_questions.module
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  ----------------------------------------------------------------------
   37 | WARNING | Unused variable $key.
   61 | WARNING | Unused variable $user.
  ----------------------------------------------------------------------
  
  Time: 37ms; Memory: 6Mb
  

• Codespell has found some spelling errors in your code.
  

  ./README.txt:12: throughly  ==> thoroughly
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review:

1.  $config = $this->config('captcha_questions.settings')->set('captcha_questions_watchdog', $form_state->getValue('captcha_questions_watchdog'));
       $config = $this->config('captcha_questions.settings')->set('captcha_questions_dblog', $form_state->getValue('captcha_questions_dblog'));
       $config = $this->config('captcha_questions.settings')->set('captcha_questions_question', $form_state->getValue('captcha_questions_question'));
       $config = $this->config('captcha_questions.settings')->set('captcha_questions_description', $form_state->getValue('captcha_questions_description'));
       $config = $this->config('captcha_questions.settings')->set('captcha_questions_form_ids', $form_state->getValue('captcha_questions_form_ids'));
       $config = $this->config('captcha_questions.settings')->set('add_form_name', $form_state->getValue('add_form_name'));
       $config->save();

   I saw many calls $this->config('captcha_questions.settings'); for settings values in a method. IMHO store this in a variable and then use it to set values.

2. AFAIK set() method returns back configuration object, so instead of using set() multiple times with $this->config('captcha_questions.settings');. Just
   

   $this->config('captcha_questions.settings')
     ->set('key1', 'value1')
     ->set('key2', 'value2');

3. Same as above for save() method

@Yogesh Pawar

In CaptchaQuestionsDblogController.php file on line 32 you have used $db = Database::getConnection(); for db connection please use dependency instead of using it in this way.

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

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

@yogesh-pawar That's what i wanted to say. It is good practice to follow Drupal Practices. I would advise you to fix all the errors and warnings in pareview if possible. Thanks

@yogesh

Hi I have found some errors in CaptchaQuestionsSettingsForm.php on line 182 & 310 $webform_forms = $this->captchaQuestionsGetWebforms(); in this function you have used select query to load the nids & title of webform node types, if there are no nodes of type webform then it will through errors:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '))' at line 1: SELECT n.nid AS nid, n.title AS title FROM {node} n WHERE (n.type IN ()); Array ( ) in Drupal\captcha_questions\Form\CaptchaQuestionsSettingsForm->captchaQuestionsGetWebforms() (line 341 of modules/captcha_questions/src/Form/CaptchaQuestionsSettingsForm.php).

Please add a check of rowcount or similar before executing this query.

Hi Yogesh Pawar

Great work,

Suggestion:
In your file "captcha_questions.module" you have used the code

 $question_asked = \Drupal::config('captcha_questions.settings')->get('captcha_questions_question');
  $answers = \Drupal::config('captcha_questions.settings')->get('captcha_questions_answers');

recommend you to please use the config object once and the use in your code below like:

$config = \Drupal::config('captcha_questions.settings');
 $question_asked =$config->get('captcha_questions_question');
  $answers = $config->get('captcha_questions_answers');

Similarly avoid the below code as well

if (\Drupal::config('captcha_questions.settings')->get('captcha_questions_watchdog')) {

use the code like

if ($config ->get('captcha_questions_watchdog')) {

Hi Yogesh Pawar,

Grt work there are some more issues please find below.

1. Error message found.
User error: Invalid placeholder (!form_id) in string: Blocked submission of form with form_id !form_id. Answer given was %answer_given in Drupal\Component\Render\FormattableMarkup::placeholderFormat() (line 235 of /home/d8demo1/web/core/lib/Drupal/Component/Render/FormattableMarkup.php) #0 /home/d8demo1/web/core/includes/bootstrap.inc(548): _drupal_error_handler_real(256, 'Invalid placeho...', '/home/d8demo1/w...', 235, Array) #1 [internal function]: _drupal_error_handler(256, 'Invalid placeho...', '/home/d8demo1/w...', 235, Array) #2

2. Custom form_id setting breaks when i try to add invalid form id and try to save the configuration.

Hi @yogesh-pawar

Automated test

I've run a new Automated test and there are still 24 ERRORs + 8 WARNINGs:
https://pareview.sh/node/1063

Manual test

I found a bug in the admin page, when you add a custom form name and try to save the configuration.

Error: Call to a member function formatPlural() on null in Drupal\captcha_questions\Form\CaptchaQuestionsSettingsForm->submitForm() (line 269 of ...\drupal8\modules\custom\captcha_questions\src\Form\CaptchaQuestionsSettingsForm.php) #0 [internal function]: 

Steps to reproduce below:

1. Go to admin page (admin/config/people/captcha_questions)
2. Add a custom form name (ex: "test")
3. Do NOT check the checkbox for this custom form name
4. Save the form
5. You see The website encountered an unexpected error. Please try again later.

$form['description'] = array(
      '#type' => 'markup',
      '#markup' => $this->t("This is just a very simple mechanism to protect from automated bots. To make it as easy for real humans, consider providing the answer with the question. Example '<em>What is Mickeys last name? It's \"Mouse\"</em>'. You could also do simply 'What is 1+1?', but some bots may figure that out."),
    );

Use '%' instead to instead of . Refer here.

'#title' => $this->t('Logging options'),
// Controls the HTML5 'open' attribute. Defaults to FALSE.
'#open' => TRUE,

Comment can be removed.

'#default_value' => $this->config('captcha_questions.settings')->get('captcha_questions_watchdog'),

Can be reduced to $config = $this->config('captcha_questions.settings'); $config->get('captcha_questions_watchdog') as you are using $this->config('captcha_questions.settings') more than once :-)

Similarly,
$this->config('captcha_questions.settings') can be optimized.

// Disables checkbox if captcha_questions_dblog module is not enabled.
    $form['configuration']['captcha_questions_dblog'] = array(
      '#type' => 'checkbox',
      '#title' => $this->t('Enable logging internal database table'),
      '#description' => $this->t('Check this box to log all failed submissions in separate database table'),
      '#default_value' => $this->config('captcha_questions.settings')->get('captcha_questions_dblog'),
      '#disabled' => $this->moduleHandler->moduleExists('captcha_questions_dblog') ? FALSE : TRUE,
    );

A more clear description would be great with a hint for enabling Captcha Questions Dblog module.

base_route: system.admin_content
I am not very sure why it is chosen to appears in here with admin/content local tasks with url as settings route is "admin/config/people/captcha_questions".

/**
   * Helper function to find webform form_ids.
   *
   * Adapted from the function webform_mollom_form_list() in webform.module.
   * Returns array keyed by webform form_ids and title, nid of the node.
   */
  public function captchaQuestionsGetWebforms() {
    $forms = array();
    $webform_node_types = $this->config('captcha_questions.settings')->get('webform_node_types');

    $result = $this->database->select('node_field_data', 'n')
      ->fields('n', array('nid', 'title'))
      ->condition('n.type', $webform_node_types, 'IN')
      ->execute();

    foreach ($result as $node) {
      $form_id = 'webform_client_form_' . $node->nid;
      $forms[$form_id] = array(
        'title' => $this->t('@name form', array('@name' => $node->title)),
        'nid' => $node->nid,
      );
    }
    return $forms;
  }

If I am not wrong, this isn't the correct way of finding webform ids.

Empty everything and hit save. Wolla! loads of errors.

$form_state->setErrorByName('captcha_questions_answer', $this->t('Please provide an answer'));

Typo here: *$form['settings']['captcha_questions_answers']*, and if I am not mistaken, but you can use #required as an alternative, until and unless you aren't trying to achieve something else.

    // Some form nids that might exist on the site.
    $form_ids = array(
      'contact_site_form',
      'contact_personal_form',
      'user_register_form',
      'user_pass',
      'user_login_form',
      'user_login_block',
      'forum_node_form',
    );

I doubt if all the form ids are available ex: contact_site_form, contact_personal_form
Some of the forms available on Drupal 8 vanilla installation:

1. contact_message_feedback_form - By contact module
2. personal - By contact module
3. webform_submission_contact_form - By webform module
4. comment_comment_form - By comment module
5. search_block_form - By search module

// Fetching webform form_ids.
    if ($this->moduleHandler->moduleExists('webform')) {
      $webform_forms = $this->captchaQuestionsGetWebforms();
      foreach ($webform_forms as $webform_id => $webform) {
        $form_ids[] = $webform_id;
      }
    }

Webform ids cannot be retrieved by this method.

$form_state->getValue('captcha_questions_watchdog')

Multiple calls to getValue() can be reduced by using getValues()

* Helper function to make a list of candidates of form ids.
Its recommended to keep form clean and move supporting functions either as a service or in module file.

Review of your submodule: Captcha Questions Dblog

File: captcha_questions_dblog.install

$pass_link = \Drupal::l(t('Configure'), Url::fromRoute('captcha_questions_settings'));
Use Link function instead on \Drupal::l().

@navneet0693: only use the % placeholder for $this->t() when you have a variable to be into the string. If not then em tags on string literals are fine.

Otherwise those are some nice tips for improvements but surely not application blockers. Anything else that you found or should this be RTBC instead?

@klausi None of them are application blockers, the suggestions can be improved later on :-). Marking RTBC.

This module is working fine for me. Great work.RTBC+