Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Webform Analysis should follow Drupal's Coding Standards because it will make it easier for people understand the code and contribute patches.
Proposed resolution
Review and follow Coding Standards
Here is the automated code review.
Some suggestions...
- When in doubt look at core.
- Add API comments to everything
- Add \Drupal\webform_analysis\WebformAnalysisInterface and code to Interfaces
- Use Dependency injection when possible.
- Always use the webform_analysis namespace, you should change 'webform-admin.css' to 'webform_analysis.admin.css'
- Instead of using \Drupal\webform_analysis\Form\WebformAnalysisForm::getWebformIdFromRoute use \Drupal\webform\WebformRequest to get the current pages webform entity.
- Remove \Drupal\webform_analysis\WebformAnalysis::MODULE_NAME and just use the module name.
- \Drupal\webform_analysis\WebformAnalysis::__construct should be used for dependency injection
You might need to 'cast' the webform to an entity in your routes...
entity.webform.results_analysis:
path: '/admin/structure/webform/manage/{webform}/results/analysis'
defaults:
_form: '\Drupal\webform_analysis\Form\WebformAnalysisForm'
_title_callback: '\Drupal\webform_analysis\Form\WebformAnalysisForm::getTitle'
options:
parameters:
webform:
type: 'entity:webform'
requirements:
_permission: 'view any webform submission'
Issue fork webform_analysis-2925863
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #7
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedOkay, it seems very clear that you want to create an awesome module, so I will keep reviewing and creating tickets.
Comment #8
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@lbaran If you have questions or need me to contribute a patch to point you in the right direction, just ask.
Comment #11
lbaran CreditAttribution: lbaran commented@jrockowitz, I'll do that. Thank you for your help, support and advice.
Comment #13
lbaran CreditAttribution: lbaran commentedHello @jrockowitz,
I simplified my code, I use a hook_entity_type_build to add a form handler on Webform.
The route looks like this :
Tell me what you think.
Sincerely,
Comment #14
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedA few more things related to code standards and patterns (that I missed)...
To prevent namespacing issues...
Personally, I would merge \Drupal\webform_analysis\WebformAnalysis into \Drupal\webform_analysis\Form\WebformAnalysisForm which should make the code easier to understand. If the code was to get more complex then I would create an injected service for certain aspects, like rendering, data storage, etc... For example, if the rendering of the charts was an injected service someone could easily override it.
Comment #17
anabpvI will review this
Comment #18
anabpvUpdated the files with the help of phpcs and phpcbf, mainly:
- Added a new line at the end of the file webform_analysis.links.task.yml
- Added the missing member variable doc comments on several files
- Splitted the array up over multiple lines in file WebformAnalysisBlock.php
Overall, the affected files are:
- webform_analysis.links.task.yml
- WebformAnalysisBlock.php
- WebformAnalysis.php
- WebformAnalysisChart.php
- WebformAnalysisForm.php
Waiting for review.
Comment #20
tmaiochi CreditAttribution: tmaiochi at CI&T commentedI'll review this!
Comment #21
tmaiochi CreditAttribution: tmaiochi at CI&T commentedThe code fixed all messages from PHPCS, however I found this message when I ran phpcs
Looking deeper into this warning, I realized that in constructor had this code:
So it's already being done the dependency injection, but for some reason has this conditional calling Drupal. I think it's better to leave it at that, so I'm moving it to RTBC!
Comment #24
batigolix