Convert Newsletter categories to custom entity

I played around with option 2 a while ago already and it's not that easy.

One problem is that this would add ~10 fields to the default installation which is way heavier than the current additional table, it's more complicated to support conditional options and the set-up code gets quite complicated and long to configure all these fields.

Option 1 however has it's problems too because we have custom conditions and checks in our loading functions e.g. only load non-hidden fields. This would actually be easier with option 2 as we could use EntityFieldQuery.

re #3: the duplication is in simplenews_newsletter, this issue is about the simplenews_category table. We should open a new issue about that duplication.

re #4: As you probably figured out already, we moved admin/structure/simplenews to configuration. Not sure about the default configuration thing, would require special treatment for that pseudo-category everywhere else. E.g. prevent it from showing up in the taxonomy reference field and so on.

Well, re. the number of fields, it maps directly to the simplenews_category table:

describe simplenews_category;
+---------------+---------------------+------+-----+---------+-------+
| Field         | Type                | Null | Key | Default | Extra |
+---------------+---------------------+------+-----+---------+-------+
| tid           | int(11)             | NO   | PRI | 0       |       |
| format        | varchar(8)          | NO   |     |         |       |
| priority      | tinyint(3) unsigned | NO   |     | 0       |       |
| receipt       | tinyint(3) unsigned | NO   |     | 0       |       |
| from_name     | varchar(64)         | NO   |     |         |       |
| email_subject | varchar(255)        | NO   |     |         |       |
| from_address  | varchar(64)         | NO   |     |         |       |
| hyperlinks    | tinyint(4)          | NO   |     | 0       |       |
| new_account   | varchar(12)         | NO   |     |         |       |
| opt_inout     | varchar(12)         | NO   |     |         |       |
| block         | tinyint(4)          | NO   |     | 0       |       |
+---------------+---------------------+------+-----+---------+-------+

Yes, all these fields would be specific to simplenews, but haven't most fields only single, specific purpose? I mean, we're not talking about field *types* here, we can re-use the existing string/list types. And that does have a few advantages, for example built-in integration with views, rules and other modules which rely on entity(_api module).

We could theoretically create a custom field type with all these values as columns but what would we gain with that? That code wouldn't be re-usable in any way either as we're just moving it around (hook_schema to hook_field_schema, custom form to hook_field_widget and so on).

There is actually a third option here and that would be to define a custom entity type for newsletter categories and stop using taxonomy completely. Instead of a taxonomy reference field, we could use entityreference instead.

That option is probably the best one in the long term and given that, we should probably postpone this issue completely.

This would also allow to use entity access so that custom/other contrib modules could provide implementations for specific permission handling, e.g.:

- #552548: Only send to own created newsletter
- #1140606: Permissions Per-Newsletter: Limit subscription options by role
- #418006: Simplenews in combination with OG

Task to be done:

Initial version:
- Add dependency on entity module
- Define hook_entity_info()
- Update schema, name and description, probably weight as well maybe rename tid to
- Replace current load_multiple() functions with entity_load(), make sure our custom conditions (hidden newsletters) and default sort according to weight is supported (will probably require a custom controller)
- Edit: Upgrade path of existing terms including their fields.

Additional steps, possibly as follow-ups:
- Define entity properties using a metadata controller (see tmgmt_translator as example)
- Make newsletters an exportable entity (again, see tmgmt_translator is an example)
- Use ui controller to replace our custom newsletter list
- Use view controller to provide better views integration.
- Use entity access to control access to newsletters

Ok, here is a new patch.

- Re-rolled for 7.x-2.x
- All tests should be fixed except i18n integration. Will need to re-think that and probably use entity translation and convert the description to a field but that can be a follow-up, not urgent.
- Fixed and improved upgrade path.
- Added a default view for newsletter/id, including a feed.
- Removed all references of tid/category/categories from the code.

The plan is to commit this asap and then fix bugs as they occur, we won't find them in this issue anyway.

