[D7] Upcoming Content

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

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.

Nice work, looks good to me.

1. The generate view helper function (upcoming_content_gen_view()) in .module file, which defines views object can be moved into separate file.
2. The Content on line 22 in "upcoming_content_wrapper.tpl.php" file needs little formatting.
3. You're not using "$opts" variable in "upcoming_content_wrapper.tpl.php" file, please remove if not required.
4. Also you're setting keys "query" and "content_type" in $opts twice, please remove 1st one if not required.

upcoming_content.install:
"incase" -> "in case"

upcoming_content_pane.inc, line # 79

'#options' => $ct_options,
this variable can be not defined on this stage, probably you need to define it earlier

1- Unabe to configure.
2- Unused file upcoming_content.install.
3- Getting the following warning on first installation of this module.
The Date API requires that you set up the site timezone and first day[warning]
of week settings and the date format settings to function correctly.
4- Please tell me how to use this module. I followed your project description but unable to configure or use it.

@svajlenka,

It's seems to be incorrect and insecure with AJAX. At first of all you should use 'delivery callback' as 'ajax_deliver' for AJAX in hook_menu(). Also add validation for $content_type to prevent error during field instance getting.

It's bad solution to use $_REQUEST['content_type'] in upcoming_content_bundle_fields_callback().

A lot of comments to functions have non-standard spaces before '/*' symbols.

Hello svajlenka,
the module is quite interesting.
Code is well written, I can't find any evident shortcomings. Here a pair of suggestions:

1) Add the module folder name to the repo clone string, e.g.,
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/svaj/2145645.git upcoming_content

2) Being not familiar with the page manager module, I'm following instructions to install. I read:

* In the page manager ( /admin/structure/pages ), in a page, under content, click "Add Content"

But there, I can only Enable or Edit pages. Editing a page allows me to “add variants” but I don’t see any Add Content or Miscellaneous, to continue the settings. Something I am missing?

Hi,

Was wondering much of the functionality of the module can be made using views module. Couple of configuration using views we can show *Upcoming Content* based on some filter criteria and create a block of those content and show it on the site.

> My project, upcoming_content provides a ctools content_type pane to easily display nodes that are "upcoming", grouped by the time period the site builder chooses

I don't understand what you mean by 'upcoming'.

Also, even if it is accepted that this is an improvement over building a view (which is yet to be shown), it's surely more flexible to expose a block rather than a Panels content pane? That allows site builders to use whatever layout system they want.

Automated Review

The automated review of your project by PAReview has found some issues with your code. As coding standards make sure projects are coded in a consistent style we ask you to please have a look at the report and try to fix them. There are all formatting issues that are simple to fix.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Yes

No duplication

No. Duplicates trivial use of Views. There may be some utility of having a simple module for admins that don't know how to use Views, but the existence of this overlap must be stated on the project page.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

Project page

