[D7] Codit

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

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.

Automated Review

PAReview complained about:

FILE: ...ar/www/drupal-7-pareview/pareview_temp/codit_blocks/codit_blocks.module
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
183 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

The problem line:

'info' => t(ucwords($human_readable)),

One of the purposes of the t() function is to facilitate extraction of string literals for localization. This does not work with a function call or a variable. If you need to sanitize the string, you should run it through check_plain.

OK, I buy your premise that there is a use-case for passing this variable through t().

I also agree that having a string with only a placeholder to make PAReview happy, is silly (and doing this is the same as not using t(), since t() does not translate the stuff passed it by means of an array assigned to a placeholder).

However, given that the whole point of passing this through t() is to allow admins to create translations of these "friendly names of blocks that are being generated dynamically from file names", admins need to be told about this feature and how to use it. The method you describe for making use of these strings for translation is IMHO far from intuitive.

So to support this use-case, you will need to, as a minimum requirement, document this (e.g. in README.txt). Documenting it will have the additional benefit of making reviewers (like me) aware of why you do such non-standard things as passing a variable through t().

The following is not a requirement, but just ideas:

To provide better support the translation, the "friendly names of blocks that are being generated dynamically from file names" should be exposed to admins on some page, so that admins can see what these names are and what they shall be able to translate. For even better support, you could generate the codit_local/codit_local_function_definitions.inc with the literal strings suitable encapsulated in calls to t() (or even better: equivalent local .po files that can be imported into the translation interface), to spare admins from the task of having to do this themselves.

Btw - are users allowed to create the files the names are derived from, and if so, are the file names sanitized? I have Drupal sites that allows file uploads, and I've seen names of uploaded files that are clearly intended to be various forms of injection attacks. (I could find out by reading your code, but it is quicker to ask than scrutinizing the code.)

This sounds very ambitious and thought-out, but unfortunately, it seems to me like it's reinventing the wheel, sorry:

> In its simplest use case for Codit Blocks, you copy a sample directory in codit_local//blocks/block_bin then rename the directory and tpl inside with the name (delta) of the block you are building. Flush cache and your block is now registered for placement with Context, Panels, or the block admin. Add some content to the tpl and you have your block. (drush will handle this soon)

Why not use Boxes module to create a Box block, and then export it to a Feature, which can then be added to the codebase?

> Beyond making a simple block, add some code to the callback function within the directory and whatever it returns will be available to the tpl. This is useful for doing the heavy lifting and keeping PHP out of the tpl.

Boxes lets you define custom box types, which are CTools plugins. Within those, you can use PHP to create the output.

About duplication:

To me, it looks like Codit has enough unique features to justify its place as a module of its own. I really like the idea of being able to manage this through code rather than trough the database.

However, the subject of duplication (i.e. similar projects and how they are different) need to be addressed on the project page, and not only in this issue summary or as comments in this issue queue. It should not be as long and comprehensive as #10. All that is required is a link to the similar project and a line or two that highlights the main difference.

I think there are at least exists two similar project:

• Boxes
• Custom Page

Please take a moment to make your project page follow tips for a great project page.

> Boxes is nearly entirely admin based. Codit: Blocks are 100% code based. The only admin is whatever tool you use to place them (block admin, Context, Panels)

Boxes are admin based, but can then be exported to code with Features. So the above is misleading.

I still think you're going to great lengths to reinvent the wheel, when Features is already the defacto standard for this sort of thing. But gisle disagrees, which is fair enough. I'm just going to unfollow this.

Joachim, I don't think that it is only in rare cases overlap with an existing module should be a blocking issue.

I don't think we should block a project just because it does the same job as another project. If we have that as a rule, we may prevent better solutions from ever entering the Drupal ecosystem.

The way I interpret § 1.2 of the Project application checklist, is that we only block if the "differences between your modules are not too fundamental for patching an existing one". IMHO, it makes no sense "patching" Boxes to contain Codit.

What we (as reviewers) instead should require in the cases of functional overlap, is that the project page:

1. acknowledges the existence of similar projects; and
2. briefly explain how they are different.

The way I see it, this requirement has been fulfilled in this case.

I haven't looked carefully at the code net, so I need to do a code review before setting status, but unless there are issues in the code, as far as I am concerned, this is close to completion.

I am going away on a week-long vacation today, so there will not be a code review from me anytime soon. I hope somebody else takes a look at the code in the meantime.

README.md

hook_help in all modules sniffs for the markdown filter module. It should be mentioned in README.md that the presence of this module alters the behaviour of the modules.

To create a new block the following shell command is suggested:

cp codit_local/blocks/block_bin/_a_sample_block codit_local/blocks/block_bin/cool_block

This does not work on most Unix-like platforms. cp need the -r option to copy directories.

There are other minor inaccuracies in these instructions as well. Please proofread.

Code review.

• Docblock for hook_init() in codit.module claims it is hook_boot().
• (*) codit_blocks.module line 256 : empty($_access($user)) - PHP function empty needs a variable, not a function, as argument.
• (*) hook_help() sniffs for the markdown filter module. No online help is displayed if the filter is found. This is probably not the intended behavior.

The starred items (*) are fairly big issues and warrant going back to Needs Work. The rest of the comments in the code walkthrough are recommendations.

I personally disagree with what was said earlier about the use of t() with a variable.

I (personally) would suggest rewriting this to allow the custom strings to be localized, while at the same time not opening yourself up to security holes.

You could accomplish this by using t() as an example localization workflow, while making sure to sanitize everything passed to it.

The thing to remember is that the best practice in drupal is to sanitize validate user input coming in and sanitize going out. What you are doing does not accomplish this.

I think I am going to reverse my earlier decision and go with gwprod on this one. I never really liked the hackish approach where the user is supposed to create corresponding wrapped strings in codit_local/codit_local_function_definitions.inc.

For the purposes of this review you could either:

1. Remove the t() wrapper around the variable, thus making PAreview and us reviewers happy; or:
2. Flesh it out into something that makes these strings localizable by the user in a non-hackish fashion.

(What you do after having passed review is up to you.)

Now, regarding best practices in Drupal I think you should always "store exactly what the user typed". (Source: Handle text in a secure fashion.)

In other words: Preserve user input coming in and sanitize going out.

swirt, my remark about sanitizing "best practices" was primarily intended for gwprod, not you.

gisle is completely correct, I misspoke. The standards require that what the user types is preserved, and that your code be designed in such a way that it doesn't matter what they type, it will not cause a security breach.

Please keep in mind though that PAreview was happy.

While warnings does not throw PAReview into a state of chronic depression of the kind that inflicts its cousin Marvin, it becomes mildly melancholic after issuing a warning. (But IMHO the t() wrapper around the variable is not a blocker.)

Automated Review

PAReview complained about a few long lines. These are not substantial and fixing all issues is not a requirement for getting through the application process.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes. Similar Projects and how they differ is adequately addressed on project page.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code/content

Yes: Follows the guidelines for 3rd party code.

README.md

Yes: Follows the guidelines for in-project documentation. It is named README.md, which we have a policy of accepting.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Follows guidelines for writing secure code.

Coding style & Drupal API usage

Yes. All issues that have been reported are fixed.

There are IMHO no more blockers. Moving to RTBC. Note that promotion will not happen until a git administrator has given this a second set of eyeballs.

Thanks for your contribution, swirt!

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.