[D7] Alert Message

1) Upon enabling this module I get the following errors:

Notice: Undefined variable: message in alert_message_process_html() (line 672 of /Users/freddybushboy/Sites/sandbox/sites/all/modules/custom/alert_message/alert_message.module).
Warning: Invalid argument supplied for foreach() in alert_message_process_html() (line 672 of /Users/freddybushboy/Sites/sandbox/sites/all/modules/custom/alert_message/alert_message.module).
Notice: Undefined variable: messages in alert_message_process_html() (line 681 of /Users/freddybushboy/Sites/sandbox/sites/all/modules/custom/alert_message/alert_message.module).
Notice: Undefined variable: color in alert_message_process_html() (line 681 of /Users/freddybushboy/Sites/sandbox/sites/all/modules/custom/alert_message/alert_message.module).
Notice: Undefined index: in alert_message_process_html() (line 681 of /Users/freddybushboy/Sites/sandbox/sites/all/modules/custom/alert_message/alert_message.module).
Notice: Undefined variable: border_color in alert_message_process_html() (line 681 of /Users/freddybushboy/Sites/sandbox/sites/all/modules/custom/alert_message/alert_message.module).
Notice: Undefined variable: background_color in alert_message_process_html() (line 681 of /Users/freddybushboy/Sites/sandbox/sites/all/modules/custom/alert_message/alert_message.module).

I first thought this was because no messages have yet been set but I still get these messages even after adding some tests.

Even when not getting these errors I still get:

Notice: Undefined variable: messages in alert_message_process_html() (line 679 of /Users/freddybushboy/Sites/sandbox/sites/all/modules/custom/alert_message/alert_message.module).

2) The module is missing a README.txt or README.md file

3) You should remove "version" from the alert_message.info file, it will be added by drupal.org packaging automatically.

4) Code sniffer has also found quite a few issues with your code in relation to Drupal standards - you can take a look here:

http://pareview.sh/pareview/httpgitdrupalorgsandboxirodriguez2172777git
In the link above there are some links to the documentation. Unless there's a good reason to not do it, you should fix all issues reported there. You can run http://pareview.sh/ as many times as you want

No problem :-)

The errors are now fixed for me.

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch.

Not sure why pareview is not picking up your readme file - it is possible it needs to be name in all caps. Try rename it to README.txt. Also sometimes things take a while to show up as fixed on pareview (changing over from the master branch is a good example of this).

Also, I think you should add a description to your main menu item in hook_menu - I don't believe this is a requirement necessarily but it would look better. Also, are you sure structure is the right section for this? I would maybe consider something less prominent like 'User Interface' or something. Again, up to you.

Duplication

What separates this module from Site Status Message?. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. You should get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

We 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.

Duplication

I have to agree with xqus (https://drupal.org/comment/8373635#comment-8373635) on this one.

From your response it really sounds like these are all features that could be patched into the "Site Status Message", since your module really just adds more configuration and functionality over the other module.

It's also worth noting that the User Notifications module seems similar as well. It is actively seeking a co-maintainer. It integrates with rules and the messaging API, so I would take a look to see if you can integrate your changes to this module and help with the maintenance.
https://drupal.org/project/user_notifications

Other similar modules:

https://drupal.org/project/alerts
https://drupal.org/project/alert_box

Also

Administrators can choose which pages the messages can display on with settings just like blocks visibility settings.

If you're replicating the block system's visibility settings, why not just use them? Your module could provide a block with the additional stylings/settings you've created, and you could have all the power of the block system for free.

Hi,

Duplication and abandoned modules is a big problem for users updating. If there is a module that aims to do the same, but lacks functionality the best thing for the community would be to contribute to the existing module.

If you feel your module solves the problem in a fundamentally different way, or the existing module developer don't want to accept your contribution that's fine.

We are aware that his process can seem a bit.. difficult, but most of the guidelines are in place for a reason.

If you still feel your module belongs here, set it back to needs review, and I will try to review it soon!

Please could you describe how this module differs from the other two I mentioned (or why collaboration wouldn't be suitable). They both look like they provide very similar functionality.

Also there are other solutions for the block admin page problem, such as the context module.

I'd like to reinstate what was said by xqus. Drupal is all about collaboration. It's great that you want to contribute, but it would be much more useful to the community if you can provide patches to existing modules rather than creating standalone new ones.
Please read https://drupal.org/node/23789.

@irodriguez

I have tested your module on pareview.sh and found 2 errors.
For detail check the link http://pareview.sh/pareview/httpgitdrupalorgsandboxirodriguez2172777git

Hi Ivan

Yes, it shows 2 errors on Pareview.
As per Drupal coding standards, each file should have an additional new line character at the end.

FILE: /var/www/drupal-7-pareview/pareview_temp/css/alert_message.css
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
57 | ERROR | Additional whitespace found at end of file
58 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Thanks
Raman

Minor coding standard errors are not application blockers, please do a real manual review of the code.

Hi,

I found some issues -

Documentation for the functions is not correct - You write 'Implements hook_title()' but the function is not a 'hook'.

In place of drupal_goto in 'hook_submit', you must use form_state['#redirect'].

If you want to render the html, you should use theme. 'alert_message_array_to_list' function is returning the HTML, you should make it a theme function and call it by using theme('xxx'). For this type of list you can use 'theme_item_list'.

Hi,

I found some issues -

Documentation for the functions is not correct - You write 'Implements hook_title()' but the function is not a 'hook'.

In place of drupal_goto in 'hook_submit', you must use form_state['#redirect'].

If you want to render the html, you should use theme. 'alert_message_array_to_list' function is returning the HTML, you should make it a theme function and call it by using theme('xxx'). For this type of list you can use 'theme_item_list'.

Hey irodriguez,

Excellent idea, your code is very tidy!

Just a couple of things i noticed in the code:

• in alert_message.module functions alert_message_add_form($form, $form_state) and others
  the $form_state variable should be a reference e.g. alert_message_add_form($form, &$form_state) (https://api.drupal.org/api/drupal/includes!form.inc/group/form_api/7)
• alert_message.module / function alert_message_add_form and others variables: $form['name'] and $form['settings']['pages'] don't have #title variables in them - this could well be by design, just pointing it out!
• There aren't a lot of comments in the functions describing what they are doing, its much easier to follow with inline comments

Just wondering why hook_uninstall is not implemented to clear up the schema?

Hi irodriguez,

First of all I would suggest you to add the link to git repo in the summary above to the appropriate git command to make it easier for reviewers i.e.
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/irodriguez/2172777.git alert_message.

Furthermore, could please explain how this module is different from "site_status_message" module, which also provides the similar functionality?

Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the video_upload issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

Thanks for your understanding.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.