[D7] Site Status Message

Thanks SkidNCrashwell, nice module.
You have a link field for more information page, but it doesn't print the link.
I think you should pass this link through url() function in theme/site-status-message.tpl.php

Hello, and welcome!
I found somewhat similar module here: https://drupal.org/project/topbar_msg
I however think your module is more straight forward and simple use.
There are some issues reported by ventral review. It looks like your IDE.editor does not follow line ending standards.

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.

SkidNCrashwell, thanks for you contribution, keep up the good work!
The idea of you module is great and the code looks good.
There's just one issue related to modules on drupal.org - your module is too small.

Here are some reasons to that:
https://drupal.org/node/1587704
2.3 Ensure your project contains a minimum of handwritten code.
We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed, that means we can't make sure you're able to write secure code and use Drupal's APIs correctly.
But we can make the module a full-project manually.

The reason for this, is that when your projects is reviewd and approved you are granted possiblity to transfer the sandbox to a full project and create full projects without approval in future. That's why contributors are asked to show more code in their reviewed modules to help them out with coding standards, best practices and writing high quiality code. Your module is too small to make a good review to approve your account status.

https://drupal.org/node/1187664
Module 'Foo' is just too complex so I have written a much simpler version.
This is just straight duplication of functionality and applications like this will almost certainly be declined. It is worth remembering that many modules start out simple also and, over time, grow into more complex systems...

Please consider contributing to Toolbar Messages or other simular modules to make it better or simplier to use.

This module seems a nice piece of functionallity, so if you still think your module is going to head in a different direction than similar modules - we could make it full project manually.
Thanks for understanding and a great job!

> Please consider contributing to Toolbar Messages or other simular modules to make it better or simplier to use.

https://drupal.org/project/topbar_msg is quite heavy duty. It's clearly something that's meant for users who want ongoing site messages which change over time.

For a site owner who just wants a one-off message that says, 'The site is going down for updates tomorrow', having to have features, strongarm, and a custom node type to hold the messages is quite a lot.

I would say if this module is small and lightweight and clean and satisfies a single use case, it has a definite case for existing.

I personally like this module. It's very lightweight and I believe the use-case is there for a very simple banner message. Oddly enough I was thinking when the banner went up on D.O that I too should do that when I plan on putting the site down for maintenance. I send out emails but not everyone reads that and when they do hit the maintenance page they assume something has actually gone wrong. This might be a small module but it quickly addresses a problem for anyone running a site and requires about 120 seconds to download, install, and setup.

Downloaded this module into my dev environment for my site and was able to install with not hassles. Code is short and clean. It uses core themeing functions so if someone wanted to override them they could with their installed theme.

The suggestion about the other module, while it will do the same function it's use case was to address all messages. With flexibility often time comes complication.

As a person running a very complicate eCommerce site built on Drupal Commerce, I would choose this small module over the heavier topbar module - and I think I will!

https://drupal.org/project/site_banner

This sounds like a feature that should live in the existing site_banner project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the site_banner 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.

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

I hadn't found site_banner when I did a search.

site_banner is lightweight in terms of what it needs to set up and install, but the actual code is rather over-engineered, the form stuff in particular: http://drupalcode.org/project/site_banner.git/blob/refs/heads/7.x-1.x:/s...

IMO it would need a good clean-up!

Hey go check out this thread! https://groups.drupal.org/node/374478

The are currently discussion ongoing about how to improve the rules and limits regarding module size: https://groups.drupal.org/node/195848

As far as the minimum size goes, it appears that it is not a hard and fast rule, but requires taking a view on module functionality/complexity.

This project application provides some of the best examples of why the current review process is broken. Too many of these Drupal 'standards' are completely arbitrary and serve no useful purpose. Proper punctuation on comments? Extra spaces at the end of lines? No camel notation? Unix only line endings? 80 char maximum? 120 line minimum? Puh-leese. These rules create more problems than they solve, do nothing for performance and provide little help in determining the most important factor of all - does the code work like it is supposed too?

Time for a metaphor. SkidNCrashwell has built a better mousetrap. It has not been over-engineered over time to catch racoons or snakes or all other kinds of rodents. It's designed to catch mice -- that is all. It is simple and low cost.

