[D7] Site banner

adding link to reviewed projects

adding another project review

updating list of project reviews

I reviewed the following project applications and would like to request the PAReview review bonus.

• https://drupal.org/node/2116843#comment-7987931
• https://drupal.org/node/2114381#comment-7988065
• https://drupal.org/node/2116923#comment-7988149

Hi kamleshpatidar,

Thanks for the review :-) and the guidance - I've added those fixes to the latest version on the Git repo. http://drupalcode.org/sandbox/ajosephau/2105389.git/commit/2e6cac1db56f9...

Thanks,
ajosephau

Hi oresh,

Firstly, the easy CSS include issue: using drupal_add_css() is a wonderful idea! Thanks for letting us know about it. I've cleaned up most of that section using the drupal core api commands in the latest build http://drupalcode.org/sandbox/ajosephau/2105389.git/commit/2e6cac1db56f9....

And with regards to using the #page-wrapper CSS override: that was one of my biggest challenges (and regrets) with this module - trying to get it to play nicely with the themes. I originally tried to follow the Toolbar (the Drupal core module that adds the administrator's toolbar to the top of the page) module's way of getting banners on the page: I think it adds an offset to the #body section. I found that the bottom banner didn't play nicely with the #body section - especially when the admin's overlay pages were displayed as the overlay would cover the bottom banner. So the only way I was able to get the banners to display correctly:
- with the variety of admin toolbar modules available,
- with the admin overlay pages displayed, and
- with a variety of themes
was to add the offset to the #page-wrapper tag.
My clients and I develop sites either with the standard Drupal core themes or customised AdaptiveTheme themes, and they all seem to have that "page-wrapper" DOM object... so my hunch before publishing this module was that the majority of themes would support the module and if it didn't then there was some guidance in the README.txt file that could help out the site builder/themer customise my module or their theme to work.

I'm more than happy to make any changes to get the banners to integrate better (and avoid hacking on top of the #page-wrapper tag) and I'd appreciate your advice, but sadly I'm out of ideas...

Thanks for your review!
ajosephau

Hi Tanay,

Firstly thanks for reviewing my module :-). I'm working on some of your suggestions at the moment, but I thought I might put in a comment in the mean time so you know I haven't forgotten about you!

I'll keep on implementing your suggestions and change the status once it's ready to go.

As for your last two suggestions, it might take a little longer to implement so I'll include them in the project page (https://drupal.org/sandbox/ajosephau/2105389).

Thanks again for your review!
ajosephau

The latest build contains an updated administrator interface using the #states imperatives for the module settings and corresponding context settings page. Please see the project page for screenshots.

Latest PAReview.sh results shows clean build http://pareview.sh/pareview/httpgitdrupalorgsandboxajosephau2105389git

Updating project status to review.

Hello coredumperror (wish I thought of a good name like that!),

Firstly, thank you very much for time to review my module, especialy for writing the patch! I totally agree about using PHP's variable interpolation instead of concatenating strings on-the-fly. I have incorporated your patch into the Git repo: I hope I got the git attribution correct (let me know if it's not right).

Oh yes, about those @codingStandardsIgnoreStart blocks: this was an issue I found with integrating with the Context module. Unfortunately now I get the following errors reported by Coder/PAReview.sh

FILE: ...pal-7-pareview/pareview_temp/site_banner_context_reaction_functions.inc
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
17 | ERROR | Public method name
| | "SiteBannerContextReactionChangeBannerText::options_form" is not
| | in lowerCamel format, it must not contain underscores
91 | ERROR | Public method name
| | "SiteBannerContextReactionChangeBannerText::options_form_submit"
| | is not in lowerCamel format, it must not contain underscores
112 | ERROR | Public method name
| | "SiteBannerContextReactionChangeBannerBackgroundColor::options_form"
| | is not in lowerCamel format, it must not contain underscores
120 | ERROR | Public method name
| | "SiteBannerContextReactionChangeBannerBackgroundColor::options_form_validate"
| | is not in lowerCamel format, it must not contain underscores
129 | ERROR | Public method name
| | "SiteBannerContextReactionChangeBannerBackgroundColor::options_form_submit"
| | is not in lowerCamel format, it must not contain underscores
161 | ERROR | Public method name
| | "SiteBannerContextReactionChangeBannerTextColor::options_form"
| | is not in lowerCamel format, it must not contain underscores
169 | ERROR | Public method name
| | "SiteBannerContextReactionChangeBannerTextColor::options_form_validate"
| | is not in lowerCamel format, it must not contain underscores
178 | ERROR | Public method name
| | "SiteBannerContextReactionChangeBannerTextColor::options_form_submit"
| | is not in lowerCamel format, it must not contain underscores
--------------------------------------------------------------------------------

