Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
8 Dec 2023 at 09:33 UTC
Updated:
7 Mar 2024 at 14:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
vishal.kadamThank you for applying!
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.
The important notes are the following.
phpcs --standard=Drupal,DrupalPracticeon the project, which alone fixes most of what reviewers would report.To the reviewers
Please read How to review security advisory coverage applications, Application workflow, What to cover in an application review, and Tools to use for reviews.
The important notes are the following.
For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues.
Comment #3
vishal.kadamComment #4
hemant-gupta commentedIn project file "comment_on_top.info.yml", the key core_version_requirement cannot be used to restrict to core version before 8.7.7 and it would throw a parsing exception.
So, you need to replace that with "core_version_requirement to ^8.8 || ^9 || ^10" to fix.
Source- https://www.drupal.org/docs/develop/creating-modules/let-drupal-know-abo...
Comment #5
nikral commentedIn each folder there are these files:
- .gitkeep
- DS_Store
- gitignore
Can you delete them please?
Comment #6
vishal.kadam.gitignore files are allowed. There is no need to delete them.
Comment #7
nikral commentedYes but, I think that if it is not necessary, it should be deleted. it's a standalone module.
The gitignore file contains:
.DS_Store
.idea/
Comment #8
drale01 commentedI have done everything you asked me to do. Thank you all.
Comment #9
nikral commentedComment #10
simonbaeseThis is a partial review! There are more issues, but some general problems should be addressed first.
CommentOnTopControllerextends theControllerBase. Therefore, the injection of the entity type manager and messenger is not necessary. Just use$this->entityTypeManager()and$this->messenger().stick_comment_on_topmight be overkill for this purpose. Why not just use a boolean field? Also, it might be more suitable to add a base field for theCommententity, rather than attaching a field. Has to be handled properly during install and uninstall though.comment_on_top_preprocess_node()hook feels out of place. Maybe the module should not be responsible for altering the comment form.comment_on_top_views_pre_render()does not distinguish which view we are dealing with.Comment #11
drale01 commented@simonbaese I made changes you wrote on comment 10
1. Fixed
2. Fixed
3. Fixed - this is much simpler. Thanks!
4. This all principle removes default Drupal comments and the comment form as it utilizes views to display comments. The comment form for allowed content types is set through the comment_on_top_preprocess_node() hook.
5. I tried many different ways instead using views, changing weight, changing comment array sort,… But none of them affect sorting of comments in default Drupal comments display.
6. Fixed
7. Are you certain that I need to utilize dependency injections? All examples of DTT tests were employing static methods.
Comment #12
drale01 commentedComment #13
avpadernoThank you for your contribution!
I updated your account so you can now opt into security advisory coverage for any project you created and every project you will create.
These are some recommended readings to help you with maintainership:
You can find more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!
Thank you 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 also the dedicated reviewers as well.
Comment #14
avpadernoComment #15
drale01 commentedGreat!
Thanks @apaderno.
I changed to security advisory coverage.
Soon it will be version 2.0.0