[D7] Tile

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

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 drupalfan79!

I had a look at your project.

- At first the git-repository in the issue-description is your private one, use "git clone http://git.drupal.org/sandbox/drupalfan79/2089683.git tile" instead.

- The path to the theme-settings in D7 should be "admin/appearance/settings", not "admin/build/themes/settings" as explained in your README.txt.

- Feel free to also add the line "configure = admin/appearance/settings" to the .info-file, to add a configure-shortcut next to your module in Drupal's module-list.

- On the theme-overview-page tile_get_theme() returns "settings", because this form is not for a specific theme, which leads to 3 notices:

Notice: Undefined index: setting in theme_get_setting() (line 1431 of includes/theme.inc).
Notice: Trying to get property of non-object in theme_get_setting() (line 1468 of includes/theme.inc).
Notice: Trying to get property of non-object in theme_get_setting() (line 1478 of includes/theme.inc).

On saving the form the notices get duplicated, but data gets saved, so you only should get rid of these notices.

- Another error lies somewhere within the paths of uploaded images. I didn't look through why this error occurs, but after uploading an image and saving the theme settings, the meta-tags are set correctly. If I then save the form again, the 4 meta-tags "msapplication-square70x70logo", "msapplication-wide310x150logo" etc. disappear.

- Overall some additional inline-comments could enhance the readability of your code.

Anyway you did a good job creating this module, maybe you could think about working together with @konstantin.komelin on pinned_site (https://drupal.org/project/pinned_site) somewhere in the future, that would be a cool merge.

regards,
Martin

update git repository

Hi, I took a look at your code.

At first I ran Coder, it came up with one error:

tile.js
comment_docblock_fileFile: @file block missing (Drupal Docs) [comment_docblock_file]

Then I quickly scanned your .module file, there are zero inline comments! I'm sure they are needed somewhere.

Then I read your code, and have some questions for you which you should answer in your code.

1. What is happening in <?php function tile_get_theme()?
2. Line 50/51 you are doing something with a tile path... why? What are you doing there? I have to carefully read it to figure it out,
   As with #1 you use rather vague variables like $i and $j, why not use $first_occurance, $last_occurance?
3. What happens when the file is not a gif/png/jpeg?
4. The file stuff is a bit hard to read due to missing comments. The way I read it: if ( file is uploaded and valid ): move the file and do create images; otherwise: Do nothing with the file but create new images anyway Please add comments as to why this happens

Some things became clear when I saw the frontend, but what if the frontend is not working for someone and someone has to work her way through ondocumented code to figure out where the error is coming from.

Furthermore:

• You have a file form which only accepts gif/jpeg/png, why not 'hard-code' it into the HTML? You could use #attributes This will make it easier for your users to find the file.
• You handle form submission in the form validation, yet you seem to do no validation (apart from the file stuff)

And I'm sure it works, I can't test it though as I use linux.

Hi drupalfan79,

I did a manual review of your module all looks fine, accept a few minor cosmetic suggestions.

The function 'tile_get_theme()' can be documented bit more for the logic used.
Please consider using short but little descriptive names of variables instead of $i and $j.

For the function 'tile_create_images', first the block comment of the function has typo 'lite', instead of 'tile' I believe. Any reason why it is restricted to 4 different size images, if the user wants more?
Here also please consider changing $i to a meaningful name. This actually helps to understand the code even if comments are not there. :)

Rest all looks fine.

Thank you for reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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. You have to get a review bonus to get a review from me.

manual review:

1. Commit messages are broken, you are always using the text "Initial" http://drupalcode.org/sandbox/drupalfan79/2089683.git . Please provide meaningful messages, see https://drupal.org/node/52287
2. The Git commits are not connected to your user account. You need to specify an email address. See http://drupal.org/node/1022156 and http://drupal.org/node/1051722
3. start screen of windows 8.1? How does that work, what does the module do? Please improve your project page, see https://drupal.org/node/997024

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

Assigning to chx as he might have time to take a final look at this.

I did review the code; it looks okay; although there's a bug: the file is moved but the $file object is not updated with the new uri and is not resaved and file usage is not added. Other than this bug, it's OK, so

Thanks for your contribution, drupalfan79 !

I updated your account so you can 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 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.

Thanks to the dedicated reviewer(s) as well.