The tests pass locally, the testbot just isn't able to cope with the added dependencies. I'll try to commit them separately, maybe that will help.

Did that, updated the patch accordingly. Will not work before the dependencies are rebuilt, which should happen when the dev snapshot for 7.x-2.x is updated.

The patch.

Fixed the test failures and re-rolled. They were caused by the fact that the rssfeed and link to previous issues settings are now only displayed if views.module is enabled.

This should be green.

Hm, testbot is behaving differently, can't reproduce locally, time for some debugging.

Another try.

Ok, let's try this.

Yay!

Removed the debug code again.

+++ b/includes/simplenews.admin.incundefined
@@ -152,38 +152,38 @@ function simplenews_admin_issues() {
-  $query = db_select('node', 'n')->extend('PagerDefault')->extend('TableSort');
-  simplenews_build_issue_filter_query($query);
-  $query->innerJoin('simplenews_newsletter', 'sn', 'n.nid = sn.nid');
-  $query->leftJoin('simplenews_category', 'sc', 'sn.tid = sc.tid');
-  $query->leftJoin('taxonomy_term_data', 't', 'sc.tid = t.tid');
-  //$query->leftJoin('users', 'u', 'ss.uid = u.uid');
-  $query->fields('n', array('nid', 'title', 'created', 'status'))
-    ->fields('sn', array('tid'))
-    ->fields('t', array('name'))
+  // TODO: reenable filtering by newsletter.t r
+  $nids = db_select('node', 'n')

weird characters in comment and filtering gone.

+++ b/includes/simplenews.admin.incundefined
@@ -152,38 +152,38 @@ function simplenews_admin_issues() {
+    foreach (node_load_multiple($nids) as $node) {
+      $categories = simplenews_newsletter_list();

list() call can be moved outside of the foreach.

+++ b/includes/simplenews.admin.incundefined
@@ -152,38 +152,38 @@ function simplenews_admin_issues() {
+      $subscriber_count = simplenews_count_subscriptions(simplenews_issue_newsletter_id($node));

Note to myself: Check if this is statically cached.

+++ b/includes/simplenews.admin.incundefined
@@ -618,80 +592,68 @@ function simplenews_admin_category_form_validate($form, &$form_state) {
-  $form_state['values']['tid'] = $category->tid;
-  $form_state['tid'] = $category->tid;
+  $form_state['values']['newsletter_id'] = $newsletter->newsletter_id;
+  $form_state['newsletter_id'] = $newsletter->newsletter_id;
   $form_state['redirect'] = 'admin/config/services/simplenews';

Wondering why we're doing this, looks useless.

+++ b/includes/simplenews.admin.incundefined
@@ -981,11 +943,11 @@ function simplenews_subscription_list_remove($form, &$form_state) {
+      '#title' => check_plain(_simplenews_newsletter_name($newsletter)),
+      '#description' => _simplenews_newsletter_description($newsletter),

Those helper functions can be removed now, translations will be broken for a while but that's ok. it won't work anymore anyway.

+++ b/includes/simplenews.admin.incundefined
@@ -1697,19 +1659,19 @@ function simplenews_build_subscription_filter_query(SelectQueryInterface $query)
  * @return number of subscribers.
  */
-function simplenews_count_subscriptions($tid) {
+function simplenews_count_subscriptions($newsletter_id) {
   $subscription_count = &drupal_static(__FUNCTION__);

Yes it is cached. Looks like there is no @param documentation here.

+++ b/includes/simplenews.controller.incundefined
@@ -0,0 +1,68 @@
+ * Seimplenews entity controllers definitions.

typo.

+++ b/includes/simplenews.controller.incundefined
@@ -0,0 +1,68 @@
+   * Override EntityAPIController.

OverrideS ..:create().

+++ b/includes/simplenews.controller.incundefined
@@ -0,0 +1,68 @@
+   * Override EntityAPIController.
+   *
+   * Make sure all subscriptions to the newsletters gets deleted.
+   *
+   * @param $transaction
+   *   Optionally a DatabaseTransaction object to use. Allows overrides to pass

param documentation is not necessary.

+++ b/includes/simplenews.controller.incundefined
@@ -0,0 +1,68 @@
+      // Delete subscriptions
+      simplenews_subscription_delete(array('newsletter_id' => $id));
+      drupal_set_message(t('All subscriptions to newsletter %newsletter have been deleted.', array('%newsletter' => $names[$id])));
+
+      // Delete subscription block

fix comment formatting.

+++ b/includes/simplenews.entity.incundefined
@@ -0,0 +1,64 @@
+  public $newsletter_id;
+  public $name;
+  public $description;
+  public $format;
+  public $priority;
+  public $receipt;
+  public $from_name;
+  public $email_subject;
+  public $from_address;
+  public $hyperlinks;
+  public $new_account;
+  public $opt_inout;
+  public $block;
+  public $weight;

Documentation missing.

+++ b/includes/simplenews.entity.incundefined
@@ -0,0 +1,64 @@
+   */
+  protected function defaultUri() {
+    return array('path' => 'admin/config/services/simplenews/categories/' . $this->identifier() . '/edit');

Should be newsletter/id. Also defaultLabel() is missing.

+++ b/includes/simplenews.info.incundefined
@@ -0,0 +1,30 @@
+class SimplenewsNewsletterMetadataController extends EntityDefaultMetadataController {
+
+  function entityPropertyInfo() {
+    $info = parent::entityPropertyInfo();
+    $properties = &$info[$this->type]['properties'];
+
+    $properties['format']['options list'] = 'simplenews_format_options';
+    $properties['priority']['options list'] = 'simplenews_get_priority';

class documentation missing.

+++ b/includes/simplenews.mail.incundefined
@@ -16,6 +16,9 @@
+  if (!simplenews_issue_newsletter_id($node)) {
+    throw new Exception('No newsletter asigned to issue.');

Wondering why this is here.

+++ b/includes/simplenews.mail.incundefined
@@ -36,10 +39,7 @@ function simplenews_add_node_to_spool($node) {
-
-  // Notify other modules that a newsletter was just spooled.
-  module_invoke_all('simplenews_spooled', $node);
+  simplenews_issue_update_sent_status($node);

Wondering why the hook was removed?

+++ b/includes/simplenews.mail.incundefined
@@ -290,10 +289,10 @@ function simplenews_mail_spool($limit = SIMPLENEWS_UNLIMITED, array $conditions
+      // @todo: This is not thread save, consider a lock or a direct query.
+      simplenews_issue_sent_count($node, simplenews_issue_sent_count($node) + $count);

Needs to be fixed or parallel runs will break.

+++ b/includes/simplenews.mail.incundefined
@@ -543,9 +542,9 @@ function simplenews_count_spool(array $conditions = array()) {
   return db_delete('simplenews_mail_spool')
-    ->condition('status', SIMPLENEWS_SPOOL_DONE)
-    ->condition('timestamp', $expiration_time, '<=')
-    ->execute();
+      ->condition('status', SIMPLENEWS_SPOOL_DONE)
+      ->condition('timestamp', $expiration_time, '<=')
+      ->execute();

looks wrong.

+++ b/includes/simplenews.mail.incundefined
@@ -620,10 +619,9 @@ function simplenews_send_status_update() {
-      db_update('simplenews_newsletter')
-        ->condition('nid', $nid)
-        ->fields(array('status' => SIMPLENEWS_STATUS_SEND_READY))
-        ->execute();
+      $node = node_load($nid);
+      simplenews_issue_status($node, SIMPLENEWS_STATUS_SEND_READY);
+      node_save($node);

same here.

+++ b/includes/simplenews.mail.incundefined
@@ -719,26 +717,26 @@ function _simplenews_absolute_mail_urls($match) {
 function _simplenews_html_replace() {
   return array(
-    '/"/i'  => '"',
-    '/>/i'    => '>',
-    '/&lt;/i'    => '<',
-    '/&amp;/i'   => '&',
-    '/&copy;/i'  => '(c)',
+    '/&quot;/i' => '"',
+    '/&gt;/i' => '>',
+    '/&lt;/i' => '<',
+    '/&amp;/i' => '&',
+    '/&copy;/i' => '(c)',
     '/&trade;/i' => '(tm)',
-    '/&#8220;/'  => '"',
-    '/&#8221;/'  => '"',
-    '/&#8211;/'  => '-',
-    '/&#8217;/'  => "'",
-    '/&#38;/'    => '&',
-    '/&#169;/'   => '(c)',
-    '/&#8482;/'  => '(tm)',
-    '/&#151;/'   => '--',
-    '/&#147;/'   => '"',
-    '/&#148;/'   => '"',
-    '/&#149;/'   => '*',
-    '/&reg;/i'   => '(R)',
-    '/&bull;/i'  => '*',
-    '/&euro;/i'  => 'Euro ',
+    '/&#8220;/' => '"',
+    '/&#8221;/' => '"',
+    '/&#8211;/' => '-',
+    '/&#8217;/' => "'",
+    '/&#38;/' => '&',
+    '/&#169;/' => '(c)',
+    '/&#8482;/' => '(tm)',
+    '/&#151;/' => '--',
+    '/&#147;/' => '"',
+    '/&#148;/' => '"',
+    '/&#149;/' => '*',
+    '/&reg;/i' => '(R)',
+    '/&bull;/i' => '*',
+    '/&euro;/i' => 'Euro ',

unrelated formatting change?

+++ b/includes/simplenews.mail.incundefined
@@ -759,7 +757,7 @@ function _simplenews_measure_usec($start = FALSE) {
-  $now = (float)($usage['ru_stime.tv_sec'] . '.' . $usage['ru_stime.tv_usec']) + (float)($usage['ru_utime.tv_sec'] . '.' . $usage['ru_utime.tv_usec']);
+  $now = (float) ($usage['ru_stime.tv_sec'] . '.' . $usage['ru_stime.tv_usec']) + (float) ($usage['ru_utime.tv_sec'] . '.' . $usage['ru_utime.tv_usec']);

same here.

+++ b/includes/simplenews.source.incundefined
@@ -280,6 +280,8 @@ class SimplenewsSpool implements SimplenewsSpoolInterface {
+    // TODO: What is wrong with the cache?
+    entity_get_controller('node')->resetCache();
     $node = node_load($spool_data->nid);

Need to check if this is still necessary.

+++ b/includes/simplenews.subscription.incundefined
@@ -146,8 +147,9 @@ function simplenews_block_form($form, &$form_state, $tid) {
+  // #newsletter_id carries the newsletter_id for processing of the right
+  // newsletter.
+  $form['#newsletter_id'] = $newsletter_id;
   $form['#validate'][] = 'simplenews_block_form_validate';

change to $form_state while it's changed anyway?

+++ b/includes/views/simplenews.views_default.incundefined
@@ -0,0 +1,102 @@
+  $views['newsletter_issue_list'] = $view;
+  return $views;
+}
\ No newline at end of file
diff --git a/simplenews.api.php b/simplenews.api.php

missing newline.

+++ b/simplenews.api.phpundefined
@@ -32,7 +32,7 @@
- * @defgroup newsletter Newsletter (category)
+ * @defgroup newsletter (category)

messed up defgroup definition.

+++ b/simplenews.moduleundefined
@@ -680,15 +566,135 @@ function simplenews_add_term_field($type) {
 /**
- * Remove the taxonomy term field for the newsletter category.
+ * Add field for status
+ *
+ * @param $type A node type object.
+ *
+ * @ingroup issue

docblock needs to be fixed.

+++ b/simplenews.moduleundefined
@@ -680,15 +566,135 @@ function simplenews_add_term_field($type) {
+ * Add field for status
+ *
+ * @param $type A node type object.
+ *
+ * @ingroup issue

same.

+++ b/simplenews.moduleundefined
@@ -1814,234 +1834,120 @@ function simplenews_views_api() {
+  return isset($result['simplenews_newsletter']) ? simplenews_newsletter_load_multiple(array_keys($result['simplenews_newsletter'])) : NULL;

returning array() might allow to avoid some if checks.

+++ b/simplenews.moduleundefined
@@ -1814,234 +1834,120 @@ function simplenews_views_api() {
  * @param $newsletter
- *   Simplenews newsletter object.
+ *   Newsletter entity object
  *
- * @ingroup issue
+ * @ingroup newsletter
  */
 function simplenews_newsletter_save($newsletter) {
-  db_merge('simplenews_newsletter')
-    ->key(array('nid' => $newsletter->nid))
-    ->fields(array(
-      'tid' => $newsletter->tid,
-      'status' => $newsletter->status,
-    ))
-    ->execute();
+  return entity_save('simplenews_newsletter', $newsletter);

We can remove this wrapper.

+++ b/simplenews.moduleundefined
@@ -2131,31 +2034,22 @@ function simplenews_help($path, $arg) {
 function _simplenews_newsletter_name($newsletter, $langcode = NULL) {
-  if (module_exists('i18n_taxonomy')) {
-    return i18n_taxonomy_term_name($newsletter, $langcode);
-  }
-  return $newsletter->name;
+  return $newsletter->getTranslation('name', $langcode);

This only does something if we either define a i18n string or if it's a field.

+++ b/simplenews.moduleundefined
@@ -2300,7 +2231,7 @@ function simplenews_subscription_confirmation_text($key) {
 function _simplenews_get_node_form_defaults() {
   $defaults = array(
-    'tid' => '0',
+    'newsletter_id' => '0',
     'status' => SIMPLENEWS_STATUS_SEND_NOT,

I think we can get rid of this stuff.

+++ b/simplenews.moduleundefined
@@ -2587,36 +2518,107 @@ function simplenews_node_tab_access($node) {
+ *
+ * @param type $node The issue to get or set the status.
+ * @param type $value The status to be set or NULL.
+ * @return int The newsletter status. Defaults to SIMPLENEWS_STATUS_SEND_NOT.

param documentation wrong.

+++ b/simplenews.moduleundefined
@@ -2587,36 +2518,107 @@ function simplenews_node_tab_access($node) {
+ * Get or set the sent count of a newsletter issue.
+ *
+ * If there is $value present, the value is set for issue.
+ *
+ * @param type $node The issue to get or set the status.
+ * @param type $value The sent count to be set or NULL.
+ * @return int The newsletter sent count. Defaults to 0.

same.

+++ b/simplenews.tokens.incundefined
@@ -14,10 +14,10 @@ function simplenews_token_info() {
-  $types['simplenews-category'] = array(
-    'name' => t('Simplenews newsletter category'),
-    'description' => t('Tokens related to the newsletter category'),
-    'needs-data' => 'category',
+  $types['simplenews-newsletter'] = array(
+    'name' => t('Simplenews newsletter'),
+    'description' => t('Tokens related to the newsletter'),
+    'needs-data' => 'newsletter',

We might be able to get rid of this, this is provided by entity.module for entity properties.

Fixed most of the above points except the token stuff.

Also re-enabled the newsletter issue translation tests and removed the newsletter translation test until that is being re-added and then it will be completely different anyway.

Added a lock for updating the simplenews sent count and fixed the test fail. I think this is ready.

Commited. We'll deal with bugs in follow-ups.

The new version is a development release only that is not visible on the project page and the release note says "Do not use this, this is in heavy development mode and probably does not work.". I'd be more worried about that than the missing fields :)

It would be possible to make the new entity fieldable, although that won't be an option in 8.x when it becomes a config entity.