Quivers offers an omnichannel commerce solution, empowering brands to control the online-to-offline consumer shopping experience, leveraging retailers to help in the fulfillment process.

Project link

https://www.drupal.org/project/quivers

Git instructions

git clone --branch 8.x-1.x https://git.drupalcode.org/project/quivers.git

PAReview checklist

https://pareview.sh/pareview/https-git.drupal.org-project-quivers.git

Comments

quiversdevelopment created an issue. See original summary.

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented
Status: Needs review » Needs work

Fix below issues :

Git errors:
There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732
Review of the 8.x-1.x branch (commit ecfb905):
The quivers.module does not implement hook_help(). See https://www.drupal.org/docs/develop/documenting-your-project/module-docu... .
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: ...01/web/vendor/drupal/pareviewsh/pareview_temp/src/QuiversService.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
277 | WARNING | Unused variable $itemUnitPrice.
--------------------------------------------------------------------------

FILE: .../vendor/drupal/pareviewsh/pareview_temp/src/Form/TaxSettingsForm.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
220 | WARNING | Unused private method loadMarketplaces()
--------------------------------------------------------------------------

Time: 3.01 secs; Memory: 8Mb
No automated test cases were found, did you consider writing 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.

FILE: ...pal/pareviewsh/pareview_temp/config/install/views.view.pricelist.yml
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
182 | ERROR | [x] Expected 1 newline at end of file; 2 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: .../pareview_temp/config/install/rest.resource.quivers_entity.media.yml
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
17 | ERROR | [x] Expected 1 newline at end of file; 2 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...areviewsh/pareview_temp/config/install/views.view.pricelist_item.yml
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
219 | ERROR | [x] Expected 1 newline at end of file; 2 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...web/vendor/drupal/pareviewsh/pareview_temp/src/OrderItemSplitter.php
--------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
--------------------------------------------------------------------------
10 | ERROR | [x] Missing class doc comment
34 | ERROR | [ ] Expected type hint "OrderInterface"; found
| | "EntityTypeManagerInterface" for $entity_type_manager
42 | ERROR | [x] Data types in @param tags need to be fully namespaced
45 | ERROR | [x] Parameter comment indentation must be 3 spaces, found 2
| | spaces
47 | ERROR | [x] Expected
| | "bool|\Drupal\commerce_order\Entity\OrderItemInterface"
| | but found
| | "boolean|\Drupal\commerce_order\Entity\OrderItemInterface"
| | for function return type
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...dor/drupal/pareviewsh/pareview_temp/src/QuiversMiddlewareService.php
--------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------
107 | ERROR | [ ] Parameter comment must start with a capital letter
113 | WARNING | [x] A comma should follow the last multiline array item.
| | Found: "Drupal"
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...r/drupal/pareviewsh/pareview_temp/src/Plugin/jsonrpc/Method/Ship.php
--------------------------------------------------------------------------
FOUND 1 ERROR AND 3 WARNINGS AFFECTING 4 LINES
--------------------------------------------------------------------------
23 | WARNING | [ ] Line exceeds 80 characters; contains 83 characters
24 | WARNING | [ ] Line exceeds 80 characters; contains 84 characters
25 | WARNING | [ ] Line exceeds 80 characters; contains 89 characters
36 | ERROR | [x] Expected "\Drupal\quivers\OrderItemSplitter" but
| | found "\Drupal\quivers\OrderItemSplitter;" for @var
| | tag in member variable comment
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...iewsh/pareview_temp/src/Plugin/jsonrpc/Method/SetOrderStateTrait.php
--------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------
9 | ERROR | [x] Missing trait doc comment
20 | ERROR | [x] Data types in @param tags need to be fully namespaced
57 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...areviewsh/pareview_temp/src/Plugin/jsonrpc/Method/ShippingUpdate.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
20 | WARNING | Line exceeds 80 characters; contains 85 characters
--------------------------------------------------------------------------

