Note: the module has now been released as a full project: https://drupal.org/project/date_repeat_entity

The Date Repeat Entity Module complements the Date suite of modules, in particular the Date Repeat module. For repeating dates that follow a recurring rule, this module creates a unique entity for each date instance in the series. There is more discussion in the Date queue that describes the need for this module.

Please refer to the following links:

Reviews of other projects

  1. https://drupal.org/comment/8437255#comment-8437255
  2. https://drupal.org/node/2273733#comment-8816509
  3. https://drupal.org/node/2275023#comment-8823709
CommentFileSizeAuthor
#18 coder-results.txt9.33 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eft’s picture

Issue summary: View changes
eft’s picture

Issue summary: View changes
PA robot’s picture

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.

MrPaulDriver’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

A valuable and much needed module which fills a big space between the utility of the Date module and the need for specific dated nodes, repeating as part of a sequence. Event and registration sites in particular require such a solution.

In addition to posts in the Date module issue queue, there have been a number of other projects which have attempted to address this need. Only a few of these have been ported to D7 and those that have are encumbered by limitations of one sort or another.

https://drupal.org/node/1490058
https://drupal.org/project/node_recur
http://drupal.org/project/date_repeat_nodegen
http://drupal.org/project/node_repeat
http://drupal.org/project/date_repeat_sequence

I have tested Date Repeat Entity extensively over the last few days and have found it to be robust and free of any significant problems.

eft’s picture

Title: Date Repeat Entity » [D7] Date Repeat Entity
Issue summary: View changes
eft’s picture

Issue summary: View changes
jgullstr’s picture

Status: Reviewed & tested by the community » Needs work

Hi eft,

I checked your module out last year while looking for something similar. It wasn't ultimately what I was looking for that time, but I can see a need for this module and believe that it fills a gap in what's currently available in contrib.

I have done a quick walk-through using your example feature, and everything works great so far. I will do more exhaustive testing later if and when I find the time.

Your code is very easy to read with thorough in-code documentation, great job on that! I did a small manual review and these are some issues and pointers I found so far:

  • Re-check pareview.sh - there's a few small errors to fix.
  • The name "Date repeat entity", as well as README.txt implies that the module can be used for different entity types. If I'm not wrong, only nodes are supported at this time. This might be good to clarify.
  • Since you're using metadata wrappers, you should add a dependency to the Entity API module.
  • Boolean and text fields are implicitly required from the required fields, so list and text could be added as dependencies, too. Not that I've heard of a site not using text :)
  • You could add the settings path to your .info file, to get a configure link to appear on the module list.
    configure = admin/config/date/date_repeat_entity
  • Instead of having to create fields manually, creating the required fields during module installation would save users some hassle. Additionally, this allows you to namespace your fields (ie date_repeat_entity_master_uuid, date_repeat_entity_clone_state). Something like this should work:
      $field_name = DATE_REPEAT_ENTITY_FIELD_MASTER_UUID;
      if (!field_info_field($field_name)) {
        $field = array(
          'field_name' => $field_name,
          'type' => 'text',
          'locked' => FALSE,
        );
        $field = field_create_field($field);
      }
      $field_name = DATE_REPEAT_ENTITY_FIELD_CLONE_STATE;
      if (!field_info_field($field_name)) {
        $field = array(
          'field_name' => $field_name,
          'type' => 'list_boolean',
          'locked' => FALSE,
        );
        $field = field_create_field($field);
      }
    

    You could then add and remove instances when configuring bundles on the settings form. If you decide to add these, an update function that migrates old field data could be useful (also remember to delete the fields on uninstall).

  • Currently, you're limiting access to the uuid and clone_state fields in form_alter. An alternative approach would be to use hook_field_access to also prevent access from other potential contexts.
  • I would recommend to use hook_field_attach_form instead of hook_form_alter. This hook is only invoked on entity forms with field data attached (ie entity edit forms), and provides $entity_type and $entity as parameters.
  • How will this module behave if the content type has several repeating date fields? However unlikely, instead of just working the first found repeating date field in the bundle, there might be use cases where you'd need to explicitly set which field to work on. One idea might be to enable the module on a date fields instance settings instead of on bundle level.

You might also want to add a link to this thread on the project page, to increase traffic.

This is what I have for now, I'll try to find time to revisit.
Again, great job and thanks for contributing!

//Josef

eft’s picture

Hi Josef,

