This is a Drupal 7 module, originally based on the Drupal 6 module called "Splash": http://drupal.org/project/splash . My original intent was to help the owner with porting the module to Drupal 7....but I never got a reply from the project maintainer. Here is the issue where I did this: #908440: Port Splash to D7.

I decided that I was going to re-write a similar module for Drupal 7 and add features that the company I work for has used in custom implementations of the Splash module. The base settings for this module were based on the splash module. But I wrote everything from scratch.

This module has the following features:

- Instead of redirecting from PHP, we now redirect via JavaScript. This prevents the splash page from showing up when search engine crawlers go to the site. We had issues where Google would index the home page as the splash page, which is a major problem.

- The module has settings for mobile devices. So, for example, you can show a specific splash page for mobile devices....or not show a splash page at all for mobile devices.

- You can have the splash page show up on any page of the site....not just the home page.

- Instead of using cookies, the module uses jStorage.

- For lightbox functionality, the module uses ColorBox.

Here is the sandbox project: http://drupal.org/sandbox/chrisroane/1423456

There is a Drupal 7 module called Front that has a few similar features. However, it doesn't allow you to control how often the front page loads and there are many other features that were in the Splash module that are not in Front. I did bring this up in an issue on the Front module, but the maintainer did not think it fit well with the module: #1134574: Only Activate Once.

I know it has only been a few days since I created the Splashify sandbox project. But keep in mind that I used some code that we had modified the Splash module with. And I spent significant time last week getting the project to where it is now. I made sure the project conforms with the Drupal standards (by using the Coder module). I feel pretty good with where the project is at.

Let me know if there is anything I need to do so that this can be released as a full project. The company I work for used the Splash module all the time, and I would be an active maintainer for the project.

git clone --branch master chrisroane@git.drupal.org:sandbox/chrisroane/1423456.git

Reviews of Other Projects

2/6/2012
http://drupal.org/node/1403436#comment-5555240
http://drupal.org/node/1355894#comment-5559502
http://drupal.org/node/1174572#comment-5565380

2/13/2012
http://drupal.org/node/1406352#comment-5597044
http://drupal.org/node/1421568#comment-5597130
http://drupal.org/node/1353840#comment-5600624

2/27/2012
http://drupal.org/node/1237070#comment-5659620
http://drupal.org/node/1400086#comment-5664420
http://drupal.org/node/1417490#comment-5664636

3/2/2012
http://drupal.org/node/1434306#comment-5681252
http://drupal.org/node/1415882#comment-5681378
http://drupal.org/node/1437082#comment-5681440

Comments

vaibhavjain’s picture

vaibhavjain
he/him
Primary language Hindi
Location Delhi
commented
Status: Needs review » Needs work

Please check on this
Manual Review
You are working on master branch, for D7 you need to create a new branch 7.x-1.x
Remove LICENSE.txt, it will be added by drupal.org packaging automatically.

Automated Review
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. Get a review bonus and we will come back to your application sooner.