No. Please take a moment to make your project page follow tips for a great project page. In particular, you should add the section "Similar projects and how they are different" and note that it should also be possible to create a list of upcoming events by means of the Views module (re: #19).

README.txt

No: Please take a moment to make your README.txt follow the guidelines for in-project documentation and the README.txt Template.
Also, the folowing line does not make sense: 'In the page manager ( /admin/structure/pages ), in a page, under content, click "Add Content"', and I am unable to follow the rest of the instructions.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Postponed until README.txt is fixed. If "no", list security issues identified.

Coding style & Drupal API usage

My notes so far:

• (*)Do not place a php statement in the .info-file for Drupal 7 unless they specifically need a higher later version of PHP than is required by core. See Writing module .info files and System requirements.
• (*) You have several places where you are using php functions directly, and not the Drupal equivalent, eg date() instead of format_date(). This may break localization.
• There is no hook_help(). It is good coding practice to have this hook for every enabled module.
• There is a typo un the docblock for hook_menu().

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.

Setting status.

My primary concern is with the AJAX callback. Under normal circumstances only site admins can access the customization interface for Panels, Panelizer or IPE, thus an AJAX callback would only be accessible by someone with access to one of these interfaces. On the other hand, almost every site grants the anonymous user the 'access content' permission, thus any anonymous user (including bots) could access the AJAX callback and potentially trigger a DDOS attack on the site. I recommend adding an access callback and checking for additional permissions.

For people questioning the value of this module, there are many projects where functionality like this is required, where the editorial team want a simple way of selecting multiple content types and a date filter to show content from that time range. While this module could use some improvements, and such improvements should be handled via issues in its queue, I certainly think it fits a niche that Views cannot fill.

DamienMcKenna, I haven't looked at the AJAX callback yet. I am postponing any detailed review until the instructions about how to set it up are clear. But if you think there is a security problem, you should tag it with PAReview: security.

Also: I am not questioning the value of the module. I just want the overlap with Views made clear on the project page, so that users know what to expect and also know that there is an alternative.

@gisle: No problem. FYI I started writing my review before you'd posted the PAReview, so my comment about the module's value being questioned was more towards joachim.

Just a brief note (this is not a review). I subscribe to the security list, and from it, I've learned that the security team is not happy about solutons that allow users with "administer something" permissions to carry out XSS or injection attacks. Allowing unsanitized user input, even from an administrator, is IMHO a blocker.

Why don't you just sanitize it?

A couple things I notice:

• You'll want to add a hook_uninstall() in upcoming_content.install to remove your variables
• In theme/upcoming_content_item.tpl.php I think you want to use url() instead of drupal_get_path_alias()
• In theme/upcoming_content_item.tpl.php I think this looks strange: $node->node_title, is that because it's coming from Views? I think you also want to use check_plain() when printing that.
• In theme/upcoming_content_item.tpl.php I think $node->available_date should be the configured date field, just like is done with the image field.

> In theme/upcoming_content_item.tpl.php I think you want to use url() instead of drupal_get_path_alias()

That's true. And additionally, you should also call functions in the preprocessor, rather than in the tpl.

> In theme/upcoming_content_item.tpl.php I think this looks strange: $node->node_title, is that because it's coming from Views? I think you also want to use check_plain() when printing that.

Views sanitizes its output. To be certain that this is fine, try making a node with markup that should be escaped in its title.

> upcoming_content_get_image_fields

This returns full field names. But the TPL prepends 'field_' to them. That seems problematic. It's generally a better idea to store the full names of things to avoid confusion. Here there is the extra complication that not all fields have to have the 'field_' prefix!

> * Define the ctools plugin and it's defaults.

Typo. Should be 'its'.

> Regarding the use of a block over a panel:
> A block does not allow for per-instance configuration.

Fair enough. But I suggest you consider Boxes plugins, at least for your future projects. A panel plugin forces your user to use panels on their site.

Automated Review

PAReview came up clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes.

While this module duplicates trivial use of Views, this module have a different target group (those who don't have the skills to configure Views themselves a prefer a ready to-run-module) and solves the problem in a different way. This is adequately explained on the 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.txt

One of the steps during Configuration:

* In the page manager ( /admin/structure/pages ), in a page's variant, under 
content, click "Add Content"**

I am unable to locate the "Add content" button in a page's variant, see image below.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Postponed until README.txt is sorted out.

Coding style & Drupal API usage

My notes:

• (*) Do not use the package name "Features" in your .info-file. It is reserved for modules that leverages on the Features project. In your case, there should be no package statement in the .info-file. See Module documentation guidelines: Follow package naming conventions.
• You have at least one place where you are using a PHP function directly, and not the Drupal wrapper. Please see PHP wrappers for more information.
  Example: You use strtolower() instead of drupal_strtolower(). This will not properly lowercase an UTF-8 string. .
• In upcoming_content_pane.inc you're calling time() to get the current timestamp. Use REQUEST_TIME instead.

The starred item (*) warrants going back to Needs Work. The rest of the comments in the code walkthrough are recommendations.

Updating status.

I am unable to locate the "Add content" button in a page's variant, see image below.

You have to first click the gear icon to the top-left of the pane, that will open a drop-down menu that includes "Add content" as one of the items.

The "Add content" thing is a basic Panels training issue, I don't think believe it's necessary that this module's README.txt should have to explain how to use Panels, IMHO it should just mention e.g. "A new item will be available under the 'Add content' menu in all Panels-based pages."

The "Add content" thing is a basic Panels training issue, I don't think believe it's necessary that this module's README.txt should have to explain how to use Panels

No, but the project page should at least said something like:

"The target audience for this project is experienced Panels users who ..."

If that had been the case, I wouldn't have tried to review it.

I now realize that I am not in the target audience for this project.

I shall to be able to finish my review of this project, so I am taking it off my TODO-list.

It looks like all the recommendations have been fixed, and there haven't been any new recommendations. I think all that was left was a security review.

I looked through the code and don't see any security issues, as well as ran a Secure Code Review through Coder and it only reported minor style issues:

upcoming_content.module

severity: minor review: style_trailing_spacesLine 54: There should be no trailing spaces [style_trailing_spaces]
return '

' . t('Upcoming Content provides configurable panels to easily
severity: minor review: style_trailing_spacesLine 55: There should be no trailing spaces [style_trailing_spaces]
display content about future items. From the IPE or page manager you
severity: minor review: style_trailing_spacesLine 56: There should be no trailing spaces [style_trailing_spaces]
may add an upcoming panel to any page variant. The upcoming content
severity: minor review: style_trailing_spacesLine 57: There should be no trailing spaces [style_trailing_spaces]
panel can be found in the Miscellaneous category when you are adding

Since style issues aren't blockers for full project status, I'm marking this as RTBC.

I skimmed the code for security issues, and it looks good to me. The updates to the AJAX command seem to satisfy DamienMcKenna's concerns. Output goes through appropriate Drupal APIs (render, theme, check_plain, filter_xss), and I see that a filter is present to exclude unpublished content.

Checked for security, licensing, Drupal API, and individual account, did not find any problems. Nice work on the security and theming!

Non-blocking issues:

• You don't need the backticks around drush commands in README.txt
• You do mention that this module is for those familiar with adding content via panels, consider a stronger statement that indicates this module can only be used with panels, or is a panels add-on. This is just a note, not required at all.
• In your $plugin defaults, 'uid' => REQUEST_TIME seems like an odd value to use
• Instead of node_type_get_types() you may be able to use node_type_get_names() instead if that's all you're after

Thanks for your contribution, svajlenka!

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.