Module Description:
This module for Drupal 7 replaces the default logo and favicon images with less-intrusive alternatives. The goal is to reduce the amount of boilerplate configuration (or theming) sitebuilders must do at the beginning of a project to make it presentable to clients. Theme settings are not altered, the markup that would otherwise be used is simply modified prior to being rendered.

Configuration settings are available on an admin-accessible configuration page. Options include selecting the image/style to be used (from a small selection of module-included images), leaving the favicon as is, leaving the logo as is, and bypassing the changes provided by the module altogether without disabling the module.

The project page is: drupal.org/sandbox/fatleaf/1564464

Git repository: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/fatleaf/1564464.git

Reviews of other projects

Thanks!

CommentFileSizeAuthor
#28 drupalcs-result.txt789 bytesklausi

Comments

mantish’s picture

mantish commented
Status: Needs review » Needs work

Hey Fatleaf,

I could see some indentation errors in your project. The list of the errors are here . You can check and verify the errors at the same link after you push.

Regards

andrewkamm’s picture

andrewkamm commented

Mantish, thanks for your feedback. I've addressed all issues reported by that code review tool and the pushed the updated code up to the Drupal.org sandbox. The report is now clean.

Thanks, ak

mantish’s picture

mantish commented
Priority: Normal » Major

Hey Fatleaf,

I have reviewed your code manually and looks clean to me, excellent work, but could you tell me how different this is from existing feature ? I could go to "admin/appearance/settings" and check "use the default logo" or "Use the default shortcut icon" or even upload a new images. Functionality wise i don't find any difference. Correct me if i am wrong. Please change the status to needs review once you make any changes and want your work to be reviewed.

Cheers,
Mantish

andrewkamm’s picture

andrewkamm commented
Status: Needs work » Needs review

Hi Mantish,

You're correct, most of what this module provides can be achieved on the theme settings page (though I'm including "FPO"-style images so that developers have some benign-yet-nice looking imagery ready-to-use; I'll be adding some more of these to the repository this week). And I don't want to replace or compete with the theme settings functionality. In developing this module I made a point not to have it alter any actual theme settings, but rather just to alter what's used for generating markup before it's sent to the user.

This module is intended as a convenience for developers to use during development and prototyping, so they can be up and running quickly with a generic, ready-for-client-eyes instance of Drupal. As such, I don't plan to incorporate capabilities like uploading of custom images.

(Note: while I do provide some fine-grained controls, like "ignore favicon" and "ignore logo", they are intended to help out with edge cases I haven't thought of yet and maybe debugging a theme-level issue.).

The motivation for me to write this came from having to mess with theme-level settings (and a point-and-click UI) whenever I set up a Drupal instance for development. Of course, if all I want to do is toggle logo and favicons off, it doesn't take very long to modify the theme settings, but it takes even less time to install a module with Drush (and installing a bunch of modules with Drush is part of my routine for a new Drupal instance anyway). If I want a decent looking, nondescript, FPO-placeholder image, that might take a while or might not otherwise happen.

Thanks again, AK

bartlantz’s picture

bartlantz commented

Hi fatleaf,

The module works as expected. I tested it on a fresh drupal 7 installation.

A couple suggestions:

1. I do not know if this is official drupal coding standards or just a suggestion, but I would suggest you rename the following internal helper function names so that they begin with an underscore:
sans_druplicon_image: _sans_druplicon_image
sans_druplicon_whitelist: _sans_druplicon_whitelist
sans_druplicon_help_text: _sans_druplicon_help_text

2. Finding the configuration page was a bit confusing at times, although there is a link from the modules page which is good. However, if you try to find the configuration page from the appearances menu it is hard to see the link.

Perhaps adding a link at:
Home » Administration » Configuration » User interface » Sans Druplicon
would help?

Thanks,
Bart

saitanay’s picture

saitanay commented
Priority: Major » Normal
andrewkamm’s picture

andrewkamm commented

Hi Bart,

Thanks for your review and feedback. I've prefixed the internal function names with an underscore as you suggested. I left the menu item in its current location (Appearances » Sans Druplicon) to avoid nesting it further, plus in my bare-bones D7 development environment there isn't already a User Interface menu item under Administration » Configuration. However, I agree that could be a good spot for a Sans Druplicon link.

Thanks! AK

andrewkamm’s picture

andrewkamm commented

Additional changes made to this module since the original posting of this thread include:

  • Added a few new images from the NounProject, and changed the default image used by the module. The images added are representative of "development" (gears) per the intended usage of this module. (public domain or CC0 licensed)
  • Added notice message at admin/appearance/settings (and children) that informing users when their logo and/or favicon is being overridden by Sans Druplicon. The message includes a link to "disable it" (admin/modules) and a link to "alter its configuration" (admin/appearance/sans_druplicon).
  • Fixed configuration variables set when installing module so logo/favicon overrides are enabled by default.
  • Changed favicon images that are associated with white logo images to black. (white favicons are not very useful)
  • Fixed access arguments value within implementation of hook_menu().
  • Various code/text formatting adjustments for adherence to Drupal standards.
marshmn’s picture

marshmn commented
Issue tags: +PAReview: GPL Issue

Hi,

You mention above that you have included some icons from the NounProject which are licensed under a Creative Commons license? I'm not sure if this is allowed under Drupal rules since I believe everything which is commited as part of a project has to be licensed under GPL and I'm not sure that this is compatible with Creative Commons? I'm going to tag this issue with the "PAReview: GPL issue" tag and hopefully someone will confirm whether this is a problem or not.

A few points from a manual review:

1) You still seem to have a master branch in addition to your 7.x-1.x branch - you should switch the default branch to be the 7.x-1.x branch (see: http://drupal.org/node/1659588) and delete the master branch (see: http://drupal.org/node/1127732).

2) There are a few places where you don't appear to be wrapping strings with t() such as setting of $msg on line 46 and setting the help text on line 300.