FILE: ...l-7-pareview/sites/all/modules/pareview_temp/test_candidate/LICENSE.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 3 WARNING(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
106 | WARNING | Line exceeds 80 characters; contains 81 characters
120 | WARNING | Line exceeds 80 characters; contains 84 characters
227 | WARNING | Line exceeds 80 characters; contains 81 characters
--------------------------------------------------------------------------------

FILE: ...upal-7-pareview/sites/all/modules/pareview_temp/test_candidate/TODO.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...view/sites/all/modules/pareview_temp/test_candidate/splashify.admin.inc
--------------------------------------------------------------------------------
FOUND 66 ERROR(S) AND 8 WARNING(S) AFFECTING 67 LINE(S)
--------------------------------------------------------------------------------
6 | WARNING | Line exceeds 80 characters; contains 92 characters
6 | ERROR | Whitespace found at end of line
7 | WARNING | Line exceeds 80 characters; contains 87 characters
7 | ERROR | Whitespace found at end of line
14 | ERROR | You must use "/**" style comments for a function comment
15 | ERROR | There must be no blank line following an inline comment
16 | ERROR | Whitespace found at end of line
17 | ERROR | Whitespace found at end of line
39 | ERROR | You must use "/**" style comments for a function comment
39 | ERROR | Whitespace found at end of line
41 | ERROR | Whitespace found at end of line
45 | ERROR | Whitespace found at end of line
51 | ERROR | Whitespace found at end of line
52 | ERROR | Whitespace found at end of line
53 | ERROR | Whitespace found at end of line
61 | ERROR | Whitespace found at end of line
63 | ERROR | Whitespace found at end of line
68 | ERROR | Whitespace found at end of line
73 | ERROR | Whitespace found at end of line
82 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
85 | ERROR | Whitespace found at end of line
86 | ERROR | Whitespace found at end of line
94 | ERROR | Whitespace found at end of line
96 | ERROR | Whitespace found at end of line
112 | ERROR | You must use "/**" style comments for a function comment
114 | ERROR | Whitespace found at end of line
115 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
116 | ERROR | An operator statement must be followed by a single space
134 | ERROR | Whitespace found at end of line
138 | ERROR | Whitespace found at end of line
143 | ERROR | Whitespace found at end of line
144 | ERROR | Whitespace found at end of line
151 | ERROR | Whitespace found at end of line
160 | ERROR | Whitespace found at end of line
165 | ERROR | Whitespace found at end of line
166 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
168 | ERROR | An operator statement must be followed by a single space
184 | ERROR | Whitespace found at end of line
186 | ERROR | Whitespace found at end of line
193 | ERROR | Whitespace found at end of line
208 | ERROR | Whitespace found at end of line
215 | ERROR | You must use "/**" style comments for a function comment
217 | ERROR | Whitespace found at end of line
221 | ERROR | Whitespace found at end of line
226 | ERROR | Whitespace found at end of line
239 | ERROR | Whitespace found at end of line
248 | ERROR | Whitespace found at end of line
253 | ERROR | Whitespace found at end of line
254 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
268 | ERROR | Whitespace found at end of line
277 | ERROR | Whitespace found at end of line
283 | ERROR | Whitespace found at end of line
290 | ERROR | You must use "/**" style comments for a function comment
292 | ERROR | Whitespace found at end of line
296 | ERROR | Whitespace found at end of line
302 | ERROR | Whitespace found at end of line
305 | ERROR | Whitespace found at end of line
307 | ERROR | The $text argument to l() should be enclosed within t() so
| | that it is translatable
307 | WARNING | Last item of an inline array must not be followed by a comma
307 | WARNING | The closing paranthesis of an array should not be preceded by
| | a white space
307 | WARNING | Last item of an inline array must not be followed by a comma
307 | WARNING | The closing paranthesis of an array should not be preceded by
| | a white space
309 | ERROR | Whitespace found at end of line
314 | ERROR | Whitespace found at end of line
323 | ERROR | Whitespace found at end of line
325 | WARNING | Line exceeds 80 characters; contains 89 characters
339 | ERROR | Whitespace found at end of line
344 | ERROR | Whitespace found at end of line
345 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
355 | ERROR | Whitespace found at end of line
357 | WARNING | Line exceeds 80 characters; contains 91 characters
371 | ERROR | Whitespace found at end of line
372 | ERROR | Whitespace found at end of line
378 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/splashify.display.inc
--------------------------------------------------------------------------------
FOUND 112 ERROR(S) AND 15 WARNING(S) AFFECTING 111 LINE(S)
--------------------------------------------------------------------------------
6 | WARNING | Line exceeds 80 characters; contains 87 characters
6 | ERROR | Whitespace found at end of line
12 | ERROR | Whitespace found at end of line
15 | ERROR | You must use "/**" style comments for a function comment
17 | ERROR | Whitespace found at end of line
20 | ERROR | The $text argument to l() should be enclosed within t() so
| | that it is translatable
20 | WARNING | Last item of an inline array must not be followed by a comma
20 | WARNING | The closing paranthesis of an array should not be preceded by
| | a white space
23 | ERROR | Whitespace found at end of line
28 | ERROR | Whitespace found at end of line
33 | ERROR | Whitespace found at end of line
42 | ERROR | Whitespace found at end of line
43 | ERROR | Whitespace found at end of line
49 | ERROR | Whitespace found at end of line
53 | ERROR | Whitespace found at end of line
69 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
79 | ERROR | Whitespace found at end of line
81 | WARNING | Line exceeds 80 characters; contains 87 characters
86 | ERROR | Whitespace found at end of line
107 | ERROR | Whitespace found at end of line
112 | ERROR | Whitespace found at end of line
113 | ERROR | Whitespace found at end of line
116 | ERROR | Whitespace found at end of line
119 | ERROR | Whitespace found at end of line
123 | ERROR | Whitespace found at end of line
124 | WARNING | Line exceeds 80 characters; contains 82 characters
124 | ERROR | Whitespace found at end of line
127 | ERROR | Whitespace found at end of line
128 | WARNING | Line exceeds 80 characters; contains 82 characters
130 | ERROR | Whitespace found at end of line
133 | ERROR | Whitespace found at end of line
134 | ERROR | Whitespace found at end of line
134 | ERROR | Comments may not appear after statements.
135 | WARNING | Line exceeds 80 characters; contains 81 characters
135 | ERROR | Comments may not appear after statements.
136 | ERROR | Whitespace found at end of line
137 | ERROR | Whitespace found at end of line
137 | ERROR | Comments may not appear after statements.
138 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
138 | ERROR | Comments may not appear after statements.
139 | ERROR | Whitespace found at end of line
140 | ERROR | Whitespace found at end of line
140 | ERROR | Comments may not appear after statements.
141 | ERROR | Comments may not appear after statements.
142 | ERROR | Whitespace found at end of line
143 | WARNING | Line exceeds 80 characters; contains 87 characters
143 | ERROR | Whitespace found at end of line
143 | ERROR | Comments may not appear after statements.
144 | ERROR | Comments may not appear after statements.
153 | ERROR | Whitespace found at end of line
155 | ERROR | Whitespace found at end of line
159 | ERROR | Whitespace found at end of line
162 | ERROR | Whitespace found at end of line
166 | ERROR | Whitespace found at end of line
168 | WARNING | Line exceeds 80 characters; contains 87 characters
168 | ERROR | Whitespace found at end of line
169 | WARNING | Line exceeds 80 characters; contains 84 characters
169 | ERROR | Whitespace found at end of line
172 | ERROR | Whitespace found at end of line
173 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
181 | ERROR | Whitespace found at end of line
184 | ERROR | Whitespace found at end of line
185 | WARNING | Line exceeds 80 characters; contains 81 characters
188 | ERROR | Whitespace found at end of line
192 | ERROR | Whitespace found at end of line
195 | ERROR | Whitespace found at end of line
196 | ERROR | Whitespace found at end of line
199 | ERROR | Whitespace found at end of line
202 | ERROR | Whitespace found at end of line
204 | ERROR | Whitespace found at end of line
207 | ERROR | Whitespace found at end of line
209 | ERROR | Whitespace found at end of line
213 | WARNING | Line exceeds 80 characters; contains 86 characters
218 | ERROR | Whitespace found at end of line
219 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
221 | ERROR | Whitespace found at end of line
224 | ERROR | Whitespace found at end of line
225 | ERROR | Whitespace found at end of line
225 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
227 | ERROR | Whitespace found at end of line
228 | ERROR | Whitespace found at end of line
231 | ERROR | Whitespace found at end of line
232 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
237 | ERROR | Whitespace found at end of line
238 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
248 | ERROR | Whitespace found at end of line
250 | ERROR | Whitespace found at end of line
253 | ERROR | Whitespace found at end of line
259 | ERROR | Whitespace found at end of line
262 | ERROR | Whitespace found at end of line
263 | WARNING | Line exceeds 80 characters; contains 106 characters
263 | ERROR | Whitespace found at end of line
266 | ERROR | An operator statement must be followed by a single space
280 | ERROR | Whitespace found at end of line
281 | ERROR | Whitespace found at end of line
282 | ERROR | Whitespace found at end of line
283 | ERROR | Whitespace found at end of line
285 | ERROR | Whitespace found at end of line
287 | ERROR | Whitespace found at end of line
288 | ERROR | Whitespace found at end of line
289 | ERROR | Whitespace found at end of line
290 | ERROR | Whitespace found at end of line
291 | ERROR | Whitespace found at end of line
293 | ERROR | Whitespace found at end of line
299 | ERROR | You must use "/**" style comments for a function comment
301 | ERROR | Whitespace found at end of line
310 | ERROR | Whitespace found at end of line
325 | ERROR | Whitespace found at end of line
326 | WARNING | Line exceeds 80 characters; contains 106 characters
328 | ERROR | You must use "/**" style comments for a function comment
331 | ERROR | Whitespace found at end of line
332 | WARNING | Line exceeds 80 characters; contains 85 characters
334 | ERROR | Whitespace found at end of line
337 | ERROR | An operator statement must be followed by a single space
337 | ERROR | There must be a single space before an operator statement
338 | ERROR | Whitespace found at end of line
340 | ERROR | Whitespace found at end of line
345 | ERROR | Whitespace found at end of line
347 | ERROR | Whitespace found at end of line
348 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
349 | ERROR | Whitespace found at end of line
353 | ERROR | Whitespace found at end of line
357 | ERROR | Whitespace found at end of line
359 | ERROR | Whitespace found at end of line
361 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
370 | ERROR | Whitespace found at end of line
373 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

FILE: ...-pareview/sites/all/modules/pareview_temp/test_candidate/splashify.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
4 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...areview/sites/all/modules/pareview_temp/test_candidate/splashify.module
--------------------------------------------------------------------------------
FOUND 14 ERROR(S) AND 1 WARNING(S) AFFECTING 14 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
2 | ERROR | "require_once" is a statement not a function; no parentheses
| | are required
7 | ERROR | You must use "/**" style comments for a function comment
9 | ERROR | Whitespace found at end of line
10 | WARNING | Line exceeds 80 characters; contains 100 characters
18 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
26 | ERROR | Whitespace found at end of line
35 | ERROR | Whitespace found at end of line
45 | ERROR | Whitespace found at end of line
55 | ERROR | Whitespace found at end of line
64 | ERROR | Array closing indentation error, expected 2 spaces but found 3
65 | ERROR | Whitespace found at end of line
75 | ERROR | Whitespace found at end of line
85 | ERROR | Whitespace found at end of line
91 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------

patrickd’s picture

patrickd commented

@vaibhavjain
please put those lines into <code> tags in future!
hard to read that this way

chrisroane’s picture

chrisroane commented
Status: Needs work » Needs review

Thanks for the quick reply!

I fixed all of the issues that the automatic review utility brought up. I also fixed a few more bugs I found and updated the README.txt file. I also moved everything out of the master branch.

Let me know if you would like me to do anything else with this.

patrickd’s picture

patrickd commented

FYI you can help out and get a deeper review sooner: #1410826: [META] Review bonus

chrisroane’s picture

chrisroane commented
Issue tags: +PAreview: review bonus

Added tag "PAReview: review bonus"

sbrattla’s picture

sbrattla commented
Status: Needs review » Needs work

I haven't checked coding style, but I did download the module and gave it a try. It seems to be working well, and it is pretty straightforward to use. Here are my comments.

1. Under the 'Where' tab and 'List Pages' : would it not be possible to accept paths?
2. Under the 'Where' tab : maybe you should check up on the 'Home Page' term...I believe Drupal often uses 'Front Page' (like <front> to indicate front page in may config. situations).
3. Under the 'How' tab : only two options are available from the drop down, but the description beneath it mentions three? Aha...i see now (from the top warning message) that i need the colorbox module. Could the description be altered depending on wether the option really is available? I thought there was something wrong since it was mentioned but i couldn't find it in the list?

chrisroane’s picture

chrisroane commented
Status: Needs work » Needs review

Thanks for taking a look at the module.

1. What do you mean by path? It uses the real drupal url for a page. For example, for nodes it would be node/[node_id] or any other drupal page. This is a drupal only mechanism that is checked via hook_init().

2. That's an excellent point. I changed "Home Page" to "Front Page".

3. To make things more clear, I updated the description for that form field.

I committed the changes to the 7.x-1.x branch.

sbrattla’s picture

sbrattla commented

With regard to #1, I just mean that it would be great if the module allowed me to enter 'alias' paths, like 'news', 'world/events' or the like. I guess this could be solved by having the module to keep track of 'path' and the real path that the provided path translates to.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus

manual review:

  • "file_exists('sites/all/libraries/jstorage/jstorage.min.js')": do not hardcode the path to the library. The Libraries API module is a recommended method for adding 3rd party dependencies.
  • splashify_init(): the library check should be done in hook_requirements().
  • "drupal_set_message(filter_xss($jstorage_error), 'error');": why do you need filter_xss() here? There is no user provided data involved, so this is not necessary. Also elsewhere.
  • splashify_init(): $splash_reason: that variable is never used?
  • splashify_init(): building the javascript here is ugly. You should put your javascript code into file(s), add it here and pass settings to it. See http://drupal.org/node/756722 and documentation for drupal_add_js().
  • splashify_admin_main(): you should use hook_help() for help text.
  • "'#title' => 'Enable Unique Mobile Splash',": all user facing text should run through t() for translation. Please check all your strings. Another one: "'#markup' => '<p>Where should the splash page come up?</p>',"
  • splashify_admin_where_form(): javascript should be in a js file.

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

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Oh and regarding your reviews: Please don't post the output of automated review tools like pareview.sh inline in a comment, as it just clutters the issue. Better add it as .txt attachment instead.

chrisroane’s picture

chrisroane commented

Regarding "drupal_set_message(filter_xss($jstorage_error), 'error');": I did this because the online PAReview tool kept on throwing a message about needing to use that in those cases. It didn't make sense to me, but I was trying to clear out those errors, and that was the only way I could get those warnings to go away.

Thanks for the review. I'll take care of these issues today.

patrickd’s picture

patrickd commented

don't concentrate on these automated tools too much,
they're not intelligent and they definitely have false-positives,
regard their issues as suggestions and handle them so.

chrisroane’s picture

chrisroane commented
Status: Needs work » Needs review

Updated the code, based on the manual review by klausi.

The two big updates where implementing the Libraries module and re-factoring the JavaScript code so that it is in a separate file (this required quite a bit of work). See this commit for further details: http://drupal.org/commitlog/commit/33260/7c745e95bb0b35a59d332b57b63fa38...

I ran the code through Coder and the PAReview tool and fixed the necessary issues. The online PAReview tool is giving one critical error relating to the filter_xss() code I had removed, but I know that the code is safe.

chrisroane’s picture

chrisroane commented

sbrattla: I did create a new issue feature request, regarding allowing alias values in the path fields: #1438874: Allow for entered site paths to be the original path or an alias for the page in the admin. Thanks for testing things from your end.

chrisroane’s picture

chrisroane commented
Issue summary: View changes

Added links to reviews of other projects.

chrisroane’s picture

chrisroane commented
Issue summary: View changes

Added a few projects I manually reviewed.

chrisroane’s picture

chrisroane commented
Issue tags: +PAreview: review bonus

Reviewed another three projects, and added the links to this issue. Added the bonus review tag.

sbrattla’s picture

sbrattla commented

I downloaded the latest release from GIT, and installed the module along with the now required Libraries module. However, something goes wrong.

[Tue Feb 14 21:59:20 2012] [warn] [client x.x.x.x] mod_fcgid: stderr: PHP Fatal error: Call to undefined function libraries_load() in /var/www/playground/public_html/sites/all/modules/splashify/splashify.display.inc on line 26

chrisroane’s picture

chrisroane commented

Thanks sbrattla for taking a look. You need to have the Libraries 7.x-2.0 installed. Currently it is in alpha release.

I looked into requiring a specific version of the libraries module, but you can't to it with development releases.

sbrattla’s picture

sbrattla commented

I did try with 7.x-2.0-alpha2, but that gave the same error?

chrisroane’s picture

chrisroane commented

sbrattla:

I received that same error when I was using a previous version of Libraries. You'll probably need to disable Splashify, upload the latest version (if it isn't already there), clear cache and then re-enable the Splash module.

