[D8]Currency Convert

Multiple Applications

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

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

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

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.

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

Hello sarah ravikumar,

can you provide me module page.

Regards.

Hello sarah ravikumar,

Your module isn't work fine, Please see screenshot
I tried to convert 1 Us Dollar to 1 Dinar Jordanian the result is 0, the result should be .708 or something like that.

Best regards.

Automated Review

[Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.]

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[No: Does not follow] 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

[No: Does not follow] 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. (*) Major finding, needs work
2. (+) Release blocker
3. * Git default branch is not set
4. + Readme.txt add table of contents in the header
5. * In Readme.txt file under the installation title given URL is Drupal7 module install step. Please give proper Drupal8 module install step URL
6. In your CurrencyConvertForm.php file you send the web service request. Please use try,catch statement to throw if any issue will occur.
7. * Validation require for amount enter text field. If I enter string or nothing to give its does not show any error message.Check here I attached currency.png image.
8. * Please explain this https://www.drupal.org/project/currency_converter module and your current module.
9. + Please add your sandbox URL page.

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.

Hello sarah ravikumar,,

Your module works on curl, so please implement "hook_requirements" to check "function_exists('curl_init')".

Thanks.

In D8 the Drupal HTTP client service should be used (Guzzle) instead of cURL. But that alone is surely not an application blocker, anything else that you found or should this be RTBC instead?

Hi @sarah ravikumar,

There are security vulnerabilities found in your code, which are as below,

In function submitForm(array &$form, FormStateInterface $form_state) function, of file CurrencyConvertForm.php
To sanitize user input, it should be passed through checkPlain.
$currency_input = $form_state->getValue('currency_convert_amount');

