Theme Utils

hi rrrob,

good job on those reviews!

here's mine for you:

WSOD
while trying to install your module, I got this error:
Warning: Unexpected character in input: '\' (ASCII=92) state=1 in /yadayada/sites/all/modules/custom/theme_utils/theme_utils.module on line 194

can you help me fix it? the PHP CSS parser is right where it needs to be.

nevertheless, here's what I gathered by looking at your code:

module group
I would group your module under User interface.

javascript
your JS file is pretty neat - dare I say, better than the one I wrote in my application ;) - but it calculates em as a fraction of 16, when it's actually a fraction of whatever the font-size is defined by the theme (sometimes it's 14, for example).

simple workaround: place a text box in your settings form in which users type in their font size for em measuring, defaulting to "16".

libraries
I think your module could check for the PHP CSS parser library in sites/all/libraries. that's more aligned with Drupal best practices for external libs.

css
in your CSS, the class "tubox" is rather obscure. I would suggest you change it to "theme-utils-box".

comments
you could add some more comments in your code - especially in the JS.

the comment in theme_utils.module - "Primary module file" - is kinda obvious, since there can be only one .module anyway. try briefly describing your module.

general thoughts
the error message for missing library could point users to a link where they can download it.

in hook_help(), there's a word missing: "Various utilities that make theming a bit easier".

your variables are too generic. for example, $module could be replaced with $block_module. in my module, I placed the module's name in every variable (even ones that don't go to the db), but I'm paranoid. ;)

there's errors in PAReview, but they're false positives, I guess.

good luck!

cheers,
Luciano

Updating status, please check comment #2.

Hi ,rrrob:
I install the module,and find some error reports below:

Notice: Undefined offset: 0 in Sabberworm\CSS\Parser->parseValue() (line 347 of /home/jeff/www/DrupalStudy/DrupalStudy/sites/all/modules/review/theme_utils/php-css-parser/lib/Sabberworm/CSS/Parser.php).
Warning: file_get_contents(/home/jeff/www/DrupalStudy/DrupalStudy/sites/all/themes/smarter/system.menus.css) [function.file-get-contents]: failed to open stream: No such file or directory in theme_utils_get_media_queries() (line 194 of /home/jeff/www/DrupalStudy/DrupalStudy/sites/all/modules/review/theme_utils/theme_utils.module).
Warning: file_get_contents(/home/jeff/www/DrupalStudy/DrupalStudy/sites/all/themes/smarter/css/page-backgrounds.css) [function.file-get-contents]: failed to open stream: No such file or directory in theme_utils_get_media_queries() (line 194 of /home/jeff/www/DrupalStudy/DrupalStudy/sites/all/modules/review/theme_utils/theme_utils.module).

It will appear when i open a page first time and disappear if I refresh the page.

manual review:

1. _theme_utils_settings_is_numeric(): are you sure that you want to translate the textbox name?
2. _theme_utils_settings_is_numeric(): use element_validate_number() instead?
3. if you depend on PHP 5.3 then you should specify that in the info file, see http://drupal.org/node/542202 . Then you can remove all the namespace checks as you can rely on the fact that PHP 5.3 is present.
4. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org. Having the library in the module folder makes it harder to upgrade.
5. theme_utils.js: all user facing text must run through Drupal.t() for translation, example: "Click to hide".

But 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 ELC as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, rrrob!

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.

http://drupal.org/project/theme_utils

I have found some additional things that should be noted/adjusted when possible as the module already has a 1.0 release. I've listed the ones that don't warrant an issue all their own here, plus created an issue on the project issue queue.

klausi has already provided a link to it, but you should be structuring your commit messages so that they follow the directions on Commit messages - providing history and credit.
eg

by rrrob: Moved parser library out of module directory.
by rrrob: Other refactoring.
Issue #1975500 by rrrob: Corrected library info.

(Referencing this issue is the one exception).

Magic numbers 1 and 0 as booleans; use TRUE and FALSE PHP reserved words instead. On theme_utils_media_query, TRUE and FALSE are translated into 1 and 0 for the #disabled key which is not needed. variable_get(<var>, 1) should also be using TRUE/FALSE instead of 1/0.

Use REQUEST_TIME instead of time() in theme_utils_get_media_queries()'s call to cache_set().

Windows file permissions; The entire repository is executable files. See Changing file permission masks on Windows to fix them.

# Example: Remove the executable bit:
  git update-index --chmod=-x path/to/file.ext

#1975500: Incorrect information in hook_libraries_info() - "My test library" etc.