[D8] Qwantsearch

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2824305

Project 2: https://www.drupal.org/node/2824844

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

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.

Automated Review

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
              FILE: /root/repos/pareviewsh/pareview_temp/qwantsearch.module
              --------------------------------------------------------------------------
              FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
              --------------------------------------------------------------------------
              8 | WARNING | Global constants should not be used, move it to a class or
              |         | interface
              9 | WARNING | Global constants should not be used, move it to a class or
              |         | interface
              --------------------------------------------------------------------------
  
              Time: 42ms; Memory: 6Mb
          

• 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.

    FILE: /root/repos/pareviewsh/pareview_temp/qwantsearch.api.php
    ----------------------------------------------------------------------
    FOUND 3 ERRORS AFFECTING 2 LINES
    ----------------------------------------------------------------------
    19 | ERROR | Type hint "array" missing for $variables
    33 | ERROR | Type hint "array" missing for $renderable_result
    33 | ERROR | Type hint "array" missing for $row
    ----------------------------------------------------------------------


    FILE: ...pareviewsh/pareview_temp/src/Service/QwantSearchDisplayInterface.php
    --------------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    --------------------------------------------------------------------------
    30 | ERROR | Type hint "array" missing for $medias
    --------------------------------------------------------------------------


    FILE: /root/repos/pareviewsh/pareview_temp/src/Service/QwantSearch.php
    --------------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    --------------------------------------------------------------------------
    47 | ERROR | Doc comment for parameter $httpClient does not match actual
    |       | variable name <'undefined'>
    --------------------------------------------------------------------------

    Time: 364ms; Memory: 8Mb

See https://pareview.sh/node/1150

Manual Review

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Notice: typo here "You must obtain a Qwant parner"

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:

(*) Major finding, needs work

1. Error: Cannot use object of type Drupal\Core\Form\FormState as array in Drupal\qwantsearch\Form\Settings->validateForm() (line 167 of modules/custom/review/qwantsearch/src/Form/Settings.php)
   Fix: change $form_state['values']['qwantsearch_http_token'] to $form_state->getValue('qwantsearch_http_token')

(+) Release blocker

1. Error while installing module via composer: composer require "drupal/qwantsearch @dev"

   Installation failed, reverting ./composer.json to its original content.
   [UnexpectedValueException]
   Could not parse version constraint ~1.x: Invalid version string "~1.x"

   Fix: replace "~1.x" to "~1.0", or any other acceptable version (optional to change version constraint)

Just a recommendations

1. use one name for module things:
   change admin/config/search/qwant to admin/config/search/qwantsearch
2. Warning: End User can override any route from modules that has lower weight,
   for example: admin/config/system/actions

   Fix: at least warn user about this possibility (in form element description)

3. Sanitise?

               $routes['qwantsearch.search_page'] = new Route(
                 '/' . trim($this->configFactory->get('qwantsearch.settings')->get('qwantsearch_search_page'), '/ '),
               

4. Assumption: provide a default value for qwantsearch_search_page variable
5. Warning: Document missed dependency for cURL
   Assumption: use Guzzle to query a Qwant endpoint instead cURL
6. <a href="{{ url }}" title="{{ title }}">{{ title }}</a>
   maybe open link in new browser tab by default?
7. Code style assumption:
   use united style for function definition (one line of func arguments vs. multiline)
   see qwantsearch/src/Plugin/Block/QwantsearchSearchBlock.php:39
   vs qwantsearch/src/Plugin/Block/QwantsearchSearchBlock.php:52
   also 6 whitespaces used to indent the code

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.

Personal opinion:

Despite of that I don't see this module is very useful due to very small favours,
I want say that this module has:
- excellent module architecture,
- can be extended via alter hooks and theme templates,
- almost united code style,
- even ready for localisation.

UPD: Add check for curl in hook_requirements
see:

$has_curl = function_exists('curl_init');

$requirements['curl'] = array(
    'title' => t('cURL'),
    'value' => $has_curl ? t('Enabled') : t('Not found'),
  );
  if (!$has_curl) {
    $requirements['curl']['severity'] = REQUIREMENT_ERROR;
    $requirements['curl']['description'] = t('The testing framework requires the <a href="https://secure.php.net/manual/en/curl.setup.php">PHP cURL library</a>. For more information, see the <a href="https://www.drupal.org/requirements/php/curl">online information on installing the PHP cURL extension</a>.');
  }

Hello @vlad.dancer,

Thank you very much for the review.

I fixed pareview results, fixed the typo and fixed composer.json.

1: ok, changed
2: I do not understand this warning. You mean in case the administrator enters an already used URL?
3: yes, I will make a check if a sanitization is needed and how to do it.
4: as you mentionned, this module will be used in very special cases, so we don't think having default values will be very useful as it will depends on clients/customers requirement.
5: done
6: Yes but we prefer to let the visitor have the choice of opening in a new tab. A site owner can override the template in its custom theme and or use modules like external links (https://www.drupal.org/project/extlink) to add the target _blank using JS.
7: done for the indentation, but in case of a plugin create method, it is almost everytime the same parameters and even if it is more than 80 characters for the create method I keep it in on one line.

Thanks again for the review.

I will check point 3 and if you can add precision on point 2 thanks.

You mean in case the administrator enters an already used URL?

Exactly.

Good job.

Hello,

2: Ok, I have added a description to help.

3: I checked for the sanitization in the path. I have tested with <script>alert('test')</script>, it works.

Thanks for the RTBC.

You're welcome!

Hello,

As there is now a new policy about the sandbox modules allowing anyone to have full project.

Do we need to wait for a security approbation as this issue is in RTBC or can we promote the sandbox?

Thanks for the reply.

Hello,

I have promoted the project.

I close this issue.