SkidNCraswell has eloquently answered all questions regarding his reasons for creating this module. It has been praised by others and set to RTBC status. I have read all the posts here and I am satisfied of his competence in Drupal programming.

In fact I will probably use this module in the future instead of what is currently available for reasons that should be quite clear.

Almost everyone who goes through this process has attempted it because he/she could NOT find an existing module that did the job, or the ones on offer did the job so poorly that there had to be a better way. And there often is a better way.

The 'duplicate module' argument for refusing promotion in this process is often totally invalid. So is the 'collaboration-not-competition' argument.

This is my first project review. Thankfully I don't have to participate in the Review Bonus airmiles program now and, to be honest, I never did. I spent way too much reviewing existing contributed modules that didn't work. I think others do too, which is one reason the project queue continues to be a problem.

Setting this back to RTBC. Here is hoping that SkidNCrashwell does not have to wait a whole month before being promoted and that he/she is not afraid to be honest (or even critical) about this review process.

I agree that there are lots of problems with the review process, and that certainly in this case the minimum line limit is not helpful at all.

However:

> Proper punctuation on comments?

I don't think it's unreasonable to expect comments to be written correctly.

> Extra spaces at the end of lines?

Many people use text editors that automatically trim whitespace. If a file has whitespace at the end of lines, then patches get noisy because they keep changing it. The best fix is to never have whitespace. It's not a big job -- even editors that don't to it automatically will have a command to do it.

> No camel notation?

We have coding standards because uniformity of code helps in readability and thus understanding.

> Unix only line endings?

Same as for the the whitespace, but worse.

> 80 char maximum?

I find this one annoying, but again there are good reasons for it.

> do nothing for performance and provide little help in determining the most important factor of all - does the code work like it is supposed too?

These rules do a lot for maintainability and readability of code, and that is important too. I see too much code that is written slap-dash by coders who don't stop to consider who might have to read it after they're done with it. If code is released on d.org, it's shared with the community, and it has to be to a standard that makes it usable by all.

Given the discussion above, I've had a look at this module.

There is a minor issue whereby the message bar overlays the browser's scrollbar in overlay mode, pehaps due to the z-index: 9999; in the CSS file. This also overlaps dropdown menus from the admin_menu module which is widely used.