Unfortunately, in order to integrate with the Context module I need to use those function names which don't pass these checks, so to get a 'clean build' from Coder/PAReview.sh. I added those ignore statements around the entire code block as putting the ignore comments around the comment only meant the review modules threw 'missing comments' errors instead.

For the mean time, I inherit the comments from the parent class' (context_reaction) documentation.

Hi klausi,

Thank you so much for your in-depth manual review :-). Here are the comments as requested:

4) I re-use a lot of form generation, validation and submission code between the admin page and the context admin page: I understand putting the form building functions makes it a bit harder to read, but I felt it was a lot more maintainable and consistent between both pages.

5 and 6) You are correct - I don't need to have the separate hook_html_head_alter() and hook_page_build() functions - I have merged both functions together and added some supporting comments to explain what has happened. But because I am using the hook_page_build() I still use the drupal_add_css() functions to add the CSS files to the page, because I don't think I have access to the #attached property on the head elements?

8) I agree - originally I wanted to give the admin freedom to put in raw HTML (hence I put the warning on the permission), but indeed you are correct it is an XSS vulnerability. I added the filter_xss_admin() to the colour and text outputs in the hook_page_build() function.

I have also commented out of completeness for the rest of your notes.

1) I originally had to exclude the whole methods from the coding standards because when I put the ignore tags around the comment headers, the reviewing modules complained there was 'blank space' between the comment block and function name. I must admit, I didn't think I would get reviews (and more importantly a full project approval) if I didn't remove all the errors thrown by PAReview.sh/Coder module. So thanks for your comment - I've removed the ignore statements and you'll see the errors in the latest results http://pareview.sh/pareview/httpgitdrupalorgsandboxajosephau2105389git

2) I used the check_plain() command as I got the error from the Coder module saying "Potential problem: drupal_set_message() only accepts filtered text..." - I read that link you suggested, and given that the only substituted text is a URL I used the check_url() command.

3) Agreed - I have added functions to define default banner text and colours, and I have put those as parameters to the variable_get() functions.

7) Agreed - fixed in latest commit.

9) Agreed.

I just need to update my automated test files to work with the fixes in points 5 and 6, but I'll update the status once the commit is ready. Thanks again for your review!

EDIT:
5 & 6) Following the guidance at https://drupal.org/node/542202#stylesheets , I put the CSS file references in the site_banner.info

Oh I understand - I must admit it I developed that misconception from looking at other reviews and assumed I needed to have a clean build before I'd get my module reviewed. I'll add an explanatory note to the issue description too soon for future reviewers (and keep it in mind for other project application reviews I do - I want my PAReview bonus back!).

changing heading to suit review bonus instructions
putting headings instead of bold text

Updating status to review after responding to/resolving feedback raised by coredumperror and klausi. Thanks again for the review!

Adding known issue to issue text

Thank you so much klausi for your second review (and your patience!). I totally agree with your feedback, and I think I'm starting to understand how these security functions like check_url(), check_plain(), filter_xss() work, especially with the t() function.

I'm making a commit to the repo soon with your fixes, and I'm hoping to get some free time to do some reviews soon.

Thank you so much klausi for updating my account, and for all of your guidance (and recent notes too - I've got to get in on this IRC thing!)

And thank you too to kamleshpatidar, oresh, saitanay and coredumperror - you all are the best!