I'm not getting this error on my local and dev server, so I'm pretty sure that is the problem.

venutip’s picture

venutip commented

I can confirm that the error does not appear with the 7.x-2.0-alpha2 version of Libraries. Got it with an older version of Libraries; started fresh and no problems.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus

manual review:

  • splashify_init(): you have a lot of ifs/elseifs in there. Use the switch() construct instead if you are checking the same variable that often.
  • drupal_add_js(): why do you set "'cache' => FALSE," on your javascript file?
  • splashify_display_splashtext(): bad alignment in the variable assignment block. Either bring all the "=" to the same column or don't do the alignment at all. Same in splashify_init(). And in others.
  • splashify_init(): do not check if the libraries module is present. You specified that as dependency in the info file already, so remove that code.
  • splashify_init(): don't check if the JS library is present in every page request. This should be done in hook_requirements() only.
  • "url(drupal_strip_dangerous_protocols(trim($paths[$next_index])))": no need to call drupal_strip_dangerous_protocols(), url() will do that for you anyway. And you are passing a path here, why do you think you need it?
  • splashify.info: if you require a minimum version of the libraries module you shoudl specify it, see http://drupal.org/node/542202
  • spotify_admin_where.js: shouldn't that also use Drupal.behaviors?
  • splashify_admin_main(): the help text should be implemented in hook_help().
  • "'#title' => 'Test Mobile',": all user facing text should run through t() for translation. Please check all your strings.
  • "$page_options = array('home' => t('Front Page'), 'all' => t('All Pages'), 'list' => t('List Pages'));": If the line declaring an array spans longer than 80 characters, each element should be broken into its own line.