Thanks for the awesome review and for sharing your knowledge. I have started on a 7.x-2.x branch to address your suggestions and will report back next week.

jgullstr’s picture

Hi again,

It just struck me that the Field API fields aren't necessary, since the data should never be updated by users, right? A cleaner approach would be to create a db table for the settings with foreign keys from the table(s) field_data_{DATE_FIELD_NAMEs}, and uuid and clone state fields. Then add the date repeat entity data to relevant entity objects on load using hook_entity_load. Save/update data using hook_field_update, hook_field_insert and hook_field_delete. This would involve some refactoring, but fits the use case better IMO (Also removes the need for creating fields, hook_field_access, list and text dependencies etc.).

I hope this makes sense.

//Josef

eft’s picture

Issue summary: View changes
Status: Needs work » Needs review

I have made a number of changes to the module in response to suggestions above (comments #7 and #9) and incorporated them into a 7.x-2.x branch. See the CHANGELOG.txt for details. I think these changes improve the usability of the module significantly. Suggestions that did not make into new branch are:

From #7 :
- I did not write an update function to migrate from 7.x-1.x branch since this is not yet a full project.
- Module does not extend to the granular level of date fields instance. It seems like an edge case to have multiple repeating date fields attached to an entity and the level of coding required would be disproportionate.

From #9 :
- I kept the master UUID and and clone state as Field API fields. This is partly because of the additional effort to manage the data outside the File API but also because my hope is that both fields can be added to the Date schema in the future.

I hope these improvements make this module worthy of full project status and welcome any additional feedback.

ram4nd’s picture

  • Remove .gitignore from your git.
  • date_repeat_entity_admin_settings() - $form['#prefix'] is not translatable.
  • date_repeat_entity_add_confirmation_buttons() - Create message part is long, messy and untranslatable.
eft’s picture

Issue summary: View changes
eft’s picture

@ram4nd - Thanks for your feedback. Changes made.

heddn’s picture

Issue summary: View changes
heddn’s picture

Status: Needs review » Reviewed & tested by the community

Nothing blocking RTBC.

.module file:
function date_repeat_entity_field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode)
Why does this seem to limit to use only on nodes and not allow attaching to user, taxonomy and other custom entities? I see a similar comment earlier in this issue. Please provide clarification in the README and projec page.
function date_repeat_entity_add_confirmation_buttons(&$form, $form_state)
PHPDoc comment does not match function or method signature (at line 97)
function date_repeat_entity_get_referenced_entities($form, $form_state)
Missing @return tag in function/method PHPDoc comment (at line 388)

api.php file:
function hook_repeating_date_update($date_entity, $updated_entity, $entity_type = 'node')
PHPDoc comment does not match function or method signature (at line 82)

update.inc file:
function _date_repeat_entity_get_updated_dates($form_state, $entity_id, $entity_type, $bundle, $scope = 'all')
Unused parameter $scope (at line 278)

.install file:
Instead of adjusting the system weight, consider using <a href="https://api.drupal.org/api/drupal/modules%21system%21system.api.php/function/hook_module_implements_alter/7">hook_module_implements_alter()</a>

admin.inc file:
$form['#prefix'] = '<h2>' . t('Date Repeat Entity Administration') . '</h2>';
Considering using theme_html_tag

$form['title'] = array(
        '#type' => 'html_tag',
        '#tag' => 'h2',
        '#attributes' => array('class' => 'title'),
       '#value' => t('Date Repeat Entity Administration'),
);

$form['#submit'][] = 'date_repeat_entity_admin_settings_submit';
I don't think this is necessary. Forms automatically use their base_name_submit for their submit handler.

eft’s picture

Issue summary: View changes
eft’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus
klausi’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
9.33 KB

Review of the 7.x-2.x branch:

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

manual review:

  • date_repeat_entity_install(): do not juggle with module weights, since that is unreliable. Use hook_module_implements_alter() instead if you must run before/after a certain hook.

but otherwise looks good to me, so ...

Thanks for your contribution, eft!

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:

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.

eft’s picture

Thanks Klausi and all the other reviewers!

Which version of PHP_CodeSniffer did you run against this project?

eft’s picture

Issue summary: View changes
eft’s picture

Issue summary: View changes
eft’s picture

Note: this module has now been released as a full project: https://drupal.org/project/date_repeat_entity

Status: Fixed » Closed (fixed)

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