- Issue created by @martelus
- 🇮🇳India vishal.kadam Mumbai
Thank you for applying! Reviewers will review the project files, describing what needs to be changed.
Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.
To reviewers: Please read How to review security advisory coverage applications → , What to cover in an application review → , and Drupal.org security advisory coverage application workflow → .
While this application is open, only the user who opened the application can make commits to the project used for the application.
Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.
- Status changed to Needs work
almost 2 years ago 5:53pm 27 January 2023 - 🇮🇳India vishal.kadam Mumbai
It seems you have missed working on the coding standards. You can use the PHPCS tool for checking and resolving issues.
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml eth_fundraiser/
FILE: eth_fundraiser/config/schema/eth_fundraiser.schema.yml
--------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------
34 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------FILE: eth_fundraiser/css/eth_fundraiser-admin.css
---------------------------------------------------------------------------------
FOUND 14 ERRORS AFFECTING 10 LINES
---------------------------------------------------------------------------------
22 | ERROR | [x] Expected 1 space after colon in style definition; 0 found
27 | ERROR | [x] Expected 1 space after colon in style definition; 0 found
31 | ERROR | [x] Expected 1 space after colon in style definition; 0 found
36 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
36 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
37 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
37 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
38 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
38 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
39 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
39 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
40 | ERROR | [x] Blank lines are not allowed in class definitions
45 | ERROR | [x] Whitespace found at end of line
52 | ERROR | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------------
PHPCBF CAN FIX THE 14 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------FILE: eth_fundraiser/eth_fundraiser.module
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
13 | ERROR | [x] There must be no blank lines after the function comment
47 | ERROR | [x] Expected 1 space after IF keyword; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------FILE: eth_fundraiser/eth_fundraiser.routing.yml
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------
7 | WARNING | The administration page callback should probably use "administer site configuration" - which implies the user can change something - rather than "access
| | administration pages" which is about viewing but not changing configurations.
------------------------------------------------------------------------------------------------------------------------------------------------------------------------FILE: eth_fundraiser/README.md
------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
------------------------------------------------------------------------
22 | WARNING | [ ] Line exceeds 80 characters; contains 148 characters
26 | ERROR | [x] Expected 1 newline at end of file; 2 found
------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------FILE: eth_fundraiser/src/Form/EthFundraiserForm.php
---------------------------------------------------------------------------------------------------------------------------------------------
FOUND 25 ERRORS AND 12 WARNINGS AFFECTING 34 LINES
---------------------------------------------------------------------------------------------------------------------------------------------
11 | WARNING | [ ] The class short comment should describe what the class does and not simply repeat the class name
51 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
51 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 1
63 | WARNING | [x] A comma should follow the last multiline array item. Found: '
80 | ERROR | [ ] The array declaration extends to column 136 (the limit is 80). The array content should be split up over multiple lines
80 | ERROR | [x] Expected one space after the comma, 0 found
80 | ERROR | [x] Expected one space after the comma, 0 found
97 | WARNING | [x] A comma should follow the last multiline array item. Found: '
99 | ERROR | [x] Functions must not contain multiple empty lines in a row; found 3 empty lines
100 | ERROR | [x] Whitespace found at end of line
101 | ERROR | [x] Whitespace found at end of line
106 | WARNING | [x] A comma should follow the last multiline array item. Found: '
121 | WARNING | [x] A comma should follow the last multiline array item. Found: )
129 | WARNING | [x] A comma should follow the last multiline array item. Found: )
143 | WARNING | [x] A comma should follow the last multiline array item. Found: '
144 | ERROR | [x] Whitespace found at end of line
149 | WARNING | [x] A comma should follow the last multiline array item. Found: '
175 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 5
176 | ERROR | [x] Array indentation error, expected 7 spaces but found 6
177 | ERROR | [x] Array indentation error, expected 7 spaces but found 6
178 | ERROR | [x] Array indentation error, expected 7 spaces but found 6
179 | ERROR | [x] Array indentation error, expected 7 spaces but found 6
180 | ERROR | [x] Array indentation error, expected 7 spaces but found 6
181 | ERROR | [x] Array closing indentation error, expected 5 spaces but found 4
186 | WARNING | [x] A comma should follow the last multiline array item. Found: '
187 | ERROR | [x] Whitespace found at end of line
192 | WARNING | [x] A comma should follow the last multiline array item. Found: '
193 | ERROR | [x] Whitespace found at end of line
194 | ERROR | [x] Whitespace found at end of line
201 | WARNING | [x] A comma should follow the last multiline array item. Found:'
203 | ERROR | [x] Whitespace found at end of line
216 | ERROR | [x] Whitespace found at end of line
217 | ERROR | [x] Whitespace found at end of line
219 | ERROR | [x] Whitespace found at end of line
225 | ERROR | [x] Whitespace found at end of line
230 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
233 | ERROR | [x] Expected 1 newline at end of file; 2 found
---------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 34 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------------------------------------FILE: eth_fundraiser/src/Plugin/Block/EthFundraiserBlock.php
--------------------------------------------------------------------------------------------
FOUND 9 ERRORS AFFECTING 8 LINES
--------------------------------------------------------------------------------------------
8 | ERROR | [x] There must be one blank line after the last USE statement; 0 found;
16 | ERROR | [x] There must be exactly one newline after the class comment
34 | ERROR | [x] Whitespace found at end of line
35 | ERROR | [x] Missing function doc comment
49 | ERROR | [x] Empty array declaration must have no space between the parentheses
62 | ERROR | [x] Short array syntax must be used to define arrays
79 | ERROR | [x] Expected 1 blank line after function; 0 found
80 | ERROR | [x] Expected 1 newline at end of file; 2 found
80 | ERROR | [x] The closing brace for the class must have an empty line before it
--------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 9 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------- - 🇮🇳India akshay.singh Noida
Usually, after reviewing a project, we allow the developer to opt projects into security advisory coverage.
This project is too small for us and it doesn't contain enough PHP code to really assess your skills as developer.
Have you made any other contributions that we could instead review?
Thank you for the quick response. I'll review and make the corrections listed above. I have worked on a similar project which is comparable in code and size. https://www.drupal.org/project/confection →
- 🇮🇹Italy apaderno Brescia, 🇮🇹
Although most of the code is for showing a form, there is enough code to review. The code would be more, if the form would avoid to use HTML markup as it does.
- 🇮🇳India jitesh_1 Jaipur
Hello @martelus,
Remember to change the status to Needs review when the project is ready for review.
- Status changed to Needs review
almost 2 years ago 9:31pm 3 February 2023 - 🇮🇳India vishal.kadam Mumbai
Hello @martelus,
I have reviewed the changes, and look fine to me.
Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.
Thanks
- Status changed to Needs work
almost 2 years ago 8:24pm 4 February 2023 - 🇮🇹Italy apaderno Brescia, 🇮🇹
- The following points are just a start and don't necessarily encompass all of the changes that may be necessary
- A specific point may just be an example and may apply in other places
- A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance
The form to configure a block is returned from its
blockForm()
method, whose purpose is returning the configuration form elements specific to the block plugin. There is no need to use a separate form class.src/Plugin/Block/EthFundraiserBlock.php
/** * {@inheritdoc} */ public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactory $config) { parent::__construct($configuration, $plugin_id, $plugin_definition); $this->config = $config; }
A class constructor does not uses
{@inheritdoc}
as documentation comment. A plugin configuration is passed in$configuration
; there is no need to use configuration objects. I'll make those changes now and look for other instances where it may apply. Thank you apaderno.
- Status changed to Needs review
almost 2 years ago 6:41pm 5 February 2023 - 🇮🇹Italy apaderno Brescia, 🇮🇹
$form['markup1'] = [ '#type' => 'markup', '#markup' => ' <div class="btn-group"> <a class="button js-form-submit form-submit" href="https://martelus.club" target="_blank" class="btn-martelus-club">Get Club Access</a> <a class="button js-form-submit form-submit" href="https://martelus.net" target="_blank" class="btn-martelus-io">Create an Account</a> </div> <p class="welcome-message"> The Eth Fundraiser Component allows you to raise funds through the ethereum blockchain. It also allows you to customize the component to match your site theme.<br> If you find that you need additional functionality there is a club member version available which offers multiple currencies, chain selection, token selection, additional wallet detection, installation assistance, technical support and more. </p> ', ];
$form['markup8'] = [ '#type' => 'markup', '#markup' => ' <div> <h3>Attention</h3> <p>Remember after saving your changes to clear cache.</p> </div>', ];
Both the messages must be translatable.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:
- Dries → ' post on Responsible maintainers
- Best practices for creating and maintaining projects →
- Maintaining a drupal.org project with Git →
- Commit messages - providing history and credit →
- Release naming conventions → .
- Helping maintainers in the issue queues →
You can find more contributors chatting on the Slack → #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 reviewers.
- Assigned to apaderno
- Status changed to Fixed
almost 2 years ago 8:57am 6 February 2023 Automatically closed - issue fixed for 2 weeks with no activity.