As we are expecting numeric value, please update below statement,
if(!empty($currency_input)){
to
if (!empty($currency_input) && is_numeric($currency_input)) {

The below line is user facing text, it should be passed through t() function and placeholders should be used to display variable values. Please correct below line,
$currency_output = 'The Converted Currency Value : ' . $currency_output;

Also, there is a similar module available Yahoo! Finance currency converter. Please check and approach maintainer if you can maintain the Drupal 8 version.

@th_tushar: no, data should NOT be sanitized when saved to the database in a submit handler: "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/node/28984

Make sure to read that article again.

Hi @sarah ravikumar,

Ignoring #21,

There are security vulnerabilities found in your code, which are as below,

As per the article posted by klausi, submitForm() function is fine.

As we are expecting numeric value, please update below statement,
if(!empty($currency_input)){
to
if (!empty($currency_input) && is_numeric($currency_input)) {

The below line is user facing text, it should be passed through t() function and placeholders should be used to display variable values. Please correct below line,
$currency_output = 'The Converted Currency Value : ' . $currency_output;

The module uses the yahoo currency converter api to convert the currency value, I would recommend the module should be renamed to yahoo_currency_converter similar like module google_currency_converter which uses the google api to convert the currency.

@sarah ravikumar

@file not required

@file is not required for classes, interfaces and traits

/src/Plugin/Block/CurrencyConvertBlock.php

/**
 * @file
 * Contains \Drupal\currency_convert\Plugin\Block\CurrencyConvertBlock.
 */

/src/Form/CurrencyConvertForm.php

/**
 * @file
 * Contains \Drupal\currency_convert\Form\CurrencyConvert.
 */

Automated Review

Coder returned some minor coding standard issues. (see attached)

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation. I understand that this module uses a different API to perform the currency conversion, but I think it's worth mentioning that there are several other modules that provide similar functionality. For example, Currency Convertor and Google Currency Convertor. It might be worth considering contirbuting to an existing project/porting to D8

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

No: Does not follow the guidelines for in-project documentation and/or the README Template. There's a bad link to the Admin Block Layout page in the README.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements

Coding style & Drupal API usage

1. (*) There is a permission "Able to convert currencies in a block" however, an authenticated user without this permission is still able to use the conversion block. I'm not sure what the intended behavior is here.
2. (+) visiting the /currency_convert route in the browser generates multiple php errors.
   

         PHP Error: User error: "ZWD" is an invalid render array key in Drupal\Core\Render\Element::children() (line 97 of /opt/apps/drupal-project/web/core/lib/Drupal/Core/Render/Element.php).
         

   This may be an indicator that currencyOptions should not be in a Controller, it might make more sense to add these as options on the CurrencyConvertForm.

3. The form error should be translated in CurrencyConvertForm (line 60)
4. Consider using Drupal Http Client instead of curl in CurrencyConvertForm.php
5. You might want to use an HTML5 number field for the currency instead of a text field (i.e. <input type="number" min="1" step="any" />)
6. Add "block" dependency to 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.

This review uses the Project Application Review Template.

Hope that's helpful.

Great Sarah,
To reproduce the error in #2, go to yoursite.co/currency_convert and then check your dblogs.
I was suggesting moving the currencyOptions to the form (rather than the Controller.)
For example, you'd add a method like this to /src/Form/CurrencyConvertForm.php

public function getCurrencyOptions(){
  return array(
      "ALL" => "Albanian Lek",
      "DZD" => "Algerian Dinar",
      "USD" => "US Dollar",
      "EUR" => "Euro",
       ...
  );
}

Then you'd update the form options like this:

$form['currency_from_format'] = array(
  '#type' => 'select',
  '#title' => $this->t('Select a currency to convert from'),
  '#options' => $this->getCurrencyOptions(),
);

This would allow you to completely remove the currency_convert.routing.yml and src/Controller/CurrencyConvertContorller.php (which doesn't seem to do anything else other than provide these select options.

#28, that's fine.if you'll rename it now you will get to know about the feedback on the renamed code.
Quick review :

1. Why are you keeping CurrencyConvertController.php ?
2. Why additional hook_requirements in currency_convert.install ? are you using/planning to use curl some where ?
3. Add a single line after <?php tag there's discussion going on regarding the same.
4. Code does not follows the coding standards and have wrong doc blocks. At lots of places new lines are needed. check your latest code on pareview.sh
5. Use single quotes in getCurrencyOptions() function and short array would be good.Also it would be good if you move this function to a new service so that logic and data would be separate from form.
6. Use this->t for submit button in buildForm() method.
7. Use form api number field that will take care of validation that you have done in validateForm method in CurrencyConvertForm
8. CurrencyConvertForm.php : Why the logic of conversion is in submitForm , move this conversion in an helper function in a service.Why are you throwing exception of curl is not setup properly.

Please take a review bonus to get your application review soon.

Git default branch is not set.
Steps to set Default branch:-
1) Edit your project
2) Click the "Default branch" tab
3) Select the desired branch
4) Click Save

In CurrencyConvertController.php file some minor errors in code like semicolon error etc.

Hi

In your comment #38 you have mentioned that you are using httpClient instead of curl to fetch the data. But still in you can see

try {
      $yql_session = curl_init($yql_query_url);
      curl_setopt($yql_session, CURLOPT_RETURNTRANSFER, TRUE);
      $json_data = curl_exec($yql_session);
    }
    catch (\Exception $e) {
    }

Please check you have committed your code in to repo properly.

Thanks,
ARUN AK

Hi Sarah,

Here are my suggestions,

In your currency convert form.php file(line no. 36), you have used number for #type. So instead of using #maxlength which is for textfield try #max. Also if you are setting the max. value to be max i don't understand the point of mentioning it explicitly. Since it's a number, please set #min to 0, because currently it allows negative value conversion. Make the currency field mandatory, thereby preventing an empty submission.

Hey Sarah,
Reviewed the code. Great start! Have some concrete feedback:

http://cgit.drupalcode.org/sandbox-sarahravikumar-2686979/tree/src/Curre...
You have a catch statement for then the API call fails. However, you aren’t doing anything there. You can respond to the API failure by logging the error and potentially returning some sort of message to the end user that something went wrong.

Additionally you might want to add a check for rate limits: https://developer.yahoo.com/search/rate.html

http://cgit.drupalcode.org/sandbox-sarahravikumar-2686979/tree/src/Form/...
Ideally you should be using dependency injection here. Instead of using the global service container.

http://cgit.drupalcode.org/sandbox-sarahravikumar-2686979/tree/src/Form/...
Need to add some sort of form validation here.

http://cgit.drupalcode.org/sandbox-sarahravikumar-2686979/tree/src/Plugi...
Need to have meaningful note. “Provides my custom block” is a little generic.

Highly recommend using guzzle. It's a bit more standard. It's there out of the box with core and you can remove you hook requirements!
http://cgit.drupalcode.org/sandbox-sarahravikumar-2686979/tree/src/Curre...

Skimming the code there are some outstanding drupal coding standard issues:

FILE: ...ten/client-repos/wfmd8/currency_convert/currency_convert.install
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
1 | ERROR | [x] The PHP open tag must be followed by exactly one
| | blank line
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...en/client-repos/wfmd8/currency_convert/src/CurrencyConverter.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
1 | ERROR | [x] The PHP open tag must be followed by exactly one
| | blank line
2 | ERROR | [x] Namespaced classes, interfaces and traits should
| | not begin with a file doc comment
18 | ERROR | [x] Whitespace found at end of line
161 | ERROR | [x] Expected 1 blank line after function; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...nt-repos/wfmd8/currency_convert/src/Form/CurrencyConvertForm.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
1 | ERROR | [x] The PHP open tag must be followed by exactly one
| | blank line
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...wfmd8/currency_convert/src/Plugin/Block/CurrencyConvertBlock.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
1 | ERROR | [x] The PHP open tag must be followed by exactly one
| | blank line
2 | ERROR | [x] There must be one blank line after the namespace
| | declaration
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Hi sarah,

What problems do you have with httpclient? I've done a quick test changing curl by httpclient and I don't see any errors.

On the other hand, I think an interesting improvement would be that the form was with ajax. What do you think?

Hi sarah ravikumar,

Kindly see the automated review of your project as its showing some errors and warnings. Meanwhile, i am using your project and will let you know if found any errors.

Link: https://pareview.sh/node/1580

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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