Hm, it seems your code has changed while I'm writing this review, so not all points may apply anymore. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

chrisroane’s picture

chrisroane commented

klausi:

Thanks for the manual review. I did merge changes to the code I was making into the 7.x-1.x branch today, which is why it changed while you were making a review.

- splashify_int(): I have caching disabled for the JS file because it is very important that JS is not cached for the splash page to show up correctly. Otherwise the splash page could show up too often or not enough.

- Regarding "url(drupal_strip_dangerous_protocols(trim($paths[$next_index])))": The path may be a drupal path or a url. I removed drupal_strip_dangerous_protocols().

- In the code that is now merged in 7.x-1.x, I deleted spotify_admin_where.js...since I now use the integrated AJAX functionality.

- I'm not sure if you are seeing what I'm seeing, but in splashify_admin_main() I link to the help text. There is now very minimal text on that default page.

- Regarding "'#title' => 'Test Mobile': I caught this when I did a line by line review of the code this afternoon. I'm confident everything is using t() that should use it.

Everything else was fixed with the merge I did earlier. I committed the changes that you requested. Tomorrow my plan is to do more testing, do another line by line review of the code and make sure everything looks good.

Question: I created a new branch this week to work on changes to the code. I merged the changes today with the main branch and deleted the branch I was using for development. Is this the correct way in doing this? When I look at the commits, the branch that I was committing changes to is now not appearing on the page: http://drupal.org/node/1423456/commits .... which makes sense, since I deleted it. But I'm not sure if I'm doing this properly. Any tips on this would be appreciated.

