BlockCT (Block Content Type) utilizes the modules nodeblock and blockcss to create a content type of block which allows users to manage blocks from within that content type. This is very useful if you are creating a site for someone you do not want to touch the actual block settings in the admin menu. It also allows for designers to create a list of styles that normal users can choose from for that specific theme.

http://drupal.org/sandbox/tyhullinger/1764268

Drupal 7.x

Depends on BlockClass and nodeblock

git clone http://git.drupal.org/sandbox/tyhullinger/1764268.git blockct

Comments

tyhullinger’s picture

tyhullinger commented
Status: Active » Needs review
2pha’s picture

2pha
Primary language English
commented

Hi,
Nice idea for a module, I have often needed to allow for a client to edit blocks but not wanted them to access the block admin page. Though usually I would create a content type and then just grab it's content with php in a block. Having a module for it might make the process a little easier.

You are still working in the master branch, you need to create a version branch
See: http://drupal.org/empty-git-master

the git clone link you posted is wrong, it should be : git clone http://git.drupal.org/sandbox/tyhullinger/1764268.git blockct

Didn't pass pareview test: http://ventral.org/pareview/httpgitdrupalorgsandboxtyhullinger1764268git

You might find it helpful to run the code review module: http://drupal.org/project/coder

Your project should not contain a license.txt. Pleasse see: http://drupal.org/node/1587704

You should probably state in the original post that this module has 2 (non core) dependencies: block_class and nodeblock.

Manual review:
blockct.info:
You should remove the version line.
You do not need the file[] lines. These are only needed if they contain a class
See: http://drupal.org/node/542202

blockct.install
You are using the st function in some places but t function in other places.
In the uninstall function, you are not deleting the variables you set in the install function

blockct.module
There is a whole function (blockct_permission) commented out?
Line 163: extra line? Is it needed? I'm not exactly sure what the policy is.
Line 207: drupal_goto(''), what is this doing?

blockct.test is empty

2pha’s picture

2pha
Primary language English
commented
Status: Needs review » Needs work

OH...nice use of comments BTW :)

tyhullinger’s picture

tyhullinger commented

All of these issues have been addressed. See my comments to each below each section.

Manual review:
blockct.info:
You should remove the version line.

Removed the version line.

You do not need the file[] lines. These are only needed if they contain a class
See: http://drupal.org/node/542202

All of the file[] lines have been removed.

blockct.install
You are using the st function in some places but t function in other places.
In the uninstall function, you are not deleting the variables you set in the install function

I made sure to use $t = get_t(); and $t('some text'); instead of using st and t directly.

blockct.module
There is a whole function (blockct_permission) commented out?

I'm debating on whether or not to implement permissions. I went ahead and removed this entire snippet of code. I'll put that in a "future" release section.

Line 163: extra line? Is it needed? I'm not exactly sure what the policy is.

This was removed while making fixes via pareview.

Line 207: drupal_goto(''), what is this doing?

This is redirecting the user to the home page so that once the block has been created they are not confused why the content is displayed differently than the block. If it makes more sense to have them redirected to the node page then I will adjust. In the specific case that I built this module for it made more sense to redirect to the home page.

tyhullinger’s picture

tyhullinger commented
Status: Needs work » Needs review

All of these issues have been addressed. See my comments to each below each section.

Manual review:
blockct.info:
You should remove the version line.

Removed the version line.

You do not need the file[] lines. These are only needed if they contain a class
See: http://drupal.org/node/542202

All of the file[] lines have been removed.

blockct.install
You are using the st function in some places but t function in other places.
In the uninstall function, you are not deleting the variables you set in the install function

I made sure to use $t = get_t(); and $t('some text'); instead of using st and t directly.

blockct.module
There is a whole function (blockct_permission) commented out?

I'm debating on whether or not to implement permissions. I went ahead and removed this entire snippet of code. I'll put that in a "future" release section.

Line 163: extra line? Is it needed? I'm not exactly sure what the policy is.

This was removed while making fixes via pareview.

Line 207: drupal_goto(''), what is this doing?

This is redirecting the user to the home page so that once the block has been created they are not confused why the content is displayed differently than the block. If it makes more sense to have them redirected to the node page then I will adjust. In the specific case that I built this module for it made more sense to redirect to the home page.

tyhullinger’s picture

tyhullinger commented
Issue summary: View changes

Updated the git clone URL and added the dependent modules to the description.

rob holmes’s picture

rob holmes commented

Hi, I spotted a few minor issues that i've listed below.

Manual Review

blockct.info

Maybe add a link to admin/structure/blockct from the .info file e.g. configure = admin/structure/blockct

blockct.module

Line 114 missing t() function around title text

Line 122 missing t() function around title text
Line 123 missing t() function around description text

Line 131 missing t() function around title text
Line 132 missing t() function around description text

Line 139 missing t() function around title text
Line 140 missing t() function around description text

Line 159 missing t() function around title text
Line 160 missing t() function around description text

Line 176 missing t() function around title text

caesius’s picture

caesius commented
Status: Needs review » Needs work

1. You still need to delete the master branch; follow steps 5 and 7 on this page: http://drupal.org/node/1127732

2. Your README file cannot have lines exceeding 80 characters.

3. There is a .patch file in your repository. Please remove it.

4. Do not leave commented-out code in your module:

  7 /**
  8  * Implements hook_permission().
  9  * TODO implement permissions possibily
 10  */
 11 /*function blockct_permission() {
...
 23 }*/

5. Also check the previous comment's issues re: t().

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes

Updated git clone command to include blockct as the folder.