This module allow site admin to set HTTP response headers to different part of the sites through admin UI. Currently this module allows to set the header by

1. path
2. content type
3. user roles.

There is a admin configuration page to manage list of headers that are allowed to set.

Similar module:
1. Context HTTP headers(https://drupal.org/project/context_http_headers) - Specific to context.
2. It works the same way block module does, but the front end functionality is different.

Project page: https://drupal.org/sandbox/vijaycs85/2108205

Project repo link: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/vijaycs85/2108205.git http_response_headers

Reviewed project:
1. https://drupal.org/node/2113649#comment-7981789
2. https://drupal.org/node/2115823#comment-7981809
3. https://drupal.org/node/2107661#comment-7981839
4. https://drupal.org/node/2096821#comment-7981865

Review set 1 (till #14):
1. https://drupal.org/node/2113649#comment-7973389
2. https://drupal.org/node/2101533#comment-7973573
3. https://drupal.org/node/2112353#comment-7976299

Comments

vijaycs85’s picture

Issue summary:View changes

Updating project page URL.

octoplod’s picture

hi,

I'm receiving 'warning; remote HEAD refers to nonexistent ref, unable to checkout' when I tried to clone your repository.

octoplod

AjitS’s picture

@vijaycs85 : You are doing awesome job on the Drupal 8 core issue queue.
For others who don't know about vijaycs85, Dries just mentioned his name in his keynote at DrupalCon Prague this year, for his contribution towards Drupal 8 core (has seen more 140+ patches through, also helps in keeping the issue queue clean, etc.)
I'd suggest we give him a privilege of working on Drupal 8 core (we need some people there badly). If it counts I can volunteer on his behalf for reviewing the projects which would help him seeing this issue through.

Enough introduction, now back to work ;-)

  • You should change your git clone link to clone the non-maintainer link of the module.
    git clone http://git.drupal.org/sandbox/vijaycs85/2108205.git http_response_headers
  • The automated review shows up the issues which needs to be rectified. http://pareview.sh/pareview/httpgitdrupalorgsandboxvijaycs852108205git
  • I wasn't able to clone the project : Got the following error:
    Cloning into 'http_response_headers'...
    remote: Counting objects: 9, done.
    remote: Compressing objects: 100% (8/8), done.
    remote: Total 9 (delta 0), reused 0 (delta 0)
    Unpacking objects: 100% (9/9), done.
    Checking connectivity... done
    warning: remote HEAD refers to nonexistent ref, unable to checkout.
  • You project page seems to be incomplete. Please have a look at Tips for a great project page to to get an idea on how a project page should be.

All the best with the module. Will stay subscribed and provide more feedback whenever I'm able to clone the project.
Changing status to needs work. Once you fix the issues, you should change the status to needs review.
Keep up the good work!

AjitS’s picture

Status:Active» Needs work
vijaycs85’s picture

Status:Needs work» Needs review

Thanks @octoplod and @AjitS for reviewing it. Here is the update on individual items:

#1:
@octoplod as mentioned by @AjitS, I put the wrong clone URL. I just updated. It should work now.

#2:
Thanks for the intro @AjitS :)