I also wonder if making the status bar/message bar sticky (or as an option) is possible, as currently it scrolls (this point is just a suggestion but would increase it's functionality/options).

You could have isolated this modules settings in its own config form and removed it from the main site administration section. This would enable you to include a hook_permission allowing administrators to give access to this option for other non-admin/partial-admin users, for example for announcing special offers, events etc. (again this would have increased it's scope)

A more important issue is that the read more link is not appearing for me despite installing it on two separate clean sites. The link is showing in the database, however it is not being rendered correctly with hook_page_build using the array of 2 variables you have set there.

Setting back to "NW" temporarily.

Thanks for making the changes.

I never use the Overlay module (it's horrible)

.. which is beside the point as 'overlay' is enabled by default. The point being that your module should behave correctly 'out of the box' on standard installations.

there's nothing to stop someone overriding the CSS and putting #site-status { position: fixed; } in their own CSS file.

... except if they aren't a developer. You are probably aware that one of the purposes of reviewing modules is to ensure that it is usable by people of at all levels, and we can't assume that a user will know anything about CSS, or even have access to edit theme files.

Besides the correction needed, my suggestions were put really in the context of what you could have done to easily enhance this module when faced with remarks about its length and corresponding feature set. Already, your new validation function, although a standard check against form inputs, has served to add to its overall functionality. But thanks for being receptive to a reviewers point of view.

> You are probably aware that one of the purposes of reviewing modules is to ensure that it is usable by people of at all levels, and we can't assume that a user will know anything about CSS, or even have access to edit theme files.

I think if you're installing a module to stick a status message at the top of the site, then you probably also have access to the theme.

This is definitely something that should be left to the project's issue queue as a feature request, but FWIW I definitely agree with it being a 'won't fix'. There's no need to add options like this to a simple module with a single purpose.

I believe the module is vulnerable to a cross-site-scripting vulnerability.

The variables should be filtered on output using something like filter_xss or filter_xss_admin. See these resources for more information.

Thanks, SkidNCrashwell. I wasn't on a machine where I could test the xss - sorry for posting before confirming it. Somehow my eyes glossed over the check_plain.

So, check_plain would have prevented an xss vulnerability, but I think filter_xss makes more sense in this case because it will allow through things like images or em or strong tags which an admin is likely to want to do.

Removing the "PAReview: Security" tag since that wasn't a security problem.

Setting to RTBC because it's been through enough reviews I do believe.

-=[ Review of this application ]=-

RTBC from me as well.

I think the author has pointed out enough differences between this and other existing modules. Topbar Messages is overkill for something like this. The module length is sufficient. I did not find any blocking issues during a manual review.

I'll leave this open for objections for another week, and then fix the issue if there are no complaints.

-=[ Responses to previous posters ]=-

I agree with joachim 100% on code style. We share code in Drupal between hundreds of thousands of users, this makes code style important. In this review process we ask you to try out Drupal's standard style once, after that you're free to code however you like.

A duplication check is part of the review process. I view these as a request to the maintainer to explain the differences, meaning it's not an application blocker. It does not prevent promotion, unless the maintainer cannot show any differences. So it's just a question. The answer often becomes a useful part of the project page.

"... wait a whole month before being promoted" - That's pretty typical for a review. If a maintainer wants to get through more quickly, we're discussing some ways to accomplish that here: https://groups.drupal.org/node/383423.

I think there are some points worth debating and we welcome your opinions in the Code Review group, where the review process is defined and improved.

I take exception to what joachim said. The quotes below are from post #14.

> Proper punctuation on comments?

I don't think it's unreasonable to expect comments to be written correctly.

What's next? The Queen's English? No dangling participles? Improper use of declining numerals? (Uh, sorry, Farsi is right out!)

You do realize that an easy solution to this is no comments at all, or as few comments as possible? Yeah, that should help work.

> Extra spaces at the end of lines?

Many people use text editors that automatically trim whitespace. If a file has whitespace at the end of lines, then patches get noisy because they keep changing it. The best fix is to never have whitespace. It's not a big job -- even editors that don't to it automatically will have a command to do it.

I have used a lot a lot of fine editors in my time. Not one automatically deleted anything. I do recall those damned Excel spreadsheets that prompted one to "save the changes" when absolutely no changes were made.

This "rule" has nothing to do with proper functioning, performance or cost. It's all about political correctness. Patches that get noisy? Is that really a problem?

> Unix only line endings?

Same as for the the whitespace, but worse.

That is NOT an answer. Most good editors handle variations of line endings without fuss using their default settings. So do source code repositories. Git certainly does.

Then along comes Drupal "coding" standards. Those with legacy code developed on outlier systems now have to enable obscure and previously unnecessary editor options to save in the correct OS format. It also helps to create a custom .gitattributes file because the git default may not work so well.

One can use a regexp to kill those orphaned space characters if your editor does not have that specific command (or the preferred automatic AK47 option).

Then, after all is almost correctly reconfigured, please don't get annoyed when every legacy file needs to be added again -- for no apparent reason (using most visual tools); and only the very weakest of rationalizations from a risk management perspective.

And how about that code review module checking for linux line endings regardless of the native OS it happens to be running on? Let's talk about noise.

This may not be the right forum to debate this topic. But this is just a warm up round.

There are now 3 RTBC endorsements here, two if you don't count mine (Hey -- I can live with that).

I have no doubt that ksheirer also reviewed it with his eagle eyes and will promote it shortly as promised. The promise came over a month after the latest RTBC and no one has objected. ksheirer even did the review on xmas day.

Of course there is always the possibility that another star chamber member will overrule without explanation, so one has to be cautious.

A one month cooling off period? We are NOT here attempting to buy a gun. All we want to do is contribute a module.

How about a one week automatic trigger?

Thanks for your contribution, SkidNCrashwell!

I updated your account to let you 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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, 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 to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Crafted, Curated, Contributed.