[D7] Mark as Read

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxprabeengiri2175185git

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.

Hey, I could get this module working :) and I can think of possibilities to use it in my projects.

I had problems updating a list which resulted in:
Warning: Missing argument 3 for mark_as_read_admin_add_edit_form_submit(), called in /includes/form.inc on line 1478 and defined in mark_as_read_admin_add_edit_form_submit() (line 129 of modules/mark_as_read/mark_as_read.admin.inc)
and
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'list_name' cannot be null: UPDATE {mark_as_read} SET list_name=:db_update_placeholder_0,

So in addition to the PA robot's result here are some hints for your module:

Remove unnecessary files

I found a eclipse settings, features.inc files

Master Branch

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. For additional resources please see the documentation about release naming conventions and creating a branch in git.

README.txt

Please take a moment to make your README.txt follow the guidelines for in-project documentation.

Cheers!

Setting the priority back to normal.
@Prabin, please make sure to read the guidelines for project applications here, specifically the Application Priority section.

Issues:

1. Change the Git link to git clone --branch 7.x-1.x http://git.drupal.org/sandbox/prabeen.giri/2175185.git mark_as_read instead of git clone --branch master prabeen.giri@git.drupal.org:sandbox/prabeen.giri/2175185.git mark_as_read

2. Follow Drupal Coding standards.

3. There is no need to write

drupal_uninstall_schema('mark_as_read_activity');
drupal_uninstall_schema('mark_as_read');

in .install file. Drupal 7 itself handles uninstallation of schema.

4. Change the name of the variables of the admin form to something like modulename_attribute_name, modulename_list_name etc so that it does not get conflicted with any other module variables.

5. Add ReadMe.txt file with your module.

6. Check all the issues at pareview.sh

For the second point have a look at the list of issues on
http://pareview.sh/pareview/httpgitdrupalorgsandboxprabeengiri2175185git

Please fix coder issues, below are list of coder issues

