[D7] Mobile Cache

You still have a master branch so remember to get rid of it and correctly set up a default branch. Information on how to do both of these can be found here and here. Remember to remove the version data in the .info file, Drupal packages that automatically. Code snipper also found some problems:

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
62 | WARNING | Line exceeds 80 characters; contains 85 characters
79 | WARNING | Line exceeds 80 characters; contains 81 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mobile_cache.module
--------------------------------------------------------------------------------
FOUND 65 ERROR(S) AND 7 WARNING(S) AFFECTING 58 LINE(S)
--------------------------------------------------------------------------------
4 | ERROR | The third line in the file doc comment must contain a
| | description and must not be indented
8 | WARNING | Line exceeds 80 characters; contains 83 characters
19 | ERROR | Concat operator must be surrounded by spaces
19 | ERROR | Concat operator must be surrounded by spaces
24 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
24 | ERROR | Function comment short description must start with a capital
| | letter
24 | ERROR | Function comment short description must end with a full stop
29 | ERROR | Array indentation error, expected 4 spaces but found 6
30 | ERROR | Array indentation error, expected 4 spaces but found 6
31 | ERROR | Array indentation error, expected 4 spaces but found 6
32 | ERROR | Array indentation error, expected 4 spaces but found 6
33 | ERROR | Array indentation error, expected 4 spaces but found 6
41 | ERROR | Expected 1 space after closing parenthesis; found 0
42 | ERROR | There must be no space between the Array keyword and the
| | opening parenthesis
48 | ERROR | There must be no space between the Array keyword and the
| | opening parenthesis
49 | ERROR | Array indentation error, expected 4 spaces but found 6
50 | ERROR | Array indentation error, expected 4 spaces but found 6
51 | ERROR | Array indentation error, expected 4 spaces but found 6
52 | ERROR | Array indentation error, expected 4 spaces but found 6
53 | ERROR | Array indentation error, expected 4 spaces but found 6
54 | ERROR | Array indentation error, expected 8 spaces but found 10
55 | ERROR | Array indentation error, expected 12 spaces but found 14
56 | ERROR | Array indentation error, expected 8 spaces but found 10
57 | ERROR | Array indentation error, expected 4 spaces but found 6
59 | ERROR | There must be no space between the Array keyword and the
| | opening parenthesis
60 | ERROR | Array indentation error, expected 4 spaces but found 6
61 | ERROR | Array indentation error, expected 4 spaces but found 6
62 | ERROR | Array indentation error, expected 4 spaces but found 6
63 | ERROR | Array indentation error, expected 4 spaces but found 6
63 | WARNING | Avoid backslash escaping in translatable strings when
| | possible, use "" quotes instead
64 | ERROR | Array indentation error, expected 4 spaces but found 6
68 | ERROR | Array indentation error, expected 4 spaces but found 6
74 | ERROR | Missing function doc comment
74 | ERROR | Expected 1 space after closing parenthesis; found 0
75 | ERROR | Inline comments must start with a capital letter
75 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
79 | ERROR | Expected "foreach (...) {\n"; found "foreach (...){\n"
80 | ERROR | Expected "if (...) {\n"; found "if (...){\n"
85 | ERROR | Expected "if (...) {\n"; found "if (...){\n"
92 | WARNING | Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
92 | ERROR | Function comment short description must start with a capital
| | letter
92 | ERROR | Function comment short description must end with a full stop
94 | ERROR | Expected 1 space after closing parenthesis; found 0
98 | ERROR | Whitespace found at end of line
103 | ERROR | Whitespace found at end of line
105 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
108 | ERROR | Whitespace found at end of line
109 | ERROR | Inline comments must start with a capital letter
110 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
112 | ERROR | Whitespace found at end of line
113 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
114 | ERROR | A cast statement must be followed by a single space
115 | ERROR | Whitespace found at end of line
117 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
125 | ERROR | Function comment short description must be on a single line
129 | ERROR | Last parameter comment requires a blank newline after it
129 | ERROR | Invalid @param data type, expected bool but found boolean
129 | ERROR | Missing comment for param "$setcookie" at position 1
130 | ERROR | Return comment must be on the next line
134 | WARNING | Line exceeds 80 characters; contains 96 characters
134 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
134 | ERROR | Comments may not appear after statements.
143 | ERROR | Whitespace found at end of line
146 | WARNING | Line exceeds 80 characters; contains 95 characters
146 | ERROR | Inline comments must start with a capital letter
148 | WARNING | Line exceeds 80 characters; contains 140 characters
148 | ERROR | No space before comment text; expected "// setcookie("device",
| | $data['device'], REQUEST_TIME+7200, $params['path'],
| | $params['domain'], $params['secure'], $params['httponly']);"
| | but found "//setcookie("device", $data['device'],
| | REQUEST_TIME+7200, $params['path'], $params['domain'],
| | $params['secure'], $params['httponly']);"
159 | ERROR | Function comment short description must end with a full stop
164 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
170 | ERROR | Function comment short description must be on a single line
173 | ERROR | Missing comment for param "$str" at position 1
184 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
--------------------------------------------------------------------------------

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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 acy76,

