Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
17 Jun 2016 at 08:38 UTC
Updated:
25 Oct 2016 at 18:04 UTC
Jump to comment: Most recent, Most recent file

Comments
Comment #2
sandeepguntaka commentedComment #3
PA robot commentedWe are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)
Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #4
Anonymous (not verified) commentedHi sandeepreddyg! First off, I think this is a great idea for a module. Below is a list of issues I came across when trying to use it.
Missing Folder Name in Git Url:
In the issue description where you put the Git clone command the folder name was missing from the end of the command. Because of that when I used the command the folder name was '2745699'. I've gone ahead and updated the description for you.
Add Module Descriptions:
It's best practice to make sure your module has a good description so that people know exactly what it does when installing it. You should go ahead and add a description to line 3 of your "maintenance_notifications.info.yml" file and line 3 of your "maintenance_notifications.links.menu.yml" file.
Add a Configuration Link:
Since the module has a configuration page you should add it to the "maintenance_notifications.info.yml" file using the "config". For your module I think you would add "config: maintenance_notifications.form".
Message Typos:
These are just small grammar issues that could be improved.
Unable to Save Settings:
When I tried to save my custom notification settings I got an error that said, "Below highlighted Fields are required". All of the fields are highlighted red even though I did fill them out. I've attached a screenshot here.
I hope this helps!
Comment #5
sandeepguntaka commentedThanks for reviewing @keeganhr. I ve commited the suggested changes.
Comment #6
sandeepguntaka commentedComment #7
sandeepguntaka commentedComment #8
visabhishek commentedHi sandeepreddyg,
Module looks good. Some suggestions are :
1: Follow the Readme template for the readme.txt https://www.drupal.org/node/2181737
2: Please add configuration and permission links in info file.
3: Add hook_help().
4: To find configure link is difficult So You can add a local.task for maintenance "base_route:admin/config/development/maintenance"
Comment #9
poojasharmaece commentedHi sandeepreddyg,
I tried to test your module , Its working fine only one functionality is not working which is
1: Not accepting multiple emails on configuration form, Please see the attachement
Comment #10
sandeepguntaka commentedThanks for the reviews @visabhishek and @poojasharmaece. The things are fixed and commited.
Comment #11
sandeepguntaka commentedComment #12
anavarreIn
maintenance_notifications.info.ymlyou don't need:In
maintenance_notifications.permissions.ymlYou can drop
description: 'Configration Maintenance Notification'completely as it doesn't bring any different help message than the permission title.Also, I think your permission machine name is wrong either in this file or in
maintenance_notifications.routing.ymlwhich has_permission: 'administer maintenance_notifications'In
maintenance_notifications.moduleIn
hook_help()You have a typo line 19:
maintennance->maintenanceYou can drop
default:altogether (line 22)Your parameters for
hook_form_FORM_ID_alter()are wrong (D7, not D8). See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Form%21fo...In
hook_mail(), you should not invokeDrupal\Component\Utility\SafeMarkupdirectly. Instead, having a use statement at the top of the file would be recommended. AlsoSafeMarkupis deprecated so you should consider replacing it.In
MaintenanceNotification.phpPerhaps you could replace
valid_email_address($maintenance_user)by\Drupal::service('email.validator')->isValid($maintenance_user)Also, in
submitForm(), you could simplify your code. E.g.Could become:
Comment #13
poojasharmaece commentedHi sandeepreddyg,
In updated code when i am putting my site in maintenance i am not getting any mail or messages. (I checked on Screen and logger both places.
And i am getting following error : "The website encountered an unexpected error. Please try again later."
Error log message is :
Error : Recoverable fatal error: Object of class Drupal\\Core\\Language\\Language could not be converted to string in /var/www/html/drupal-8/core/lib/Drupal/Core/StringTranslation/Translator/CustomStrings.php on line 37, referer: http://localhost/drupal-8/admin/config/development/maintenance
Please correct me , if i am doing something wrong.
Thanks
Comment #14
sandeepguntaka commented@poojasharmaece Thanks for review again, The issue is now fixed. Please have a check now. @anavarre I Thanks for the suggestions. I've implemented them now, please make some time to review.
Comment #15
poojasharmaece commentedI think we don't have any blockers now. So i am marking as RTBC.
Comment #16
anavarreThanks for the quick fixes, @sandeepreddyg.
One nitpick I have is you could
use Drupal\Core\Form\FormStateInterface;to invokeFormStateInterface $form_stateinstead of\Drupal\Core\Form\FormStateInterface $form_stateinhook_form_FORM_ID_alter()- Also, it's missing a whitespace between the 1st and 2nd parameters.Comment #17
sandeepguntaka commented@anavarre Thanks for reviewing again. I've added them to the code.
Comment #18
sandeepguntaka commentedComment #19
avpadernoI will update your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.
Here are some recommended readings to help with excellent maintainership:
You can find lots more contributors chatting on IRC in #drupal-contribute. 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.
Thanks go the dedicated reviewer(s) as well.
Comment #20
sandeepguntaka commentedThanks for the update @kiamlaluno