1. Clone URL - fixed
2. Automated review - fixed (Ref: http://pareview.sh/pareview/httpgitdrupalorgsandboxvijaycs852108205git).
3. Can't clone - may be URL issue in 1?
4. Incomplete - Updated information as much as I have. Please let me know, if I still miss any.

vijaycs85’s picture

Issue summary:View changes

Updated issue summary.

PA robot’s picture

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.

AjitS’s picture

Code sniffer is still not happy

FILE: ...sites/all/modules/http_response_headers/http_response_headers.drush.inc
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
36 | ERROR | Multi-line array contains a single value; use single-line array
    |       | instead
46 | ERROR | Multi-line array contains a single value; use single-line array
    |       | instead
49 | ERROR | Multi-line array contains a single value; use single-line array
    |       | instead
59 | ERROR | Multi-line array contains a single value; use single-line array
    |       | instead
62 | ERROR | Multi-line array contains a single value; use single-line array
    |       | instead
72 | ERROR | Multi-line array contains a single value; use single-line array
    |       | instead
--------------------------------------------------------------------------------
FILE: ...sites/all/modules/http_response_headers/http_response_headers.admin.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
126 | ERROR | Multi-line array contains a single value; use single-line array
     |       | instead
--------------------------------------------------------------------------------
bneil’s picture

Status:Needs review» Needs work

Hello,

After uninstalling the module, there are some variables that this module set that were not deleted. You will want to use hook_uninstall() and variable_del() to remove them.

UX:
It might be useful to add a configure link from the modules page: https://drupal.org/node/542202#configure
Also having a message on admin/config/system/http-response-headers that informs you that you need to set the allowed headers in settings might be helpful.

vijaycs85’s picture

Status:Needs work» Needs review

Thanks @AjitS and @bneil for review. I have fixed all in #7 and can't get the code review issues in #6.Not sure, if I'm missing any.

vijaycs85’s picture

Issue summary:View changes

Updated issue summary with anonymous clone URL

vijaycs85’s picture

Issue summary:View changes

Updated issue summary.

bappa.sarkar’s picture

Still has some CS issues http://pareview.sh/pareview/httpgitdrupalorgsandboxvijaycs852108205git

Manual review

  • It is better to add htmls in tpl files. You can use custom theme function and add a tpl for the help hook.
  • you dont have to use $_GET['q']. use request_path() instead.
  • You can decrease the code lines from
    // Compare the lowercase internal and lowercase path alias (if any).
    $page_match = drupal_match_path($path, $pages);
    if ($path != $_GET['q']) {
      return $page_match || drupal_match_path($_GET['q'], $pages);
    }
    return $page_match;

    To
    return drupal_match_path($path, $pages) || drupal_match_path($_GET['q'], $pages);

  • You can replace
    $header_rules[$header] =  $header_value; with drupal_add_http_header($header, $header_value); No need for double foreach loop inside init hook.

Otherwise looking fine to me.

bappa.sarkar’s picture

Status:Needs review» Needs work
vijaycs85’s picture

Status:Needs work» Needs review

Thanks for your review @bappa.sarkar. here is my updates on them:

#9.1 Not sure, whether we really use .tpl for hook_help content. I checked core and found more content and still they are on .module. So I guess it is better to leave it in .module.
#102. request_path() vs $_GET['q'] - fixed.
#9.3 I guess, there is a if condition and that need to be executed if the path is not same.
#9.4 Removed the foreach.

There are 3 defects related to lowerCamel naming of properties. Ctool requires them as '_'. So can't help more there.

vijaycs85’s picture

Issue summary:View changes

Updated issue summary.

vijaycs85’s picture

Adding my 3 project application reviews in issue summary and updating tag...

abghosh82’s picture

I did a manual review of your module and I like the idea behind it. A few very minor suggestions:

There are some line in the code which are too long for good readability. You can consider breaking them into multiple lines.
Some of these are as below:
Content in hook_help I agree no tpl is not required but consider breaking the long lines into multiple lines.
File 'http_response_headers.drush.inc' line no. 59, 76, 143, 325 and 344.
File 'http_response_headers.admin.inc' line no. 27.
Some line in 'http_response_headers_ui.module'.

Other than these all looks good and a very well written module.

Not setting to needs work as its very minor but you can consider addresing them.

klausi’s picture

Assigned:Unassigned» dman
Status:Needs review» Reviewed & tested by the community
Issue tags:-PAReview: review bonus

manual review:

  • http_response_headers_ui_form_validate(): empty function, so remove it.

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

vijaycs85’s picture

Thanks for your review @klausi. I have removed the empty validation.

PA robot’s picture

Status:Reviewed & tested by the community» Closed (duplicate)
Multiple Applications
It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2115857

Project 2: https://drupal.org/node/2108495

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

PA robot’s picture

Issue summary:View changes

Updated issue summary with reviews

vijaycs85’s picture

Status:Closed (duplicate)» Needs review
Issue tags:+PAReview: review bonus

Ok, closed the other application(#2115857: [D7] Client cache) and updating reviews in summary and adding 'review bonus' tag.

vijaycs85’s picture

Issue summary:View changes

Updated issue summary - Updating reviews

klausi’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

klausi’s picture

Status:Reviewed & tested by the community» Fixed

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

Thanks for your contribution, vijaycs85!

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:

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.

vijaycs85’s picture

Thank you.

vijaycs85’s picture

Issue summary:View changes

Updated issue summary.

Status:Fixed» Closed (fixed)

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