[D7] Accessible Exit Disclaimer

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

Automated Review

[Best practice issues identified by pareview.sh / drupalcs / coder. These test results can be found here and I discussed a few of them below.

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Maybe: Causes] module duplication and/or fragmentation. The External Links module provides a similar exit confirmation message to what this module does, but the module does a lot more as well, so I do not think that this specifically tailored solution is bad for the community. Additionally, I work in government, but am not a 508 guy, so I cannot speak to the accessibility of External Links. This could go a long way on its own.

Master Branch

[Yes: Follows] the guidelines for master branch, however pareview.sh sees this as being not true. The 7.x-1.x branch likely still needs to be set as default despite being the only one.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[No: List of security requirements identified.] At first glance, your code is spot on with making sure input is secure. The only thing you might look into doing is wrapping your aed_message variable (line ~160 in the module) in filter_xss or filter_xss_admin as this field can be editied in full HTML. While its unlikely that an attacker would sabotoge your system by getting to the administrative side first and then content editing, you could be opening the door for accidents. This is not a show stopper.

Coding style & Drupal API usage

[No identified code issues.] Code looks great. Fantastic job with the comments. Readability is top notch and nothing is overly complex when it doesnt have to be. I'd like to see more commit history than a single initial commit, but overall, great.

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

This review uses the Project Application Review Template.

A couple of other small notes:

1. In the admin interface you explicitly spell out 15 seconds for the time to auto-redirect. I'd suggest taking this number out as it could be confusing to users who place a custom value in there. They might be inclined to believe, without looking at code, that it's always 15 seconds no matter what. This is line 156 of accessible_exit_disclaimer.settings.inc.
2. On line 306 of js/accessible_exit_disclaimer.js you are using jQuery's on() function which doesn't exist before jQuery 1.7 and as you know, this makes that version a requirement. Drupal ships with jQuery 1.4.4 so this functionality is completely broken unless you're using a module like jQuery Update. As a suggestion, you might add this module as a dependency or at least a heavy recomendation in your project description. As an alternative to that, you might investigate other more backwards compatible functions to replace on() and any others that might not work. This isn't a show stopper either, but its a major annoyance as jQuery version conflicts have killed many a developer.
3. Last, but not least, line 187 in accessible_exit_disclaimer.module should be removed. It renders the conditional statement right after it (lines 189-191) completely irrelevant and causes the elements in the $output variable to be rendered on all pages including admin pages.

Fantastic work and fantastic submission. I might very well use this module in some of my projects.

Hi ,

Still I can see a lot of warning is pareview : https://pareview.sh/node/370, please fix those.

Thanks
anup.singh

Automated Review:

Review of the 7.x-1.x branch (commit 004ea71):

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

Manual Review:

1. In accessible_exit_disclaimer_help function, please define the variable $output outside the switch case, otherwise it will give an error as undefined variable.
2. In the function theme_accessible_exit_disclaimer, I can see you are using the following
if (!path_is_admin(current_path())) {
return $output;
}
But I can see you are defining the output and then not returning the output if the path is admin. You can add this line above the function to avoid processing the output altogether for admin paths.

@vaibhavdev: nice tips, but those alone are surely not application blockers. Anything else that you found or should this be RTBC instead?

@klausi: i think this is not a application blocker and can be moved as RTBC

Hi alsnyder,

I really like this module and will use it as soon as it is fully functional.

My manual review.

1. On settings page.

Notice: Use of undefined constant css_path - assumed 'css_path' in accessible_exit_disclaimer_settings_validate() (line 191 of /sites/all/modules/accessible_exit_disclaimer/accessible_exit_disclaimer.settings.inc).

To reproduce. Install module. Goto admin settings. Leave CSS path empty. Click save.

2. Javascript attaches to all links on page.
3. Links without http or https redirect to Page not found on site.
4. Links with target attribute do not behave as expected. For example target="_blank" does not open the link in a new window.

Alsnyder,

2. I commented on this because internal links were also triggering the JS. I like the idea of your option to filter that you have added.

3. You could also use the base_url variable and pass that to J's to determine what is an internal vs external URL.

4. Can you make the script still execute, but in a new new tab/window? This is possible with JS and I believe is very useful.

I will review the module again.

Git clone failed for https://git.drupal.org/sandbox/alsnyder/2822863.git while invoking http://pareview.sh/pareview/httpsgitdrupalorgsandboxalsnyder2822863git

Git clone failed. Aborting.

I'm a robot and this is an automated message from Project Applications Scraper.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.