FILE: ...drupal/pareviewsh/pareview_temp/src/Plugin/jsonrpc/Method/Refund.php
--------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------
23 | WARNING | [ ] Line exceeds 80 characters; contains 84 characters
24 | WARNING | [ ] Line exceeds 80 characters; contains 88 characters
35 | ERROR | [x] Expected "\Drupal\quivers\OrderItemSplitter" but
| | found "\Drupal\quivers\OrderItemSplitter;" for @var
| | tag in member variable comment
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...drupal/pareviewsh/pareview_temp/src/Plugin/jsonrpc/Method/Cancel.php
--------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------
35 | ERROR | [x] Expected "\Drupal\quivers\OrderItemSplitter" but found
| | "\Drupal\quivers\OrderItemSplitter;" for @var tag in
| | member variable comment
46 | ERROR | [ ] Parameter $order_item_splitter is not described in
| | comment
58 | ERROR | [ ] Unknown type hint "MethodInterface" found for
| | $plugin_definition
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...pal/pareviewsh/pareview_temp/src/EventSubscriber/RouteSubscriber.php
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
8 | ERROR | [x] Missing class doc comment
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...01/web/vendor/drupal/pareviewsh/pareview_temp/src/QuiversService.php
--------------------------------------------------------------------------
FOUND 136 ERRORS AND 8 WARNINGS AFFECTING 88 LINES
--------------------------------------------------------------------------
79 | ERROR | [x] Missing function doc comment
81 | ERROR | [x] Inline comments must start with a capital letter
81 | ERROR | [x] Inline comments must end in full-stops, exclamation
| | marks, colons, question marks, or closing
| | parentheses
82 | ERROR | [x] Expected 1 space before "/"; 0 found
82 | ERROR | [x] Expected 1 space after "/"; 0 found
83 | ERROR | [x] Inline comments must start with a capital letter
83 | ERROR | [x] Inline comments must end in full-stops, exclamation
| | marks, colons, question marks, or closing
| | parentheses
84 | ERROR | [x] Expected 1 space after FOR keyword; 0 found
84 | ERROR | [x] Expected 1 space before "="; 0 found
84 | ERROR | [x] Expected 1 space after "="; 0 found
84 | ERROR | [x] Expected 1 space before "
Reviewed on
03/11/2020 - 17:42
Powered by PAReview.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
Status: Needs work » Needs review

Thank you for applying! I added the PAReview checklist link. Reviewers will check the project and post comments to list what should be changed.

If you haven't done it, yet, please check the PAReview report and fix what needs to be fixed. There could be some false positives; verify that what reported is correct, before making any change.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

There are 27 commits done from saurabht2b. If there is somebody that can apply using that module is saurabht2b, since he has the major of commits done in that module.
Other users need to use a different module for these applications which, I remember, aren't about projects, but users. The purpose of these applications is giving to users a Drupal role to make them able to opt into security coverage, not altering the status of projects.

This application can continue if saurabht2b will handle it, from now on.

quiversdevelopment’s picture

quiversdevelopment commented

Hi Kiamlaluno,

Thank you for your feedback. Saurabh is no longer contributing to this project. Since user quiversdevelopment is primarily responsible for contributions to this project at present, is it possible to proceed based on this user?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Needs review
Issue tags: +PAreview: single application approval
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

Thanks for your contribution!

  1. ConfigSettingsForm::validateForm(): a validation callback should never save configuration, that should only be done in a submit handler.
  2. ConfigSettingsForm::validateForm(): do not set form errors on the messenger service directly. Use $form_state->setErrorByName() instead. See https://www.drupal.org/docs/8/api/form-api/introduction-to-form-api
  3. $this->logger->error("Region not supported with Quivers - " . (string) $order_region_code . "|" . (string) $order_country_code);: do not concatenate variables into log messages, use placeholder instead. See https://drupalize.me/blog/201510/how-log-messages-drupal-8 . Please check all your log calls and use placeholders for variables.
  4. QuiversService: I see a lot of float conversions here. Never use float for price values as they are not precise. Use Drupal\commerce_price\Calculator with strings to do all your arithmetic correctly instead of using float. Not sure if this qualifies as security issue and what could potentially go wrong, but this should be fixed if possible.
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

If the floating point value is not used, for example, to authenticate a user, or encrypt a string, it's not a security issue.
Floating point values are subject to rounding errors, and there are surely better way to handle calculations on currency values, but this is not a wrong use of Drupal API, and we cannot say that a module necessarily need to depend from a third-party module.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

They already depend on Drupal Commerce, that includes Drupal\commerce_price\Calculator. So that is always available for Commerce modules.

Anyway, I would still wait for the logger improvements and then we could approve this.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Priority: Normal » Minor
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Closed (won't fix)

I am closing this application due to lack of replies.