FILE: /var/www/drupal-7-pareview/pareview_temp/js/mark_as_read.api.js
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"
26 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/js/mark_as_read.js
--------------------------------------------------------------------------------
FOUND 10 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"
40 | ERROR | Whitespace found at end of line
42 | ERROR | There should be no white space after an opening "("
42 | ERROR | There should be no white space before a closing ")"
48 | ERROR | Whitespace found at end of line
85 | ERROR | Whitespace found at end of line
94 | ERROR | Functions must not contain multiple empty lines in a row; found
| | 2 empty lines
231 | ERROR | Whitespace found at end of line
267 | ERROR | Files must end in a single new line character
267 | ERROR | Additional whitespace found at end of file
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mark_as_read.admin.inc
--------------------------------------------------------------------------------
FOUND 16 ERROR(S) AND 1 WARNING(S) AFFECTING 14 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found
| | "\r\n"
73 | ERROR | Use XHTML style tags instead of
107 | ERROR | Spaces must be used to indent lines; tabs are not allowed
107 | ERROR | Whitespace found at end of line
108 | ERROR | Spaces must be used to indent lines; tabs are not allowed
109 | ERROR | Spaces must be used to indent lines; tabs are not allowed
117 | WARNING | A comma should follow the last multiline array item. Found: )
139 | ERROR | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
| | found "true"
140 | ERROR | Expected 1 space after "=>"; 2 found
151 | ERROR | Use XHTML style tags instead of
160 | ERROR | Use XHTML style tags instead of
197 | ERROR | Inline control structures are not allowed
201 | ERROR | Space found before comma in function call
201 | ERROR | Space before opening parenthesis of function call prohibited
221 | ERROR | No space before comment text; expected "// exisiting columns."
| | but found "//exisiting columns."
221 | ERROR | Inline comments must start with a capital letter
234 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mark_as_read.api.php
--------------------------------------------------------------------------------
FOUND 16 ERROR(S) AND 2 WARNING(S) AFFECTING 13 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found
| | "\r\n"
9 | ERROR | There must be an empty line before the parameter block
9 | ERROR | Doc comment for var $list_id. does not match actual variable
| | name $list_id at position 1
9 | ERROR | Missing parameter type at position 1
10 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
11 | ERROR | Missing parameter type at position 2
12 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 2
19 | WARNING | Line exceeds 80 characters; contains 111 characters
19 | ERROR | Function comment short description must be on a single line,
| | further text should be a separate paragraph
21 | ERROR | There must be an empty line before the parameter block
21 | ERROR | Missing parameter type at position 1
22 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
29 | ERROR | Function comment short description must end with a full stop
30 | ERROR | Invalid @return data type, expected bool but found boolean
31 | WARNING | Line exceeds 80 characters; contains 90 characters
31 | ERROR | Return comment indentation must be 2 additional spaces
36 | ERROR | Inline comments must start with a capital letter
37 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mark_as_read.class.inc
--------------------------------------------------------------------------------
FOUND 38 ERROR(S) AFFECTING 25 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"
4 | ERROR | The third line in the file doc comment must contain a description
| | and must not be indented
11 | ERROR | Whitespace found at end of line
12 | ERROR | Class property $is_new should use lowerCamel naming without
| | underscores
13 | ERROR | Whitespace found at end of line
14 | ERROR | Missing function doc comment
14 | ERROR | No scope modifier specified for function "__construct"
16 | ERROR | Spaces must be used to indent lines; tabs are not allowed
16 | ERROR | Line indented incorrectly; expected 6 spaces, found 5
17 | ERROR | Spaces must be used to indent lines; tabs are not allowed
17 | ERROR | Line indented incorrectly; expected 6 spaces, found 5
17 | ERROR | Whitespace found at end of line
18 | ERROR | Whitespace found at end of line
20 | ERROR | Whitespace found at end of line
23 | ERROR | There must be an empty line before the parameter block
23 | ERROR | Missing parameter type at position 1
23 | ERROR | Missing comment for param "$property" at position 1
24 | ERROR | Missing parameter type at position 2
24 | ERROR | Missing comment for param "$value" at position 2
26 | ERROR | Spaces must be used to indent lines; tabs are not allowed
26 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
26 | ERROR | No scope modifier specified for function "__set"
27 | ERROR | Spaces must be used to indent lines; tabs are not allowed
27 | ERROR | Line indented incorrectly; expected 4 spaces, found 3
28 | ERROR | Closing brace indented incorrectly; expected 1 spaces, found 2
29 | ERROR | Whitespace found at end of line
32 | ERROR | There must be an empty line before the parameter block
32 | ERROR | Missing parameter type at position 1
33 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
35 | ERROR | No scope modifier specified for function "__get"
41 | ERROR | Whitespace found at end of line
44 | ERROR | There must be an empty line before the parameter block
44 | ERROR | Missing parameter type at position 1
45 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
49 | ERROR | Expected 1 space before "=>"; 0 found
49 | ERROR | Expected 1 space before "=>"; 0 found
53 | ERROR | Whitespace found at end of line
65 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mark_as_read.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
10 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mark_as_read.install
--------------------------------------------------------------------------------
FOUND 51 ERROR(S) AND 1 WARNING(S) AFFECTING 38 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found
| | "\r\n"
12 | ERROR | Spaces must be used to indent lines; tabs are not allowed
12 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
13 | ERROR | Spaces must be used to indent lines; tabs are not allowed
14 | ERROR | Spaces must be used to indent lines; tabs are not allowed
15 | ERROR | Array indentation error, expected 3 spaces but found 4
31 | ERROR | Array indentation error, expected 3 spaces but found 4
32 | ERROR | Array indentation error, expected 3 spaces but found 4
33 | ERROR | Spaces must be used to indent lines; tabs are not allowed
33 | ERROR | Array indentation error, expected 6 spaces but found 5
34 | ERROR | Spaces must be used to indent lines; tabs are not allowed
35 | ERROR | Array indentation error, expected 7 spaces but found 8
36 | ERROR | Array indentation error, expected 7 spaces but found 8
37 | ERROR | Array closing indentation error, expected 5 spaces but found 6
51 | ERROR | Spaces must be used to indent lines; tabs are not allowed
51 | ERROR | Array indentation error, expected 8 spaces but found 7
56 | ERROR | Array indentation error, expected 3 spaces but found 4
57 | ERROR | Array indentation error, expected 3 spaces but found 4
58 | ERROR | Spaces must be used to indent lines; tabs are not allowed
58 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
61 | ERROR | Spaces must be used to indent lines; tabs are not allowed
61 | ERROR | Array indentation error, expected 4 spaces but found 3
63 | ERROR | Spaces must be used to indent lines; tabs are not allowed
63 | ERROR | Array indentation error, expected 6 spaces but found 5
64 | ERROR | Spaces must be used to indent lines; tabs are not allowed
65 | ERROR | Array indentation error, expected 7 spaces but found 8
66 | ERROR | Array indentation error, expected 7 spaces but found 8
67 | ERROR | Array indentation error, expected 6 spaces but found 4
67 | ERROR | Array closing indentation error, expected 5 spaces but found 4
68 | ERROR | Array indentation error, expected 6 spaces but found 4
69 | ERROR | Spaces must be used to indent lines; tabs are not allowed
69 | ERROR | Array indentation error, expected 6 spaces but found 5
73 | ERROR | Array indentation error, expected 6 spaces but found 4
74 | ERROR | Array indentation error, expected 6 spaces but found 4
75 | ERROR | Spaces must be used to indent lines; tabs are not allowed
75 | ERROR | Array indentation error, expected 6 spaces but found 5
78 | ERROR | Array closing indentation error, expected 4 spaces but found 6
80 | ERROR | Spaces must be used to indent lines; tabs are not allowed
81 | ERROR | Array indentation error, expected 6 spaces but found 5
84 | ERROR | Spaces must be used to indent lines; tabs are not allowed
84 | ERROR | Array indentation error, expected 6 spaces but found 5
85 | ERROR | Spaces must be used to indent lines; tabs are not allowed
86 | ERROR | Array indentation error, expected 7 spaces but found 8
87 | ERROR | Array closing indentation error, expected 5 spaces but found 6
89 | ERROR | Spaces must be used to indent lines; tabs are not allowed
89 | ERROR | Array indentation error, expected 8 spaces but found 4
90 | ERROR | Spaces must be used to indent lines; tabs are not allowed
90 | ERROR | Array indentation error, expected 8 spaces but found 7
98 | WARNING | Line exceeds 80 characters; contains 84 characters
98 | ERROR | Function comment short description must be on a single line,
| | further text should be a separate paragraph
98 | ERROR | Function comment short description must end with a full stop
105 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mark_as_read.module
--------------------------------------------------------------------------------
FOUND 64 ERROR(S) AND 8 WARNING(S) AFFECTING 49 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | You must use "/**" style comments for a file comment
19 | ERROR | Do not use t() in hook_menu()
26 | ERROR | There must be no space between the Array keyword and the
| | opening parenthesis
27 | ERROR | Do not use t() in hook_menu()
31 | ERROR | Do not use t() in hook_menu()
36 | ERROR | There must be no space between the Array keyword and the
| | opening parenthesis
37 | ERROR | Do not use t() in hook_menu()
41 | ERROR | Do not use t() in hook_menu()
47 | ERROR | There must be no space between the Array keyword and the
| | opening parenthesis
48 | ERROR | Do not use t() in hook_menu()
56 | ERROR | There must be no space between the Array keyword and the
| | opening parenthesis
57 | ERROR | Do not use t() in hook_menu()
59 | ERROR | TRUE, FALSE and NULL must be uppercase; expected "NULL" but
| | found "null"
66 | ERROR | Do not use t() in hook_menu()
73 | ERROR | Do not use t() in hook_menu()
85 | ERROR | Function comment short description must be on a single line,
| | further text should be a separate paragraph
91 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
97 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
104 | ERROR | Function comment short description must be on a single line,
| | further text should be a separate paragraph
106 | ERROR | There must be an empty line before the parameter block
106 | ERROR | Last parameter comment requires a blank newline after it
106 | ERROR | Doc comment for var $list does not match actual variable name
| | $lists at position 1
106 | ERROR | Missing parameter type at position 1
108 | ERROR | Additional blank line found at the end of doc comment
111 | ERROR | Concat operator must be surrounded by spaces
123 | WARNING | A comma should follow the last multiline array item. Found:
| | $marked_as_unread_class
132 | WARNING | Line exceeds 80 characters; contains 115 characters
132 | ERROR | Function comment short description must be on a single line,
| | further text should be a separate paragraph
133 | WARNING | Line exceeds 80 characters; contains 165 characters
134 | WARNING | Line exceeds 80 characters; contains 98 characters
136 | WARNING | Line exceeds 80 characters; contains 98 characters
143 | ERROR | Expected 1 space between asterisk and tag; 2 found
143 | ERROR | Missing parameter type at position 1
144 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
151 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
177 | ERROR | Function comment short description must be on a single line,
| | further text should be a separate paragraph
193 | ERROR | Expected "while (...) {\n"; found "while(...) {\n"
201 | ERROR | There must be an empty line before the parameter block
201 | ERROR | Last parameter comment requires a blank newline after it
201 | ERROR | Expected 1 space before variable type
201 | ERROR | Missing comment for param "$list_id" at position 1
202 | WARNING | Line exceeds 80 characters; contains 83 characters
202 | ERROR | Return comment must be on the next line
212 | ERROR | Function comment short description must be on a single line,
| | further text should be a separate paragraph
214 | ERROR | There must be an empty line before the parameter block
214 | ERROR | Superfluous doc comment at position 1
214 | ERROR | Missing parameter type at position 1
265 | ERROR | There must be an empty line before the parameter block
265 | ERROR | Expected 1 space before variable type
265 | ERROR | Doc comment for var Individual does not match actual variable
| | name $list_id at position 1
265 | ERROR | Parameter comment must be on the next line at position 1
285 | ERROR | There must be an empty line before the parameter block
285 | ERROR | Doc comment for var $list_id, does not match actual variable
| | name $list_id at position 1
285 | ERROR | Missing parameter type at position 1
285 | ERROR | Parameter comment must be on the next line at position 1
290 | ERROR | Whitespace found at end of line
297 | ERROR | Function comment short description must be on a single line,
| | further text should be a separate paragraph
298 | WARNING | Line exceeds 80 characters; contains 135 characters
300 | ERROR | There must be an empty line before the parameter block
300 | ERROR | Doc comment for var $list_id, does not match actual variable
| | name $list_id at position 1
300 | ERROR | Missing parameter type at position 1
300 | ERROR | Parameter comment must be on the next line at position 1
307 | WARNING | Line exceeds 80 characters; contains 82 characters
311 | ERROR | Whitespace found at end of line
315 | ERROR | Inline control structures are not allowed
316 | ERROR | Space found before semicolon; expected "return;" but found
| | "return ;"
344 | ERROR | There must be an empty line before the parameter block
344 | ERROR | Missing parameter type at position 1
362 | ERROR | There must be an empty line before the parameter block
362 | ERROR | Missing parameter type at position 1
384 | ERROR | There must be an empty line before the parameter block
384 | ERROR | Missing parameter type at position 1
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mark_as_read.pages.inc
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AND 1 WARNING(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found
| | "\r\n"
27 | ERROR | Function comment short description must be on a single line,
| | further text should be a separate paragraph
28 | WARNING | Line exceeds 80 characters; contains 89 characters
30 | ERROR | @return data type must not contain "$"
41 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/test/mark_as_read.test
--------------------------------------------------------------------------------
FOUND 10 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"
2 | ERROR | Missing file doc comment
4 | ERROR | Missing function doc comment
15 | ERROR | There must be an empty line before the parameter block
15 | ERROR | Missing comment for param "$modules" at position 1
52 | ERROR | Space found before comma in function call
60 | ERROR | Spaces must be used to indent lines; tabs are not allowed
60 | ERROR | Line indented incorrectly; expected 4 spaces, found 3
60 | ERROR | Whitespace found at end of line
70 | ERROR | Files must end in a single new line character
---------------------------------------------------------------------

@sandykadam: Please don't change the title on the projects you review! This is the third one I've seen that you changed recently. Also, please DON'T paste the entire pareview output here - link to it instead, like was done in #1.

Still an awful lot of pareview errors. It doesn't look like you fixed any of the things shown in #13. Here is the latest review: http://pareview.sh/pareview/httpgitdrupalorgsandboxprabeengiri2175185git

Hello prabeen.iri,

I tried your module and I can see the following potential blockers:

- When I edit a list item's name, a new one is created instead of just updating the one I'm editing.
- In the List/Menu Name description, it's written "For Eg: MainMenu or User Navigation or Primary Menu" - does that mean it doesn't work with machine names ?
- t() functions would be welcome on menu item's description values.
- This warning appear on every page whether or not I have a list:

"Warning: array_intersect(): Argument #1 is not an array in mark_as_read_is_js_required() (line 169 of mark_as_read.module)."

Apart from that and coding standards, I failed to see how to use the module.

Good luck on your project application.

- t() functions would be welcome on menu item's description values.

No, that's wrong, don't do that - descriptions in hook_menu() should never use t().

Prabeen, RE: Comment #9, Storing variables to a custom table defined by your hook_schema seems dicey. The drupal core variable_set() function runs db_merge on the 'variable' table itself the global $conf variable returns an array of variables from the variable table, variable_get() and variable_de are based on that. Perhaps I don't fully understand what you are saying in comment #4, but I don't see how you can run variable_set in you hook_install and possibly set the variable to a custom table. In which case if I'm not mistaken, there needs to be a mark_as_read_uninstall() with a variable_del() to remove this variable. I too am confused a littl eby what D7 automatically removes on unistall, but I don't see why there would be hook_uninstall function that can run variable_del if it does this automatically, that seems to redundant for the great folks coding drupal core. Take a look at: https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func... and https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/vari... and subsequent links.

I could get this working. I am thinking of using this in one intranet site that I am working on.

I have moved your status to reviewed by community as I dont see any major issue.

Hi Prabeen,

I wasn't actually able to get the module working (due to an error documented below) but was still able to carry out the following testing:

There are quite a lot of errors in the pareview review. It's always a good idea to clear these down before setting the projects status to needs review.

I hope you don't mind but I taken the liberty of editing you readme file - I've just changed a few grammatical points to make it read more easily but not changed any of the meaning. The updated copy is on pastebin here.

On the settings page the description for "Render Javascript on all the pages specified" and "Render Javacsript on all the pages except" the end of the descrption reads "is the front page." but should read "<front> is the front page."

Whilst using the admin interface I created a new list and upon clicking save got the error:

Fatal error: Call to undefined function mark_as_read_db_get_by_name() in /var/www/test/htdocs/sites/all/modules/mark_as_read/mark_as_read.admin.inc on line 202

Because of this I was not able to carry out any further user testing.

I'm not sure about the purpose of the file mark_as_read.api.php. It shows examples of how to use 3 hooks however the only hook you implement is "mark_as_read_prevent". Should either implement the other hooks mentioned or remove any mention of them all together. I also think that this information could sit in a readme file and could remove this file all together.

Removing review bonus tag, you have not done all manual reviews, you just copied the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. I don't understand the project page. What is going to be marked as read, nodes or entities or links? What is the use case of this module? Could you describe a user story on the project page how this is supposed to be used? Is this used for click tracking to analyze that as admin?
2. mark_as_read.api.php: the hook bodies should have some example code.
3. mark_as_read_install(): no need to set variables upon installation as you can make use of the default values with the second parameter of variable_get() anyway.
4. mark_as_read_init(): why do you use hook_init()? It does not make sense to add Javascript on drush invocations for example? I think you should rather use hook_page_build() when an actual HTML output page is generated.
5. mark_as_read_add_attribute(): this is not an access callback, this is a page callback, right? See page router callback functions at https://drupal.org/node/1354#functions
6. mark_as_read_add_attribute(): so this looks vulnerable to CSRF exploits since you are not using the form API and you are not validating any security tokens before writing to the database. I think you just need to add a drupal_get_token() to the Drupal.settings array somewhere which you then send along with you ajax request. You could also look into using Drupal's AJAX form API which would also handle that for you. That way you can make sure that the request is genuine and not being forged from somewhere else. More info on CSRF: http://epiqo.com/en/all-your-pants-are-danger-csrf-explained . And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
7. "$form_state['redirect'] = drupal_get_path_alias(MARK_AS_READ_ADMIN_URL);": you don't need drupal_get_path_alias() here, Drupal will translate into an alias later automatically if one exists.
8. mark_as_read_db_insert_visited_activity(): the check_plain()s are wrong here. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." Same in mark_as_read_add_attribute(). Make sure to read https://drupal.org/node/28984 again.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Tagging.

Hi prabeen.giri,

First thing I ran into when I tested your module was a warning after I saved the mark_as_read configuration settings.

Illegal offset type in drupal_set_message() (line 1780 of /Users/nathalie/Sites/drupal-7.27/includes/bootstrap.inc).

That had something to do with the code in line 229 of mark_as_read.admin and how you used the variable in the t function:

drupal_set_message(t("Data for <em>@name</em> successfully saved."), array('@name' => $list->list_name));

Then, for some reason, I couldn't get the results I expected from the module, neither in Safari nor in Chrome. Maybe I understood the purpose of your module wrong. I thought that it would add an attribute to the menu ul li a link called data-id, if I clicked on a main menu item with the settings as shown in the attached screen shots. Maybe you need to explain the purpose and the results a bit more in your README file.

please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Contrary to the project description page, this is not a small project. But it will be a great addition to the module space. Thanks for contributing it to the community.

Maybe I don't understand the purpose well enough, but should you be wrapping $ in a closure in mark_as_read.api.js? Any maybe using Drupal behaviors?

$form_state['redirect'] = drupal_get_path_alias(MARK_AS_READ_ADMIN_URL);
This is not necessary. Drupal will translate into an alias later automatically if one exists.

Nit: Typo in // Exisiting columns. & in Consturct the object based on the argument provided.

• also in
  

      '#description'  => t("Name of the attribute that represents each list/menu item uniquely.</li>
    <li>     <div><strong>Note: If anchor(a) tag is not item provided above, you can provide any custom or default attribute which reprsents item uniquely in the list. For eg, <em>data-id</em> or <em>id</em></strong></div>"),</li>
    <li>    '#default_value'  => $list->attribute_name,

• '#title' => t('CSS class for Unread/Un-Visited HTML elment'),
• '#description' => t('Javascript will be loaded for every loggedin users regardless their roles.
• '#title' => t('Render Javacsript on all the pages except'),
• * Check if submited list name already exists or not. It is used as the unique
• Unique value of the attribute thats going to inserted.
• * Returning TRUE won't load Javsacript file at all and
• * Retuns all the list entry and its direct properties.
• throw new LogicException(t("There is no user activity for the speficified list. Hence, update failed"));
• // As this data direclty comes from the URL, so filter it.
• * Delete List Read/Undread Activity.
• // While inserting the deafult value to the database, its important
• * This is acutally doing the Binary Search on the sorted array. Extending the
• * @see has_attribtue_selector
• * Unique attribute name that idetifies elements uniquely.

And many other places. Please run the code through a spellchecker.

/**
   * Read data from inaccessible properties.
   *
   * @param string $property
   *   Inaccessible Property.
   */
  public function __get($property) {

Missing @return

Consider using fetchAllAssoc() as it will return the results keyed by the requested field, without having to loop over the results and assign it manually.

  $query->fields('lv')
  ->condition('list_name', $name);

This should have two spaces before the condition.

  $query->fields('lv', array('list_id', 'css_selector', 'attribute_name'))
  ->fields('lva', array('attribute_values'))
  ->leftJoin('mark_as_read_activity', 'lva', "lv.list_id = lva.list_id AND lva.uid = {$user->uid}");

Same here.

Use

// Retrieve all records into an indexed array of stdClass objects.
$result->fetchAll();

rather than:

while ($record = $result->fetchObject()) {
    $rows[] = $record;
  }

No need to load module_load_include('inc', 'mark_as_read', 'mark_as_read.features'); unless the features module is available. Use module_exists

JS implicitly declares variables in several places.
ex. activeLists = [];

api.js:
The example js still doesn't use behaviors. If it shouldn't, please provide a note to that effect so folks will know to make sure to format their JS in such a way. However, it really should be a behaviour, in case someone uses AJAX on a page.

js file:
Typo: In word 'Fascade' (at line 44)
Variable activeLists implicitly declared at line 46
Variable lists implicitly declared at line 47
Variable i implicitly declared at line 56
Variable $lists_json implicitly declared at line 62
Variable $list implicitly declared at line 63
Parameter class described in JSDoc does not appear in function signature (at line 107)
Unnecessary semicolon at line 173
Unnecessary semicolon at line 178
Unused parameter settings (at line 39). Rather than grabbing settings from the global Drupal namespace, please pass it into init, etc. That will cause for better performance.
Drupal.MarkAsRead.ElementHandler = function(el, list) -- This doesn't actually return anything.
Drupal.MarkAsRead.ReadElementHandler = function(el, list) -- This doesn't actually return anything.
Drupal.MarkAsRead.UnReadElementHandler = function(el, list) -- This doesn't actually return anything.
Unnecessary semicolon at line 173
Unnecessary semicolon at line 178

.module file:
Missing @return tag in function/method PHPDoc comment (at line 145)
Typo: In word 'Javasript' (at line 98)
Typo: In word 'Javsacript' (at line 118)
Typo: In word 'makred' (at line 127)
Typo: In word 'indvidual' (at line 148)
Typo: In word 'Javasript' (at line 149)
Typo: In word 'accsess' (at line 158)
Unused parameter $lists (at line 168)

.api.php
Typo: In word 'thats' (at line 14)

class.inc
Typo: In word 'Consturct' (at line 16)

admin.inc
Inconsistent use of single and double quotes. '#description' uses double quotes several times, with no real good reason. Single quotes have better PHP performance.
mark_as_read_lists_table_theme should use a theme definition.

In general, I think this is getting closer. However, after looking at the code now several times, I think you should at least consider using drupal's entity system to store the data represented in the two schema entries. That would require some thought and effort, so take this suggestion with a grain of salt. However, the nice thing is that with the entity system the code would cleanup and be easier because things like saving and updating would be taken care of for you by the entity api. Plus you could easily expose the data via Views and other methods. See https://drupal.org/node/878784 for a great launching point.

I like your response about entity api. It makes sense.

I'm marking RTBC. I'd still like to see the example js converted to a behavior. Most developers are copy/paste so developing example code that uses a behavior will only help other developers. But that isn't a blocker.

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. mark_as_read_add_attribute(): why do you throw InvalidArgumentExceptions here? I think it would be enough to just deny access in such cases by returning MENU_ACCESS_DENIED?
2. theme_mark_as_read_lists_table(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as list name I will get a nasty javascript popup. You need to sanitize user provided text if you are printing it directly, make sure to read https://drupal.org/node/28984 again. This is not a security blocker since the high level "administer site configuration" permission is required for an attacker to exploit this, but you should fix it anyway.
3. theme_mark_as_read_lists_table(): A theme function should not call a DB loading function. Anything that is needed for theming should be passed in variables to the theme function.
4. mark_as_read_admin_list_form(): why is this a form, if it really is not a form but a standard page callback?

But otherwise looks good to me, so ...

Thanks for your contribution, prabeen.giri!

I updated 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:

• 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 stay 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.