andrewkamm’s picture

andrewkamm commented

The icons selected from the Noun Project are either Public Domain or CC0 (Public Domain Dedication). There's a note about this in the README as well. The CC0 license is distinct from other CC licenses in that it is used to dedicate your work to the public domain.

The person who associated a work with this deed has dedicated the work to the public domain by waiving all of his or her rights to the work worldwide under copyright law, including all related and neighboring rights, to the extent allowed by law.

You can copy, modify, distribute and perform the work, even for commercial purposes, all without asking permission. ... (from http://creativecommons.org/publicdomain/zero/1.0/)

Also, while not required under CC0, I've named the image files in a manner that makes their source and license available. For example, gear-cc0-nounproject-1241-blue.png is Noun Project image 1241 licensed under CC0.

Regarding the manual review items:

  • Item #1 (branches): I'll review the Git branching soon and post an update if anything changes.
  • Item #2 (wrapping strings with t()): The two examples cited are actually being wrapped with t() when used, just not when declared. This is to make the code easier to read/edit in the future and better accommodate various Drupal code-formatting standards.

Thanks for the review, ak

andrewkamm’s picture

andrewkamm commented

Hi marshmn,

I just took care of the branching item you mentioned. The default branch is now set to 7.x-1.x and I've removed the master branch entirely.

thanks again, ak

patrickd’s picture

patrickd commented

t() must be used on string declaration otherwise the system on localize.drupal.org is not able to find them and therefore not able to translate them.

correct:

$string = t('Look at me! I'm translatable!');
...
print $string;

false:

$string = 'I am not!';
...
print t($string);
andrewkamm’s picture

andrewkamm commented

patrickd: Thanks for the example! I didn't realize t() had to be used during the initial assignment. I've corrected the invalid usage and pushed the change into the 7.x-1.x branch at drupal.org.

Thanks for your help, ak

marshmn’s picture

marshmn commented

Hi,

Regarding CC0: Ah, ok, I wasn't aware of CC0 - I guess that's fine, I can't see any problem with it being used in a GPL project, but I don't claim to be an expert in such things...

I see that you got rid of the master branch and made the 7.x-1.x branch the default now which is good. I also notice however that you seem to have a 7.x branch - is that meant to be there? It seems to perhaps be out of date? It seems to have a symlink in there that points to a location that doesn't exist and things. I'm guessing the branch should maybe be removed?

andrewkamm’s picture

andrewkamm commented

Hi marshmn, I just deleted that unused 7.x branch. Thanks again for your feedback! ak

aaronschachter’s picture

aaronschachter commented
Status: Needs review » Reviewed & tested by the community

This module looks good to me, I've reviewed it and found no errors, passes Coder review.

I can see a good use for this. It's nice to have my dev site tab stand out in the sea of drupal.org tabs I usually have open as I develop. As mentioned, sure it could be done by manually changing the icons in the theme settings, but its a lot more convenient to have it done instantly with this module.

andrewkamm’s picture

andrewkamm commented

Thanks for your review Aaron!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

cubeinspire’s picture

cubeinspire commented

I see that this module doesn't work well with admin_menu
The icon on the admin_menu bar is still the default from drupal.

Is it possible to change that?
I believe 80% of Drupal projects uses administration menu module.

andrewkamm’s picture

andrewkamm commented

@logicdesign: I'll check into this. I've created a separate ticket for tracking that request #1800990: Integration with admin menu module and will post a follow up there (probably later this week). thanks!

andrewkamm’s picture

andrewkamm commented
Issue tags: -PAReview: GPL Issue

Removing the GPL issue tag (there are no GPL conflicts).

andrewkamm’s picture

andrewkamm commented
Issue tags: +PAreview: review bonus

Adding "PAReview: review bonus" tag.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -PAreview: review bonus

Thank your for your 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).

manual review:

  1. sans_druplicon.module: "global $base_url;" this line is not needed, remove it.
  2. sans_druplicon.install: module_load_include() is not needed, because you are not using any functions from the module file?
  3. sans_druplicon_enable(): problem: if I disable the module and re-enable it, my settings get overridden? Why do you need that function, you can use default values with variable_get() anyway? This should be at least changed to hook_install(), so that this runs on module installation.
  4. sans_druplicon_init(): the filter_xss() calls are useless here, because no user provided data is involved. Please read http://drupal.org/node/28984 again.
  5. _sans_druplicon_image(): do not concatenate dynamic variables into watchdog() messages, use placeholders instead. If you use "@" or "%" you get check_plain() for free.
  6. "'Invalid $type argument provided'": this variable won't be expanded in single quotes. And again, you should use placeholders in watchdog() anyway.
  7. sans_druplicon_init(): do not concatenate the parts in t() with ".", as this cannot be picked up by translation extraction tools. Same in _sans_druplicon_help_text().

Setting this back to "needs work", you need know where to use sanitization functions and where not. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

andrewkamm’s picture

andrewkamm commented
Status: Needs work » Needs review

@klausi, thanks for the feedback. I've addressed the items you mentioned. Please see my comments below for specifics.

sans_druplicon.module: "global $base_url;" this line is not needed, remove it.

The $base_url var is used in the definition of the constant "SANS_DRUPLICON_IMG_DIR" and needs the global declaration in order to be accessible. Removing the global declaration results in an undefined notice from PHP.

sans_druplicon.install: module_load_include() is not needed, because you are not using any functions from the module file?

This has been removed.

sans_druplicon_enable(): problem: if I disable the module and re-enable it, my settings get overridden? Why do you need that function, you can use default values with variable_get() anyway? This should be at least changed to hook_install(), so that this runs on module installation.

I've moved the code in hook_enable() to a new hook_install() implementation. Regarding the use of variable_get(), I prefer to keep a the variable information in one place (sans_druplicon_variables()) and handle the instantiation and removal with dynamic code (as is done in hook_uninstall() and now hook_install()).

sans_druplicon_init(): the filter_xss() calls are useless here, because no user provided data is involved. Please read http://drupal.org/node/28984 again.
_sans_druplicon_image(): do not concatenate dynamic variables into watchdog() messages, use placeholders instead. If you use "@" or "%" you get check_plain() for free. "'Invalid $type argument provided'": this variable won't be expanded in single quotes. And again, you should use placeholders in watchdog() anyway.

I've removed the instances of filter_xss() in sans_druplicon_init() and the call to the watchdog() you referred to in _sans_druplicon_image(). The text "$type" in that message was intended to be literal, not expanded, but I've removed it entirely so there's no confusion about the intent.

sans_druplicon_init(): do not concatenate the parts in t() with ".", as this cannot be picked up by translation extraction tools. Same in _sans_druplicon_help_text().

I've removed all instances of concatenated strings within t().

thanks, ak

bhosmer’s picture

bhosmer commented
Status: Needs review » Needs work

Works as designed and seems useful.

Code review caught a few minor standards issues though: http://ventral.org/pareview/httpgitdrupalorgsandboxfatleaf1564464git-7x-1x

FILE: ...view/sites/all/modules/pareview_temp/test_candidate/sans_druplicon.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
6 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/sans_druplicon.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
243 | ERROR | Concat operator must be surrounded by spaces
--------------------------------------------------------------------------------

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review

Those two minor coding standard issues really don't justify "needs work". Any other application blockers?

bhosmer’s picture

bhosmer commented
Status: Needs review » Reviewed & tested by the community

Not from what I see. Functionally it works. Obviously, I am not the best for a manual code review though.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new drupalcs-result.txt789 bytes

The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

  remotes/origin/7.x-1.x-dev

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

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. sans_druplicon_init(): I think you should use hook_form_system_theme_settings_alter() instead, so that you don't need your custom request path logic. And then this will not run on every single page request, which is good for performance.
  2. sans_druplicon_admin_menu_output_alter(): do not create image markup yourself, use theme('image', ...) instead.
  3. _sans_druplicon_help_text(): is that really needed as separate function? Why not using the text directly in hook_help().

Anyway, that are not blockers, so ...

Thanks for your contribution, fatleaf!

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:

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.

andrewkamm’s picture

andrewkamm commented

Thanks @klausi! I updated the module to address items 2 and 3 from your previous review; I'll be updating soon to follow your recommendation in item 1 (moving some code from hook_init() to hook_form_system_theme_settings_alter()).

I also ran thru the code and cleaned up the formatting items noted by @bhosmer.

As an aside for others following this thread: The module has been enhanced so it will also override the icon shown by the Admin Menu module, as wisely suggested by @logicdesign.

I've promoted the module to a full project (http://drupal.org/project/sans_druplicon).

Thanks everyone!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Added "Reviews of other projects"