[D7] Alert

Alert module Review

1. Run coder module and test it once.

alert.module
Line 21: There should be no trailing spaces
Line 24: There should be no trailing spaces
Line 28: in most cases, replace the string function with the drupal_ equivalent string functions
if (substr($key, -9) == '_operator') {
Line 32: There should be no trailing spaces
Line 39: There should be no trailing spaces
Line 96: There should be no trailing spaces
Line 114: else statements should begin on a new line
} else {
Line 117: There should be no trailing spaces
Line 120: There should be no trailing spaces
Line 126: There should be no trailing spaces
Line 130: else statements should begin on a new line
} else {
Line 134: There should be no trailing spaces
Line 143: There should be no trailing spaces
Line 156: There should be no trailing spaces
Line 190: in most cases, replace the string function with the drupal_ equivalent string functions
if (substr($key, 0, 2) != 'a_') {
Line 194: else statements should begin on a new line
} else if (is_array($alert)) {
Line 194: Use "elseif" in place of "else if"
} else if (is_array($alert)) {

alert.install
Line 87: There should be no trailing spaces

You are working on master branch. Please create a branch with name 7.x-1.x
please refer to this for release naming conventions http://drupal.org/node/1015226

alert.module

1. use drupal_substr function instead of substr
2. Line 100, use a variable which is more meaningful instead of a_id. I assume you are referring this to alert_id
3. Line 123, remove unwanted spaces for assignment.

alert.rules.inc
1. all functions should have doxygen comments

README file is missing.

I like the idea of your module. I almost can't believe there is no other module for this? If not: Good work, once you fix the errors already stated.

Also I would encourage you to backport to D6.

Please see the following for branching

Release naming conventions

http://drupal.org/documentation/git

You can either use the master branch, or not use it.

If you use it, the code in there will be relevant and someone downloading the source code from git will run into the current working tree. If you don't use it, then the master branch needs to be empty and not have any code in there to confuse people. Just a readme telling people what's what.

The main reason not many projects use the master branch is that d.o will not allow you to create releases from the master branch, only something like the 6.x, 7.x or 8.x branch and even then, only from a tag on the branch.

A normal branch layout for a project would look something like this: (but not necessarily since you can do anything you want so long as the releases have the right branch)

master
- only the README.txt saying where things are
6.x
- full of files for the 6 release
- 7.x branch off the 6.x where you ported the module from 6.x to 7.x

Another might have all the branches starting at the master initial commit from developing the 6.x and 7.x at the same time, thus they have no common history. In your case having a 6.x branch off the 7.x just means that's the flow of the code. You could have started with an 8.x release if you really wanted to :P They're give version revision histories. You could branch them all off the "pigs in space" branch.

Don't forget you can also branch off for experiments, creating a patch for an issue, because you felt like it, and then merge things back to the parent branch when you're done, or just throw it away. You would normally not push these to the remote repository though. Only if you were working on a branch with another person and wanted to share code between yourselves easily.

I didn't look at the code last time, just that you needed some direction on the git issues. Here's a look at the code and function.

git commit messages

Please review commit messages - providing history. There's a common format to allow ease of following what is going on in each commit, which include who was responsible for the change, any issue involved, and the normal informational message.

seems like an API

For anything aimed specifically at developers, it's quite nice to include a [module].api.php file which doesn't actually do anything but provide examples on what the important functions in the module do. See Module documentation guidelines

missing newline characters

All text files should end in a single newline (\n). This avoids the verbose "\ No newline at end of file" patch warning and makes patches easier to read since it's clearer what is being changed when lines are added to the end of a file. See Coding standards

prefixing with a_

This is a bit strange. You have full control over what goes into the database, simply don't use names that conflict in the first place. It's simply adding complexity when it's not needed. What are the conflicting potential reserved word in this module?

alert_save

documentation does not match expectation - @param $alert says it's an object, the function can only accept an array, and then converts it to an object. For what reason? If it remains an array, you can use array_merge instead of the for loop and drupal_write_record will accept it as an array.

saved forever

Are all these messages saved forever, or is there some kind of cleanup happening (I can't see any)? Some sites go a little message mad, so saving every message the site has ever set using this method is going to add up to an incredibly large stockpile. There also doesn't seem to be any kind of repeat message filtering? If I set 10 million messages in one request, all 10 million get saved and and only then is the repeat checked by dsm() - ie. the $repeat flag is not working quite right.

alert_load

More of a comment. Amazing amount of complexity built into an overloaded $args value, when a simple default value second param would do. eg $condition = array(), or a multi level array eg pass in the code below No benefit from the complexity and it makes it much harder to understand what is going on.

alert_load(array(
  'uid' => array(
    'value' => 'blah',
    'operator' => '=',
  ),
));

or

alert_load(array(
  array(
    'column' => 'uid',
    'value' => 'blah',
    'operator' => '=',
  ),
));

That reduces it down to a single line of code instead of 10 with string manipulation.

function names

hook_load and hook_save are .. hooks. Usually for node modules. Unless you're treating these things as node, it might be a good idea to move out of this namespace to avoid confusion, especially if someone has made a local module with their own alert node type module.

hook_init

If it ever comes to it ... If you added caching to the user messages, you could allow people using this module on a huge number of user site to take advantage of their caching engine instead of making one extra query to the database regardless of the messages. ie. they could hold them in memcached instead of the database.

module name

alert doesn't seem specific enough to convey what this module does ... how about drupal user messages?

readme and project page

Both of these are a bit sparse, and there is not even any documentation on how to use this module anywhere. Please read Best practices for creating and maintaining projects.

module name

Simple string search and replace will do it. The number and range of options for ways and programs for doing this is longer than the accepted input length for a comment. Since this is a user specific dsm, how about User Drupal Set Message or userdsm.

alert_load, alert_save, function names

I think these are all very closely related. The hook_load you're doing here doesn't match the interface for hook_load at all. It could be overloaded to work but it is generally setup so that it takes a single parameter which is the id for the item being loaded. You wouldn't navigate through them one at a time like you would nodes. That's what it is primarily for - loading things that people look at, or require one of when loading a page. eg %user, %node, anything else with a single thing. Perhaps it could be adjusted to load all the messages for a user by feeding the uid to the %alert param. But again, these messages turn up when a user loads any page, not a page full of alerts. Although, there's a possibility.
To remove all the of strangeness in the alert_load, I would pass full conditions as rows. Anything to take out the special string handling. eg.

// Load for uid = 1, status = USERDSM_ACTIVE, reference LIKE 'pickles%'
// The ones that use = just pass the args, anything else needs the value to be an array
$result = alert_load(array(
  array('uid' => 1),
  array('status' => USERDSM_ACTIVE),
  array('reference' => array(
    'value' => 'pickles%',
    'op' => 'LIKE',
  )),
));

Which, one all the cruft is removed would be handled by (no string handling)

$query = db_select('alert', 'a')
    ->fields('a');
foreach ($args as $key => $value) {
  if (isset($value['op'])) {
    $query->condition($key, $value['value'], $value['op']);
  }
  else {
    $query->condition($key, $key);
  }
}
return $query->execute()->fetchAll();

The isset($value['op']) instead of is_array() means that an array of things can be passed for an IN operation without specifying it.
Of course, this format and the previous one are both completely useless in the hook_load name since the auto loading can't build a query array like this, just provide a single term.

prefixing with a_

The only one that is in there and causing issues is "repeat" which I'm sure could be renamed to something more local (msgrepeat), which would then save you having to do the text prefixing and greatly simply things. The ones you have listed are merely in other tables and should not restrict you - if fact, calling them the same thing is normal practice for making sure that people know that they are foreign keys. When you see "uid" in a table, you know instantly that it's probably meant to be the numerical user id from the users table.
Your status_auto field could be combined into a flag field that held all the behaviours for the field. eg USERDSM_DELETE_AFTER_DISPLAY, USERDSM_DELETE_MANUALLY etc. rather than setting up an entire fields for delete_auto, status_auto, pickles_auto.

magic values

Speaking of defined values, all of the 1,2,3 numbers you have around the place should be defined rather than simply numbers. This makes understanding what is going on easier, and reduces the chance of future screw up by not getting the number right, or changing the number.

@jessepinho has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

Could we mark this as postponed until you got the time to work on it?

No answer, so I mark as postponed until you got the time to work on it.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Duplicate of #1799252: [D7] Admin Toggle.