[D7] Favicon Generator

Hai,
- please add prefix in your title of Issue Post with [D7]
- change git clone with:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/johncook/2378569.git favicon_generator
your current code for cloning git is only for you as maintener, not for public.

My review use bartik theme.

1. In file favicon_generator.info
- Add this line:
configure = admin/config/user-interface/favicon_generator
- You don't need define package if you set "package = Other". You should remove it.
2. in file favicon_generator.module inside function favicon_generator_theme_form_submit()
- Your code don't anticipate if $settings['favicon_path'] is empty string. It makes
image_get_info() return FALSE. Not all themes provide input for favicon_path.
3. Founds to many issue of coding standard in
http://pareview.sh/pareview/httpgitdrupalorgsandboxjohncook2378569git

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.

Done

There are multiple TODOs in this module that you mentioned.
One of most important one is related to access.

Please work on these issues and then submit for review.

for review by me

Manual Reviews

- Secure code
(*) Your admin functions are accessible by anon users because the hook_menu() has 'access callback' set to TRUE. Don't use this entry unless you provide your own access callback function.

$items['api/favicon'] = array(
    'title' => 'favicon_generator',
    'page callback' => 'favicon_generator_json_return',
    'access callback' => TRUE,
    'type' => MENU_CALLBACK,
  );

- Coding style & Drupal API usage
(+) All of your variables should be removed by variable_del() function once uninstall

Automated Review

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

./favicon_generator.api.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming

function hook_favicon_generator_admin() {
function hook_favicon_generator_spec(array $settings) {
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

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: 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

[No: Does not follow] 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 issues identified.]
line no 146 , $r = json_decode($response->data); since it is coming from the 3rd party, it needs to sanitized accordingly. For more info Check Santization functions

Coding style & Drupal API usage

1. (*) favicon_generator_theme_form_submit in favicon_generator. module is doing validation for favicon image size, This should be in the validation function and form_set_error() can be used
2. (*) Form #title for every form (.module, aa) should be enclosed by t() so that it can be translated
3. (*) Rename file favicon_generator.api.inc to favicon_generator.api.php

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.

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.

@Sagar: "$r = json_decode($response->data);": is not a XSS security issue by itself, since no data is printed to HTML in this line. You only need to sanitize user provided data when actually printing something.

Thanks, Now i read it carefully. It is not being used anywhere for printing.

Hey John,

Nice work on the module, I actually have been doing all of this manually in Drupal, and wrote about this on my blog post, when someone mentioned you are creating this module.

My cursarely look over the module:

• Why are you using the default favicon path in the appearance admin screen, rather than a dedicated form upload in your admin settings screen? To me this seems like a more natural place for it to be. I mean the configure link does point to it from the modules page, yet you cannot upload the favicon? You can maybe do a form alter on the appearance admin screen to mention the favicon is now being controlled by favicon_generator (and link to it).
• Why introduce 4 sub modules into the equation? Having to install 5 modules is too much for what should be a lightweight API that for the most part does nothing, here there are a lot of files and a lot of code included on every page request. My thoughts - have a single module, with includes for the 1 time use code (e.g. the favicon fetching and downloading code). The alter hooks are fine if you want to support further contrib modules down the path (ideally they would be merged in).

I am also keen to help with the development of the module, if you need the help.

Sean

> * Implements hook_settings().

Hasn't been a hook since something like Drupal 4.7.

> * @author JohnCook

Our coding guidelines recommend not putting author names in code -- this can deter future contributors. Credit will be in the git history.

> * Implements hook_form_submit().

That's not a hook either. Check the documentation at api.drupal.org ;) That should say something like 'Additional submit handler for the yada yada form'. ('Additional' since you're adding it in with form alteration).

        $path = dirname(__FILE__) . '/README.txt';
        if (file_exists($path)) {
          $readme = file_get_contents($path);
        }

Your code should probably know the contents of its own module folder... ;)

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: 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 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.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance.

1. If I change the module settings page (ie change background colour) after uploading my favicon file to the theme settings page, the favicons aren't regenerated. I need to go back to the theme settings page and re-save. Perhaps just adding a message to this affect when saving the module settings page will save some people a bit of confusion.

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.

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.

Hi

These are some suggessions which must be fixed,

Readme file
1. Spell check, change "moule" to "module".

Module file
1. Remove blank lines.
3. It would be better if you would add a configuration form for getting the api key and its settings values from site admin.
4. Remove unused variables from the code.
5. In the sub modules please remove unwanted blank lines from code.

favicon_generator.css file
1. Line length must not exceed 80 characters.

Please use http://pareview.sh for reviewing.

Thanks

Great module! I can see using this a lot.

Here are some recommendations and observations:

1. Your documentation says the files should be 260px or bigger, but when I saved the file it says "Your icon should be at least 300x300 for optimal results." and yet in the code there's another message that says "270x270".
2. I get ambiguous messaging when there's an error. In Watchdog it showed that the file I referenced didn't exist, but the error message displayed to me seemed generic and did not tell me the file could not be found.

Hi John

I have a suggession that when we uninstall the module the icon generated must be deleted along with the changed settings of default icon, since the generated icon is used for all the devices. I think it would be better to use the default icon if the module is not used in the site.

Thanks

Hi John, how is progress on this? I could review it for you when ready.

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.

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: 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
[No: Does not follow] 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]
Coding style & Drupal API usage
(*) Line 106: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_dsm]
drupal_set_message($result, 'error');

This review uses the Project Application Review Template.

Found issues on Click here

Hi John,

(*)Line 106: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_dsm]
drupal_set_message($result, 'error');

Hi, please check your code in here.
http://pareview.sh/pareview/httpgitdrupalorgsandboxjohncook2378569git
You have a some issues. Thank's.

That minor coding standard errors are surely not application blockers, anything else that you found or should this be RTBC instead? Please do a real manual review.

@klausi, ok, sorry.

Hi,

I evaluated the module, and I have a few remarks.

These remarks a purely informational, and they should not block the application from passing.

1. In the _favicon_generator_download_and_save_icons function, you decode the response before you check if the response has errors, I would swap this, as it now is pretty confusing.

2. Avoid using variables named $r, they hold no information.

3. Improve inline documentation.

4. Non hook functions should be prefixed with an underscore.

function favicon_generator_theme_form_submit($form, &$form_state) {

5. There are a lot of coding standard issues. They are easy to resolve, so I suggest you do so.

I have no further comments. I think this module is ready for release.

Apologies for the months-long delays in this, when it had clearly passed relevant reviews long ago. Thanks for your contribution! Congratulations, you are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to 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 (here you get to ask fans of your project to help you!)

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.