[D7] sometimeago

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

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.

Hi, cool submission! Here are my nitpicks/comments, in no particular order:

1. Spelling mistake in "You mus enter the days as a number."
2. The Git command in the top post if wrong, it should be "git clone --branch master http://git.drupal.org/sandbox/thelynge/2242143.git sometimeago"
3. Generally you should not use the "master" branch, and should use a core-version-specific branch, like "7.x-1.x".
4. You're building a DB query dynamically but through direct string concatenation. Instead you should use placeholder syntax or the object-oriented approach (either SelectQuery or EntityFieldQuery). You can read more here: https://drupal.org/node/310075
5. I found the admin labels "How many days ago should the content approximately have been published" and "Allowed range of days forward in time" to be unnecessarily confusing. I would have understood better if it were "Start date (in days ago)" and "End date (in days ago)".
6. Instead of only providing titles/links, you should provide the template with a fully rendered node, and allow the user to specify the view mode to display. This way if they want only titles, they can do that, but if they want teasers or full content, they can do that too. It also gives themers more to work with if they override the template.
7. Users will probably want the option to sort the nodes in descending order.
8. Generally I've seen constants being declared outside of any functions, rather than in hook_init(). I could be wrong but that strikes me a slightly dangerous.
9. I don't think you need the back-up query that looks for older content. If the user's settings don't pull enough content, then that is their problem. It actually degrades the experience I think if the module does something outside of how it is configured.

Hope this helps, good luck!

Some more general feedback, after further thought: I think that this idea may suffer from being something that the Views module could do instead. I'd recommend thinking more on the use-case that you had in mind, and see if you can add some additional features/functionality that would justify using this as an alternative to making a Views view with start/end date filters.

you need to set the status to "needs review" if you want to get a review, see the workflow: https://drupal.org/node/532400

If #9 is the whole point of the module, I think it needs to be detailed in the README. I have other comments but will hold off until I see an explanation of this double-query functionality.

Also a coding note: You can put the sort direction directly into your queries, and then you wouldn't have to sort the arrays afterwards. Probably not a huge deal but it will avoid some PHP load.

/**
 * Add admin configuration page to the section "Modules".
 */
function sometimeago_menu() {

This is a hook implementation, so should follow standards for documenting it. (There are several instances of this!)

function sometimeago_init() {
  define('SOMETIMEAGO_DEFAULT_CONTENTTYPE', 'article');

Using hook_init() for defining constants is weird.

  $items['admin/config/modules'] = array(
    'title' => 'Modules',
    'description' => 'Module configuration',

It's not recommended to invent a new subseection of config, unless you're part of a big area of functionality and you're going to provide lots of config subpages. That's not the case here.

/**
 * Access: Currently no access limitations.
 */
function sometimeago_access() {
  return TRUE;
}

Nobody seems to call this!

> 9. the backup query is basically the whole point of the module so I'm keeping it. The goal is not to show content from exactly x days ago, it is to fill up a teaser with content from approximately x days ago. If you don't write a whole lot of content, the teaser would be half empty all the time. The backup query increases my chances of having a teaser with a full limit of content. With the views module I wasn't able to get this.

I've read through the queries, and I don't see why this can't be done using Views.

  // Query for primary content.
  $query = new EntityFieldQuery();
  $query->entityCondition('entity_type', 'node')
        ->propertyCondition('status', 1)
        ->propertyCondition('created', array($timestamp_primary, $timestamp_secondary), 'BETWEEN');
  if ($contenttype && $contenttype != '') {
    $query->propertyCondition('type', $contenttype);
  }
  $query->propertyOrderBy('created');
  $query->range(0, $limit);
  $result = $query->execute();

You are asking for $limit nodes, created between $timestamp_primary, $timestamp_secondary.

If you get fewer than $limit back, then:

    $query_old_content = new EntityFieldQuery();
    $query_old_content->entityCondition('entity_type', 'node')
                      ->propertyCondition('status', 1)
                      ->propertyCondition('created', $timestamp_primary, '<');
    if ($contenttype && $contenttype != '') {
      $query_old_content->propertyCondition('type', $contenttype);
    }
    $query_old_content->propertyOrderBy('created');
    $query_old_content->range(0, $limit_old_content);
    $result_old_content = $query_old_content->execute();

This is asking for ($limit - how many you got in the first query) that are older than $timestamp_primary.

So surely if you simply query for $limit nodes that are older than $timestamp_secondary, and order your nodes by created time descending, then either:

* you get $limit nodes that happen to be newer than $timestamp_primary
* you get fewer that are newer than $timestamp_primary, and the query ordering takes care of getting you older nodes that the end.

Unless I am missing something here, this could all be done in Views.

> But I want the primary content from the first query and only when there's not enough there (content newer than the start date), I want the older content only if there's not enough newer content.

But that's exactly what you'd get with the Views configuration I described.

Suppose I want 5 nodes. Today is 31 July. I want ideally nodes created since 1 July. If we have these nodes:

* 31 July
* 27 July
* 23 July
* 14 July
* 9 July
* ---- cut-off point
* 28 June

-- then I have 5 nodes since 1 July, and all is fine: the view returns those nodes. The 28 June node is not returned, because it's not in the first 5.

Now suppose we don't have enough nodes in July:

* 31 July
* 14 July
* 9 July
* 28 June
* 23 June
* ---- cut-off point
* 14 June

Now the first 5 nodes takes us further back. We need to include the 23 June and 14 June ones to get 5 nodes.

Have I misunderstood something? What would your module do differently?