Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
28 May 2019 at 16:30 UTC
Updated:
5 Aug 2019 at 09:29 UTC
Jump to comment: Most recent
Comments
Comment #2
marcinkazmierski commentedComment #3
marcinkazmierski commentedComment #4
avpadernoThank you for applying! I added the Git instructions for non-maintainer users. 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.
Comment #5
avpadernoDrupalclass ort()when dependency injection is available, for example inside plugin classesComment #6
marcinkazmierski commentedHi kiamlaluno,
Thanks for review!
I fix all of errors from PAReview report.
Is there anything else I can fix? :)
Comment #7
marcinkazmierski commentedComment #8
vuilHi! Thank you for the submission.
Please review the 8.x-1.x branch and fix the following issues:
User::loadcalls should be avoided in classes, use dependency injection instead\Drupalcalls should be avoided in classes, use dependency injection insteadComment #9
marcinkazmierski commentedDone :)
Comment #10
vuilThank you for the contribution!
Please, fill the uncompleted sections in README.md with the required content (text) at first:
## INTRODUCTION
todo...
## CONFIGURATION
todo...
The descriptive documentation, and code comments, are the main requirements for each code. Please update also the module page with the required content (text) with the specific (h3 & other) tags inside.
And, be patience through the whole review process.
Comment #11
marcinkazmierski commentedThank you for review!
hook_help, readme and module description - updated :)
Comment #12
vuilThank you for the contribution!
Could enable some "Automated tests" here? (example)
I want to see what will resulted there at first.
Please, be patience through the whole review process.
Comment #13
marcinkazmierski commentedThank you for review!
Added: "Automated tests".
Comment #14
seroton commentedThanks for your module!
Haven't installed yet but here's 2 things I noticed:
-You've added a hook_help but also there is an open issue about it on your module's issue queue since almost a month ago that looks ignored. Please review and close if fixed as a release shouldn't have open issues, also please try to be on top of that as a module maintainer.
-Not too sure about this but there is a .gitignore left in your repo that shouldn't be there.
Comment #15
marcinkazmierski commented.gitignore - deleted
opened issues - they were closed
Comment #16
vuilI have not additional security issues found. Thank you!
Comment #17
marcinkazmierski commented@ilchovuchkov - Thank you for review!
What is the next step? :)
Comment #18
vuilPlease, be patience through the whole process... Thanks.
* Note: Please, don't change the current status.
Comment #19
avpadernoModules don't need to implement
hook_uninstall()to delete the database tables they use; that is done by Drupal core using thehook_schema()implementation.Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:
You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
I thank all the dedicated reviewers as well.
Comment #20
avpadernoComment #21
marcinkazmierski commentedThank you!
@kiamlaluno Is everything alright?
What I have to do to get a green bar with text: "Stable release covered by the Drupal Security Team" in download section on module page? (https://www.drupal.org/project/rest_api_access_token) :)
Comment #22
avpadernoYou need to edit the project to opt into security coverage.
Comment #23
marcinkazmierski commented@kiamlaluno, it works, thank you for your help! :)
Comment #24
avpadernoComment #25
vuil