chrisroane’s picture

chrisroane commented
Status: Needs work » Needs review

I did another code review and more testing. I determined that I didn't need to have the JS file included in hook_init(), have caching off. So caching is now enabled for that. I also ran the code through Coder and the PAReview utility and fixed any necessary updates.

Hopefully this gets the project very close!

chrisroane’s picture

chrisroane commented
Issue summary: View changes

Added another manual review.

chrisroane’s picture

chrisroane commented
Issue summary: View changes

Added a project review link.

chrisroane’s picture

chrisroane commented
Issue summary: View changes

Added a link to a project review.

chrisroane’s picture

chrisroane commented
Issue tags: +PAreview: review bonus

Did another in depth review of all the code and tested the system on my local server and a dev server. All changes are committed and ready to be reviewed.

I reviewed another three projects and added the review bonus tag to this issue..

cborgia’s picture

cborgia commented

Hi @chrisroane, when I use the git commend to clone the project I get a empty folder, any ideas why?

chrisroane’s picture

chrisroane commented

The master branch is supposed to be empty. The latest files are in the 7.x-1.x branch. Just run git checkout 7.x-1.x and that should get the correct branch.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

manual review:

  • splashify_admin_when_form_submit(): this is not a hook, it's a form validation handler. See http://drupal.org/node/1354#forms . Also elsewhere.
  • splashify_display_splashtext(): do not use "exit;", use drupal_exit() instead.

