Closed (outdated)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
26 Jun 2016 at 13:54 UTC
Updated:
29 Mar 2017 at 13:08 UTC
Jump to comment: Most recent
Comments
Comment #2
PA robot commentedThere are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgsandboxjunkuncz2742467git
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.
Comment #3
dilipsingh02 commentedAutomated Review:
Git errors:
Git default branch is not set, see the documentation on setting a default branch.
Review of the 7.x-1.x branch (commit ae6a239):
Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
-----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
-----------------------------------------------------------------------
2 | WARNING | [ ] Line exceeds 80 characters; contains 245 characters
3 | ERROR | [x] Expected 1 newline at end of file; 0 found
-----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------
Time: 195ms; Memory: 8.75Mb
DrupalPractice has found some issues with your code, but could be false positives.
FILE: /var/www/drupal-7-pareview/pareview_temp/partner_links.install
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
16 | ERROR | Do not use t() or st() in installation phase hooks, use $t
| | = get_t() to retrieve the appropriate localization function
| | name
19 | ERROR | Do not use t() or st() in installation phase hooks, use $t
| | = get_t() to retrieve the appropriate localization function
| | name
--------------------------------------------------------------------------
FILE: /var/www/drupal-7-pareview/pareview_temp/partner_links.module
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
--------------------------------------------------------------------------
29 | WARNING | Open page callback found, please add a comment before
| | the line why there is no access restriction
37 | WARNING | Open page callback found, please add a comment before
| | the line why there is no access restriction
45 | WARNING | Open page callback found, please add a comment before
| | the line why there is no access restriction
53 | WARNING | Open page callback found, please add a comment before
| | the line why there is no access restriction
303 | WARNING | Unused variable $absolute.
--------------------------------------------------------------------------
Time: 98ms; Memory: 6.5Mb
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.
Comment #4
junkunczFixes added based on observations.
Comment #5
dilipsingh02 commentedPlease add the hook_uninstall() in .install file to remove any information that the module sets.
Manual Review
Individual user account
Yes
No duplication
Yes
Master Branch
Yes
Licensing
Yes
3rd party assets/code
Yes
README.txt/README.md
YES
Code long/complex enough for review
YES
Secure code
yes
Coding style & Drupal API usage
No (Please add the hook_uninstall() in .install file to remove any information that the module sets.)
Comment #6
junkunczUninstall hook added.
Comment #7
sanjayk commentedManual Review
I have manually review and run the code.
When I click on 'Check Partners' button redirect on 'Status report' page and show an error
Error:
Partner Links
PHP CURL library is not installed. Installation instructions
I have checked phpinfo it's already enabled.
And also add instruction in .info file for Curl requirement.
Comment #8
yogeshmpawarManual 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
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: Not Follows the guidelines for in-project documentation and/or the README Template.
Please refer to this link https://www.drupal.org/node/2181737 for README Template and please provide some more information about module mentioned in template.
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
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
(*) Nothing
(+) It would be good, if you implement hook_help in your module.
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.
Comment #9
heykarthikwithuManual review:
1. In
admin/structure/partner-links, if you inject javascript in the 'Partner Name' and save the form. It saves the form and the injected javascript would be executed on page load.2. Instead of
_partner_links_sanitize_url($url), you can use thevalid_urlrefer this.And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
Comment #10
junkunczThe code was completely refactored and improved. The security bugs are fixed of course.
Comment #11
junkunczComment #12
junkunczComment #13
PA robot commentedProject 1: https://www.drupal.org/node/2819693
Project 2: https://www.drupal.org/node/2755901
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.
Comment #14
junkunczEverything works as expected. Full code review is done and fixes added.
https://www.drupal.org/project/issues/2742467?status=All&categories=All
I think is ready to go live.
Comment #15
klausiPlease don't rtbc your own issue, see the workflow: https://www.drupal.org/node/532400
Comment #16
junkuncz