This module marks the link/HTML element read/unread providing the respective class to that particular HTML element.
Project Link
https://drupal.org/sandbox/prabeengiri/2175185
Git Link
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/prabeen.giri/2175185.git mark_as_read
Manual reviews of other projects
https://drupal.org/comment/8595829#comment-8595829
https://drupal.org/comment/8571449#comment-8571449
https://drupal.org/comment/8672091#comment-8672091
Some more manual reviews
https://drupal.org/comment/8703447#comment-8703447
https://drupal.org/comment/8696547#comment-8696547
https://drupal.org/comment/8696515#comment-8696515
Comment | File | Size | Author |
---|---|---|---|
#50 | coder-results.txt | 3.58 KB | klausi |
#42 | drupal_mark_as_read_module_test_screenshot-2.png | 25.2 KB | nsuit |
#42 | drupal_mark_as_read_module_test_screenshot-1.png | 10.92 KB | nsuit |
#37 | coder-results.txt | 7.26 KB | klausi |
Comments
Comment #1
PA robot CreditAttribution: PA robot commentedThere 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.
Comment #2
markaspot CreditAttribution: markaspot commentedHey, 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:
Cheers!
Comment #3
prabeen.giri CreditAttribution: prabeen.giri commentedI have removed the unnecessary files and also fixed the admin add/edit form submission exception. I have added some extra fields in the admin which has not been implemented yet. Its under progress and will be soon implemented.
Comment #4
prabeen.giri CreditAttribution: prabeen.giri commentedComment #5
prabeen.giri CreditAttribution: prabeen.giri commentedComment #6
rmn CreditAttribution: rmn commentedSetting the priority back to normal.
@Prabin, please make sure to read the guidelines for project applications here, specifically the Application Priority section.
Comment #7
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedIssues:
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 ofgit 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
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
Comment #8
prabeen.giri CreditAttribution: prabeen.giri commentedComment #9
prabeen.giri CreditAttribution: prabeen.giri commentedNitesh Thanks for the review, however will the 4th comment, you were mistaken as the admin form is not saving the data to the 'variables' table ,actually its saving to the custom table, and for the settings part , its is using module_name as the prefix.
And with your second, you are not being specific as its too abstract. I have tried to follow the Drupal standards as much as I can, it would be great if you can point out the specific of Drupal standards.
For others, I have made the changes.
Comment #10
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedComment #11
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedFor the second point have a look at the list of issues on
http://pareview.sh/pareview/httpgitdrupalorgsandboxprabeengiri2175185git
Comment #12
prabeen.giri CreditAttribution: prabeen.giri commentedFixed most of the Indentation and New Line characters and other character encoding issues to align with Drupal standards
Comment #13
sandykadam CreditAttribution: sandykadam commentedPlease 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
---------------------------------------------------------------------
Comment #14
TR CreditAttribution: TR commented@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.
Comment #15
prabeen.giri CreditAttribution: prabeen.giri commentedComment #16
TR CreditAttribution: TR commentedStill 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
Comment #17
prabeen.giri CreditAttribution: prabeen.giri commentedThere are just few errors that is with Drupal doc with comments and white spacing on the javascript which I was not able to figure out and they are not obvious(Its not that I did not try). I don't think that should not be considered as the blocker here and I will really appreciate that anyone who reviews other that formatting and indentation.
Comment #18
prabeen.giri CreditAttribution: prabeen.giri commentedComment #19
b2f CreditAttribution: b2f commentedHello 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.
Comment #20
b2f CreditAttribution: b2f commentedComment #21
TR CreditAttribution: TR commentedNo, that's wrong, don't do that - descriptions in hook_menu() should never use t().
Comment #22
j.branson CreditAttribution: j.branson commentedPrabeen, 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.
Comment #23
prabeen.giri CreditAttribution: prabeen.giri commentedThanks for taking time to review my project.
@ B2F I have fixed the logical and php errors, that happened as I checked the old files.
I will not add t() function as I don't even see drupal core modules doing that.
@TR
I have adde the variable_del on the hook_uninstall() .
Thanks all!!
Comment #24
prabeen.giri CreditAttribution: prabeen.giri commentedComment #25
gbohara CreditAttribution: gbohara commentedI could get this working. I am thinking of using this in one intranet site that I am working on.
Comment #26
gbohara CreditAttribution: gbohara commentedI have moved your status to reviewed by community as I dont see any major issue.
Comment #27
prabeen.giri CreditAttribution: prabeen.giri commentedChanging the status as I made little changes!!
Comment #28
prabeen.giri CreditAttribution: prabeen.giri commentedComment #29
prabeen.giri CreditAttribution: prabeen.giri commentedComment #30
prabeen.giri CreditAttribution: prabeen.giri commentedComment #31
dahousecat CreditAttribution: dahousecat commentedHi 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.
Comment #32
dahousecat CreditAttribution: dahousecat commentedComment #33
prabeen.giri CreditAttribution: prabeen.giri commentedThank you dahousecat for all the testing you made. I am able to fix all the pareview error and undefined function error.
Comment #34
klausiRemoving 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.
Comment #35
prabeen.giri CreditAttribution: prabeen.giri commentedComment #36
prabeen.giri CreditAttribution: prabeen.giri commentedI have added one more missing manual review!!
Comment #37
klausiReview of the 7.x-1.x branch:
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:
Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #38
klausiTagging.
Comment #39
prabeen.giri CreditAttribution: prabeen.giri commentedThank you Klausi for all the review. I just got busy with my project work. I will fix all the issues that you mentioned as I will get some free time.
Comment #40
prabeen.giri CreditAttribution: prabeen.giri commentedComment #41
prabeen.giri CreditAttribution: prabeen.giri commentedKlausi, I have addressed all the issues that you mentioned above and also added 3 manual reviews on issue summary.
I have also updated the project page.
For number 3 I did not do anything because as its the default page list, which I don't want to create as default value everywhere I used variable_get(). I don't think it would hurt to set only that variable through install file. If that sounds as the blocker then let me know, I can have that resolved too!!
Comment #42
nsuit CreditAttribution: nsuit commentedHi prabeen.giri,
First thing I ran into when I tested your module was a warning after I saved the mark_as_read configuration settings.
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:
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.
Comment #43
prabeen.giri CreditAttribution: prabeen.giri commentedThanks @Insuit for you review. Issue you mentioned above has been resolved, that was a very stupid mistake as I was passing argument to the drupal_set_message() function instead of t() function. With the doc, I have to tried to make it as compherensive as possible, however I have added the image, may be that will supplement the document. Basically it adds the class "marked-as-read' or "marked-as-unread" or bold/unbold to the link of any other html element if it is already clicked by the user. Its the same concept as how email client has "mark as read" feature if the link is clicked, or it unbolds the email on the list if its clicked already.
Thanks again for taking some time to review my module!!
Comment #44
heddnContrary 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.
& inConsturct the object based on the argument provided.
'#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.
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.
This should have two spaces before the condition.
Same here.
Use
rather than:
No need to load
module_load_include('inc', 'mark_as_read', 'mark_as_read.features');
unless the features module is available. Usemodule_exists
JS implicitly declares variables in several places.
ex.
activeLists = [];
Comment #45
prabeen.giri CreditAttribution: prabeen.giri commentedThanks @heddn for all of your reviews. They were really helpful. I have fixed all the issues that you pointed out.
Comment #46
prabeen.giri CreditAttribution: prabeen.giri commentedComment #47
heddnapi.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.
Comment #48
prabeen.giri CreditAttribution: prabeen.giri commented@heddn. Thank you so much again for taking time to go into depth to cover every minor to major issues. I tried using spell checker but could not find the valid versions. With example js API and Drupal behaviors, Its just for the documentation purpose and how those two events can be triggered outside the module. It can be used anywhere if required. I assumed that the end developer is just aware of Drupal.behaviours. I thought adding the Drupal.behaviors in the API might confuse the novice Drupal Developers. I just wanted to explain it as just a jquery 'click', 'change' event which can be triggered as per requirement.
With your last statement of using entity API for storing and retrieving the data. I do have a very good experience using entity API with custom tables and web services, however, for this case, I don't think it suits as data will be just used for the JavaScript settings on the client side. These data serves no purpose to the end client.
Thank you so much again taking time to look into the module and point very small things which as a developer tend to ignore most of the time. I really appreciate your effort.
Comment #49
heddnI 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.
Comment #50
klausiReview of the 7.x-1.x branch:
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:
<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.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:
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.
Comment #51
prabeen.giri CreditAttribution: prabeen.giri commented@klausi All of the issues that you mentioned are fixed.
Thanks for the review and approval!! woho!! :)