Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
10 Oct 2021 at 19:51 UTC
Updated:
17 Nov 2021 at 08:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
avpadernoThank you for applying!
Only reviews for other Drupal.org security advisory coverage applications are relevant for us.
Comment #3
tim-dielsAh sorry didn't know that, thanks for that feedback. Is there any more information you need to help in the review?
Comment #4
avpadernoThat property is already defined from the parent class; there is no need to define it again.
What is supposed to show, that route? It seem the route for the settings form, but it doesn't have any associated form.
Classes that implement
ContainerFactoryPluginInterfacedon't use\Drupal.Comment #5
rajeshreeputraAdded PAReview link: http://pareview.net/r/488
Comment #6
tim-diels@apaderno thank you very much for the review, I really appreciate the time you took to analyse the module.
@Rajeshreeputra thanks for the PAReview link, that service didn't work a couple of weeks ago.
I'll do my best to answer with as much information as possible to help in understanding the module.
The route
nth_mobile.admin_configshows a list of sub routes like thenth_mobile_psms.settings_formfrom the submodule NTH Mobile PSMS and can have more sub routes when more modules or routes to current modules are added in the future. I added a screenshot displaying what is shown on the page.The raised issues are fixed with #3246568: Issues raised by the Drupal.org security advisory coverage applications in the 1.0.x-dev branch
Comment #7
marijan gudelj@tim-diels before I go fully in the review mode can you please help me with something ?
Does this payment gateway need the Commerce module or any other E-commerce module?
Because I cannot see how you log the payment statuses. Also without any e-commerce module I fail to see the use case here.
Comment #8
tim-diels@marijan This module does not need any commerce related modules. It provides webhooks with events to allow to interact with NTH Mobile. A developer needs to build logic using eventsubscribers. There is an example module explaining what someone needs to do. It is possible to use this in conjunction with commerce modules. But I was not in the need to make that for my use case. If one needs that, I can create a submodule for that or one can create a module on itself. I can always show you in private how I implemented this module for one of my clients.
Comment #9
avpadernoThank 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 #10
tim-dielsThank you very much for the approval!