[D7] Theme-color meta tag

It is working correctly. @bobrov1989 great work!

Hi bobrov1989,

Thank you for you module. I tested it and seems working but it is also confusing.

• Issue

If I visit the global configuration page (admin/appearance/settings), change the default value and save the form, the color selector is then reflecting my choice as the meta value in the source code. If I visit the configuration page for a specific theme (let's say Bartik as I used a default Drupal core - admin/appearance/settings/bartik), the color picker is reflecting the default configuration. The issue occurs when I chance the color for this specific theme and submit the form. The color picker (and the value) doesn't reflect my previous choice. If I visit the front office, the meta value is reflecting my choice so it seems only an issue with the default value on the form but may be an issue if I submit the form without resetting my appropriate color (to fix it, you should probably provide a theme name to the theme_get_setting() function).

• Question

Would it be possible to have the field from your module being part of the "Color scheme" field-group when editing settings for a specific theme?

• Code review
• Javscript file seems wrong as it is starting with a semi-colon and there is a trailing comma on line 15
• It may be interesting to specify a package to your module (SEO or Appearance for example) in .info file
• Adding the entire function description for hooks implementation is probably not required and the code looks more complex (probably a personal opinion here)
• In theme_color_meta_tag_form_validate, you can probably use color_valid_hexadecimal_string() instead of creating your own function (based on the core one as the comment seems the same). You may include the color.module file manually if it is not enabled but you can rely on its presence since it is core module (I understand that you don't want a dependency here)
• I would suggest you to provide a hook to alter the color before it's being added to the page so developers can use this hook to dynamically alter the value based on something else (for example I may have a taxonomy to determine the main color on each article and may use it to fill the metatag)
• Line 67 is missing a *.
• Line 33 is missing 2 spaces.
• You may use the @variable instead of %variable in form_set_error().
• Why are you checking the value is not empty (line 60) instead of using #required in the form ?

Thanks,

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

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 bobrov1989,

Providing a hook_alter() is very easy.

$color_value = theme_get_setting('theme_color_meta_tag') ? theme_get_setting('theme_color_meta_tag') : '#48a9e4';

drupal_alter('theme_color_meta_tag_color', $color_value);

Developers can now use my_module_theme_color_meta_tag_color_alter($color);

Please note that it's good practice (maybe mandatory) to describe your hook in theme_color_meta_tag.api.inc (see other contributed modules for example).

Hope that helps

Hi bobrov1989,
1. Please remove LICENSE.txt and add README.txt. Its missing in your project.
2. There does not seem to be a heavy use of 'THEME_COLOR_META_TAG_DEFAULT' in your .module file.( I found it only on two places). So you can directly use the value instead.

Manual Review

1. Individual User Accounts
Yes: Follows the guidelines for individual user accounts.
2. No Duplication
Yes: Does not cause duplication and/or fragmentation.
3. Master Branch
Yes: Follows the guidelines for master branch.
4. Licensing
Yes: Follows the licensing requirements.
5. Secure code
Yes: Meets the security requirements.
6. Code long/complex enough for review
No: Does not follow the guidelines for project length and complexity.

Hi bobrov1989,

I have reviewed your project.
Good work!
But i have some findings:

• According to pareview you need to add small changes into your README.txt
• Please, change your hex value to lowercase (theme_color_meta_tag.api.php, 18)

Automated Review

Pareview.sh show some minor JS errors - http://pareview.sh/pareview/httpgitdrupalorgsandboxbobrov19892514202git

Coder modules does not show any errors.

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 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 guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

No: 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. Replace the text below by the issues themselves:

1. You should address Pareview.sh errors before any release.
2. You should add hook_help to provide information about your module in Drupal UI.
3. I think you should remove maintainer name from the .info file. I have never see a project that have maintainer name in the info file. For more information about the the .info file please read, Writing module .info files (Drupal 7.x).

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.

I do not see any blocker on this project. I have added the PAReview: Single project promote tag because the project is too short. For more information about Single Project promote visit the What is a single project promotion? page.

In Drupal we normally filter on output, rather then sanitize on input. I would add a check_plain where you output the color.

FYI this meta tag is already customizable via the Metatag module.

Thanks for your contribution, Vitaliy!

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.

I am giving credits to the users who participated in this issue.