The Block Inject module allows for regions to be created and injected in the middle of chosen node types.

You can create any number of regions to inject as well as select as many node types you would like these regions to be injected in. The blocks you assign to these node types will then appear in the middle of the body field of these node types if the number of paragraph is bigger than 2.

The difference between this module and Block Inject Filter or Insert Block is that it offers an automatic way of inserting as many blocks you want in the middle of the body of any number of node types you want.

Possible use cases: Ad blocks that you may want to appear in the middle of all your articles - You set it once and you don't need to insert it with every node creation.

Link to the project page: http://drupal.org/sandbox/Knob/1859588

You should clone the 7.x-1.x branch as the module is for Drupal 7.

git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/Knob/1859588.git block_inject

Looking forward for suggestions and ways of improvement.

Thank you!

CommentFileSizeAuthor
#1 code_review.txt29.7 KBasifnoor

Comments

asifnoor’s picture

asifnoor commented
Status: Needs review » Needs work
StatusFileSize
new code_review.txt29.7 KB

Hello knob,

Thanks for creating this module. I did a quick review and found few isseus. Please correct them.

1. git clone url provided is wrong. remove username@
correct clone url will be git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/Knob/1859588.git block_inject

2.Ran a coder module. Many issues found. Please check attached file

3. Would be nice to provide configure link in modules page

Once you correct them, assign it back for review. Also would suggest you to review other proejct application and get a review bonus.

asifnoor’s picture

asifnoor commented
Issue summary: View changes

Update

upchuk’s picture

upchuk commented
Status: Needs work » Needs review

Hello asifnoor,

Thanks for checking the module.

I corrected the git URL, made the code modifications so that Coder doesn't flag them anymore (I had my net beans mixed up a bit) and added the config link on the module page.

I committed it back in so you can check it.

Thanks!

mohs3n71’s picture

mohs3n71 commented
Status: Needs review » Needs work

automatic review :
http://ventral.org/pareview/httpgitdrupalorgsandboxknob1859588git
there are many errors and warnings in your code please fix them first
manual review :
your code looks good just a little thing , in .info file i think you should specify a package for your code

upchuk’s picture

upchuk commented
Status: Needs work » Needs review

Hey there,

So I cleaned up the code better. I reduced it down to only a few

ERROR | Missing function doc comment

This tells me that there are no comments before the functions but there are in fact.

As for the package in the .info file, I don't know what package to add it to at this point. Do you have any suggestions?

Thanks!

mohs3n71’s picture

mohs3n71 commented

I think you can use "Other" package

upchuk’s picture

upchuk commented

It goes by default to the 'Other' package.

willvincent’s picture

willvincent commented

Yes, if package is omitted from the .info file, the module will default to appearing as part of the 'other' package.

You can assign it to pretty much whatever you want though, so if it made sense you could create a new package for your module to live in. I suspect there's no compelling reason to do that, as that generally only makes sense if your module provides a bunch of sub-modules as well.

So, assuming there's no better place for it to live besides 'other' there is no reason to define the package in the .info file.

upchuk’s picture

upchuk commented

Yes indeed, and for this reason I did not create a separate package for the module.

D

udaksh’s picture

udaksh commented

Hi Knob,

Your module can result into XSS vulnerability if Available Content Type(s) contains some XSS value. Your module is getting content from node_type table in function block_inject_get_all_node_types() and displaying the results in function block_inject_add_inject_form(). You should use as check_plain($record->name) at line 72 in file block_inject.module.

Also look at http://drupal.org/node/263002. Instead of applying check_plain() filter in function block_inject_add_inject_form_submit(). You should apply that filter at the time of displaying the results (inside function block_inject_list()). You can use check_plain($row->region) and check_plain($row->node_name) at line 22 in file block_inject.admin.inc.

Thanks,
Udaksh

anwar_max’s picture

anwar_max
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +PAreview: security

I think you forgot to change status and adding security tag.

upchuk’s picture

upchuk commented
Status: Needs work » Needs review

Hello there,

Thanks a lot for the comments.

I made the changes you asked. Thanks for pointing them out. In the interest of learning, I have a couple of follow up questions if you can indulge me:

1. Can there be an XSS vulnerability if the the module looks in the node_type for the available content types? Isn't content safe in there?

2. If I do not apply the check_plain function before the database insert (although I understand the benefits of doing so), doesnt that constitute SQL injection vulnerability? Does the database layer take care of that?

Thanks a lot! And please, if you have any other comments, let me know!

bhosmer’s picture

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

At least for question number 2, I learned from some mentors that Drupal best practices are to store the data exactly as it is entered, and then clean it on the way out.

I don't believe the database layer offers any protection against a SQL vulnerability, this is really where SQL vulnerabilities come from. This is handled with code instead.

upchuk’s picture

upchuk commented

Thanks for the tip!

Anything else that needs to be done at this stage?

bhosmer’s picture

bhosmer commented

I would say it looks pretty good to me.

If you participate in the PAReview process, @klausi will see it quickly and you shouldn't have any problem getting this closed out.

jthorson’s picture

jthorson commented
Status: Reviewed & tested by the community » Fixed

A few comments:

Re #11: Drupal's policy is to store exactly what is entered, and then filter on output. Generally, do not trust any user-generated content, even if it is supplied via an administration interface. The handbook pages at http://drupal.org/writing-secure-code (and every page under it) go into this in detail, and are strongly recommended reading material for any contrib author. :)

block_inject.module Line 68: Do you have a particular reason for limiting to the first 10 node types on a site?

.

Thanks for your contribution, Knob!

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.

upchuk’s picture

upchuk commented

Thanks a lot jthorson!

To answer your question, the limit was for my dev environement, I will remove it. Thanks for noticing that.

Cheers!

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Corrected git clone URL