I've had a quick look at the return error its throwing your way. Have you tried changing :

 /**
  * Device detection using Mobile_Detect class - stores results in cookie.
  *
  * @param bool $setcookie
  *   Boolean to trigger setting cookie.
  *
  * @return array $data
  *   Array containing device information.
  */

to something like this:

 /**
  * Device detection using Mobile_Detect class - stores results in cookie.
  *
  * @param bool $setcookie
  *   Boolean to trigger setting cookie.
  *
  * @return array
  *   Array containing device information.
  */

I dont think the return normally needs to be given a name...

Hi guys,

Before i will check the code please fix the issue.
yanniboi you forgot to put back the status of the project to "needs work". This is important for the owner to see what to do.
Please check how to Apply for full projects.

Fix the coding standard issue.

Thanks balintcsaba!

Coder Review

mobile_cache.module

Line 114: the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead [security_12]
$request_string = substr($_SERVER['REQUEST_URI'], -8);
Line 179: the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead [security_12]
if (isset($_SERVER['REQUEST_URI'])) {
Line 180: the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead [security_12]
if (strpos($_SERVER['REQUEST_URI'], '?') === FALSE) {
Line 181: the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead [security_12]
$_SERVER['REQUEST_URI'] .= ('?' . $str);
Line 184: the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead [security_12]
$_SERVER['REQUEST_URI'] .= ('&' . $str);

1) Try deleting all variables after uninstall the module. You should implement hook_uninstall() and on it to try implementing the hook_uninstall() and delete persistent variables that are comming from the settings.

2) I think hook_requirements() will be good place to put dependancies for '/sites/all/libraries/Mobile_Detect/Mobile_Detect.php'

Looks good enough. You may want to check out hook_libraries() and the rest of Libraries API - that will let you add the files you need without having to hardcode the path in _mobile_cache_detect(). Also PHP's parse_url funciton and http_build_query are much easier for manipulating url strings.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Instead of calling mobile_cache_preboot() from settings.php, can you use hook_boot() instead?

----
Top Shelf Modules - Crafted, Curated, Contributed.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

adding 1 review bonus project

current module (sandbox version) is working nicely for me, would love to have this module approved

Please don't RTBC your own issues, see the project application workflow: https://drupal.org/node/532400

Hi, acy76, very nice and usefull module. But here are some suggestions.
1) In my case it didn't work out of the box.
This code didn't move me to mysite.com?device=1

  $_SERVER['REQUEST_URI'] .= $str;

In my case $_SERVER['REQUEST_URI'] on front page is equal to '/', so in fact I get $_SERVER['REQUEST_URI'] = '/?device=1';
And no url change. No change no cache rebuild for mobile or tablet.

2) Also we could make some enhancements by adding this code

function _mobile_cache_add_query_string($str) {
  if (empty($_GET['device'])) {
    if (strpos(request_uri(), '?') === FALSE) {
      $str = '?' . $str;
    }
    else {
      $str = '&' . $str;
    }
    // Make possible to use on non apache servers.
    // Code taken from here api.drupal.org/api/drupal/includes!bootstrap.inc/function/request_uri/7
   if (isset($_SERVER['REQUEST_URI'])) {
      $_SERVER['REQUEST_URI']  = '/' . ltrim($_SERVER['REQUEST_URI'] . $str, '/');
      header("Location: " . request_uri());
    }
    else {
      if (isset($_SERVER['argv'])) {
        $_SERVER['argv'][0] = '/' . ltrim($_SERVER['argv'][0] . $str, '/');
      }
      elseif (isset($_SERVER['QUERY_STRING'])) {
        $_SERVER['QUERY_STRING'] = '/' . ltrim($_SERVER['QUERY_STRING'] . $str, '/');
      }
      else {
        $_SERVER['SCRIPT_NAME'] = '/' . ltrim($_SERVER['SCRIPT_NAME'] . $str, '/');
      }
    }
  }
}

But anyway it is a great module. Thank you! I hope soon it will be used on some projects. After adding some small changes and testing it is RTBC for me, thanks!

Well, all my points from #14 and #15 still apply:

• Using Libraries API would allow your users to install the Mobile Detect library wherever was best on their site.
• parse_url() and http_build_query() would make some of the functions a bit cleaner.
• Using hook_boot() (which is called for all page requests during Drupal's bootstrap phase) is probably a much cleaner way to issue the redirect to the device-specific url.
• Additionally, replacing the magic numbers 0, 1, 2 with defined constants would make the logic much easier to understand.

But none of those are blocking issues.

Thanks for your contribution, acy76!

I updated your account to let you 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 get 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.

----
Top Shelf Modules - Crafted, Curated, Contributed.