[D7] CSS Reset

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

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.

Hi Daniel,
Thanks for submitting this application. I'm not aware of such CSS resets modules either, and the aim of this module is clear and unique!

I have found a few points, but none of them are blocking issues. However, it's always good to clean them up to prevent buggy behavior and for best practices.

- Instead of using hook_init(), use hook_page_build() to add the CSS reset file. hook_init is removed in D8, and in D7, hook_init is invoked on Ajax pages as well, but not cached. hook_page_build is perfect to add CSS files because the output can be cached, and Drupal does not invoke it unless the page is serving full HTML.

- This modules does not define any PHP classes. It's not necessary to use the files[] in the module .info file to explicitly define them. It only adds some overhead to the code registry scanner.

It's not necessary, but it can improve readability (IMHO) - store the "drupal_add_css" options in a $options variable.

$options = array(
'group' => CSS_SYSTEM,
'every_page' => TRUE,
'media' => 'all',
'preprocess' => TRUE,
'weight' => '-1001'
)
drupal_add_css($path . '/' . 'meyer.min.css', $options);

Attached patch for code formats & a tag issue

File : css_reset.module

Function : clear_cache_action
Please add comments incase you are using any drupal core functions.

Please fix code format issues

Line 48: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms drupal_add_css($path . '/' . 'meyer.min.css',

File : css_reset.admin.inc

Please do not use a tags, use l() function to generate links.

Please fix code format issues

Line 24: Use an indent of 2 spaces, with no tabs
'#type' => 'checkbox',
Line 25: Use an indent of 2 spaces, with no tabs
'#title' => t('Enable Reset library site wide'),
Line 26: Use an indent of 2 spaces, with no tabs
'#default_value' => variable_get('css_reset_sitewide', FALSE),

Please add git clone link :)

@2dareis2do I mean add comment for function clear_cache_action ( if u agree :) )

Hi,
Fix all pareview issues.

Manual Review

-> Fix git url in your git clone command;

-> * Implements hook_help()
It is recommended to get the basic understanding of the module to new users.

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

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

This module appears to work as intended. As you stated above, there is a warning being thrown by pareview. Whether it's a false positive or not, I think you might need to check with the developers on IRC as to what they think. From what I understand, you are better off doing it the way you're doing it. Also, it's a warning and not an error. I don't know how strict those with approval permission are, however. You should check on IRC or something.

Also, I think included the Meyer reset.css might also violate the 3rd party assets/code requirements. Again you might want to check on IRC.

Lastly, I think this module would be even nicer if it just supported any custom css file created and uploaded into the css_reset directory in the library. Meyer's reset CSS is great, but I could see other applications where you'd want to use something else/something custom.

Automated Review

There is one warning in the code:
http://pareview.sh/pareview/httpgitdrupalorgsandboxhenryw2496335git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes: Does not cause module duplication and/or fragmentation.

Master Branch
Yes: Does follow the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
No: I could be wrong here, but I think by adding the meyer reset to your module goes against this requirement, where you should just point people to where to get it in your documentation (which you have also done). I appreciate that you added the file into your module to make it easier for the suer, but I believe this puts it in violation.

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
Yes: Meets the security requirements.

Automated Review

No issues identified http://pareview.sh/pareview/httpgitdrupalorgsandbox2dareis2do2275019git

Manual Review

Individual user account

Yes: Follows.

No duplication

Yes.

Master Branch

Yes: Follows the guidelines for master branch.

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.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements

Coding style & Drupal API usage

css_reset.test

1. Update the documentation for the testCssResetMenus() test. Seems it was copied from the js_example module.
   

     /**
      * Tests the menu paths defined in js_example module.
      */
     public function testCssResetMenus() {
       $paths = array(
         'admin/config/media/css-reset',
       );
       foreach ($paths as $path) {
         $this->drupalGet($path);
         $this->assertResponse(403, '403 response for path: ' . $path);
       }
     }

2. (*) Do you really this that test is needed? I'd say it's not since it isn't testing any module functionality. It could at least check if the CSS file is being added. Consider removing it or updating it.

css_reset.module

1. (*) You don't need to return anything on this hook. Remove the last lines
   

   /**
    * Implements hook_page_build().
    */
   function css_reset_page_build() {
     $path = libraries_get_path('css_reset');
     // Needs to be in the CSS_SYSTEM group rather than the CSS_DEFAULT group.
     $options = array(
       'group' => CSS_SYSTEM,
       'every_page' => TRUE,
       'media' => 'all',
       'preprocess' => TRUE,
       'weight' => '-1001',
     );
     // drupal_add_css($path . '/reset.css', $options);
     /*Do not use drupal_add_css() in hook_page_build(), use
     | | #attached on the $page render array instead */
     $page['#attached']['css'][$path . '/reset.css'] = $options;
     // dprint_r($page);
     return drupal_render($page);
   }
   

css_reset.install

1. (*) Based on how you're including your css file. This should not be necessary any more.

   /**
    * Implements hook_install().
    */
   function css_reset_install() {
     db_update('system')
       ->fields(array('weight' => -1001))
       ->condition('name', 'css_reset', '=')
       ->execute();
   }
   

CSS files included

1. (*) Although it follows the licensing standard, it's also using the library api. So there is no need to have these on the module.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Seems that is almost ready for being published. Only one major issue found.

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.

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.