But that are just minor issues, otherwise this looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

chrisroane’s picture

chrisroane commented

Thanks klausi and everyone else who tested this module.

I applied the updates to the comments and updated the splashify_display_splashtext() exit code like you suggested.

chrisroane’s picture

chrisroane commented
Issue summary: View changes

Added a link to another project review.

chrisroane’s picture

chrisroane commented
Issue summary: View changes

Added a link to another review.

chrisroane’s picture

chrisroane commented
Issue summary: View changes

Added a new review link.

chrisroane’s picture

chrisroane commented
Issue tags: +PAreview: review bonus

Okay, I did another three manual reviews and added the bonus tag. I'm hoping this is able to make this project a full project! :)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » tim.plunkett

Assigning to tim.plunkett, as he said he might have time to review this.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Fixed

This looks great, very well written. Two points:

  • There are inline comments that use /* ... */, those should never be used inside a function
  • There is no reason to use $(window).load(function(){ inside Drupal.behaviors.splashify.attach

That said, welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

chrisroane’s picture

chrisroane commented

Thanks Tim! I appreciate you giving me the upgraded user permissions!

- I will update the comments like you specified.

- Regarding $(window).load()....I had several issues where the JS inside of Drupal.behaviors.splashify.attach was being hit multiple times (specifically when using the colorbox option). I am open to other ways of doing this, but for the time, this appears to work for all methods.

tike012’s picture

tike012 commented

Congrats Chris! I look forward to testing this out soon!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Added a new review link.