- Issue created by @drale01
- 🇮🇳India vishal.kadam Mumbai
Thank 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.
- If you have not done it yet, you should run
phpcs --standard=Drupal,DrupalPractice
on the project, which alone fixes most of what reviewers would report. - For the time this application is open, only your commits are allowed.
- The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
- We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.
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.
- It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
- Reviewers should show the output of a CLI tool → only once per application.
- It may be best to have the applicant fix things before further review.
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 → .
- If you have not done it yet, you should run
- Issue was unassigned.
- Status changed to Needs work
about 1 year ago 10:41am 8 December 2023 - 🇮🇳India hemant-gupta New Delhi
In 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... →
- 🇫🇷France nikral
In each folder there are these files:
- .gitkeep
- DS_Store
- gitignore
Can you delete them please? - 🇮🇳India vishal.kadam Mumbai
.gitignore files are allowed. There is no need to delete them.
- 🇫🇷France nikral
Yes but, I think that if it is not necessary, it should be deleted. it's a standalone module.
- 🇧🇦Bosnia and Herzegovina drale01 Sarajevo
I have done everything you asked me to do. Thank you all.
- Status changed to Needs review
about 1 year ago 5:01pm 12 December 2023 - Status changed to Needs work
about 1 year ago 9:04pm 16 December 2023 - 🇩🇪Germany simonbaese Berlin
This is a partial review! There are more issues, but some general problems should be addressed first.
- There are still many coding standards violations. For example, the indentation and camelCase vs. snake_case should be fixed.
- The
CommentOnTopController
extends theControllerBase
. Therefore, the injection of the entity type manager and messenger is not necessary. Just use$this->entityTypeManager()
and$this->messenger()
. - The field type
stick_comment_on_top
might be overkill for this purpose. Why not just use a boolean field? Also, it might be more suitable to add a base field for theComment
entity, rather than attaching a field. Has to be handled properly during install and uninstall though. - I may not understand correctly what is happening there, but the
comment_on_top_preprocess_node()
hook feels out of place. Maybe the module should not be responsible for altering the comment form. - The actual sorting of the comments happens in a view hook. What would happen if the comments are displayed through a different approach? I am not sure how to solve this, but maybe one could provide a service to sort the comments properly. Or maybe a custom storage could be implemented to sort the comments on load.
- Also, the hook
comment_on_top_views_pre_render()
does not distinguish which view we are dealing with.
- 🇧🇦Bosnia and Herzegovina drale01 Sarajevo
@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. - Status changed to Needs review
12 months ago 12:34pm 28 December 2023 - Assigned to apaderno
- Status changed to RTBC
10 months ago 3:51pm 18 February 2024 - 🇮🇹Italy apaderno Brescia, 🇮🇹
Thank 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:
- Dries → ' post on Responsible maintainers
- Maintainership →
- Git version control system →
- Issue procedures and etiquette →
- Maintaining and responding to issues for a project →
- Release naming conventions → .
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.
- Status changed to Fixed
10 months ago 3:51pm 18 February 2024 - 🇧🇦Bosnia and Herzegovina drale01 Sarajevo
Great!
Thanks @apaderno.I changed to security advisory coverage.
Soon it will be version 2.0.0
Automatically closed - issue fixed for 2 weeks with no activity.