Add skeleton for storing info on digests to send

1. +++ b/src/Form/DigestSettingsForm.php
   @@ -0,0 +1,127 @@
   +    $instance = parent::create($container);
   +
   +    $instance->entityTypeManager = $container->get('entity_type.manager');
   +
   +    return $instance;
   

   I've not seen someone use dependency injection like this before. Where does it come from?

2. +++ b/src/Form/DigestSettingsForm.php
   @@ -0,0 +1,127 @@
   +    /** @var \Drupal\node\Entity\NodeType $contentType */
   +    foreach ($contentTypes as $contentType) {
   

   As discussed, we don't want content types in the config form.

The dependency injection style is a bit of a newer thing. People, myself included, complained about __construct functions having unwieldy long signatures. Turns out many see that function as unnecessary and some modules (like webform) are adopting it.

I don't have a strong opinion on injecting the container vs services. My first thought about it is that injecting the container would make it harder to test by making it harder to mock or otherwise control the service's dependencies in a test, but I don't know the arguments for or against it. I don't think webform doing it is means you should, though. For example, that module thought it was OK for an interface to have 103 public methods https://git.drupalcode.org/project/webform/-/blob/602b05c0e317c556dc162f....

diff --git a/digest.module b/digest.module
new file mode 100644
index 0000000..6497a83
--- /dev/null
+++ b/digest.module
@@ -0,0 +1,18 @@
+<?php
+
+use Drupal\Core\Routing\RouteMatchInterface;
+
+/**
+ * Implements hook_help().
+ */
+function digest_help($route_name, RouteMatchInterface $route_match) {

This file needs to start with an `@file` docblock. See https://www.drupal.org/docs/develop/standards/api-documentation-and-comm... and https://www.drupal.org/docs/develop/standards/api-documentation-samples#... for what that should look like (@file comments in .module files should describe the module's purpose).

+system.admin_config_digest:
+  path: '/admin/config/digest'
+  defaults:
+    _controller: '\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage'
+    _title: 'Digest'
+  requirements:
+    _permission: 'access administration pages'

Should this use the `administer digest settings` permission instead, and specify that it's an admin route?

+  /**
+   * {@inheritDoc}
+   *
+   * Adds the entity type manager to this class.
+   */
+  public static function create(ContainerInterface $container) {
+
+    $instance = parent::create($container);
+
+    $instance->entityTypeManager = $container->get('entity_type.manager');
+
+    return $instance;
+
+  }

You can't mix @inheritdoc with additional comments. {@inheritdoc} has to be the only line in the docblock. https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

+    $form['send_from_email'] = [
+      '#type' => 'email',
+      '#title' => 'Digest from email',
+      '#description' => $this->t('The email to use when sending digests.'),
+      '#default_value' => $config->get('digest.digest_from_email'),
+    ];

The value of #title should be translated too. There's a couple #titles that need this.

+    $form['views_section_tag'] = [
+      '#markup' => $this->t('<h3>Views</h3><p>Choose whether a view should have notifications.</p>'),
+    ];

Don't mix sentences in one `t()` string, and keep markup out of that string except in circumstances where you need it to avoid splitting up one sentence. See https://www.drupal.org/docs/7/api/localization-api/localization-api-over..., and https://www.drupal.org/docs/7/api/localization-api/dynamic-strings-with-... and https://www.drupal.org/docs/7/api/localization-api/dynamic-or-static-lin.... The last one specifically talks about how to handle HTML with translatable strings. For the string you have, you'd want the "Views" heading to be one string and "Choose ... notifications." to be another, and the markup should not be part of the translatable string.

+      foreach ($dateStamps as $dateStamp) {
+
+        $converted = strtotime($dateStamp);
+        if (!$converted) {
+          $form_state->setErrorByName($sendAtName, 'The "When to send" value of '
+            . $view->id() . ' is not formatted properly.');
+        }

That string should be translatable.

It should also help fix the problem. "When to send" could be a really complex value. Besides having to figure out what a valid PHP date string is, the user might have multiple in there. It would help to say which part doesn't work.

It's possible for this error to be set multiple times. If you have three times in there, all invalid, you'd get this same message 3 times. It might be better to build a list of invalid $dateStamps, then set the error once if the list is non-empty. Then the message can say something like what you have now, plus a list of which parts couldn't be parsed.

In additional to syntax errors like using a date that can't be parsed, someone could make the logical error of not writing the schedule that they intended. It might be nice to have feedback about the schedule, such as showing the date and time that each string resolves to.

Overall, I don't like this way of managing the schedule at all, but that's better discussed after looking at the form element that this validation pertains to:

+      $form[$name][$containerName][$sendAtName] = [
+        '#type' => 'textfield',
+        '#title' => 'When to send',
+        '#description' => $this->t('A string containing the times of when
+          to send a digest. For example, "friday 12:00" would mean the digest is
+          sent Fridays at 12pm. Multiple values can be specified by separating
+          them with a comma (,). This means, "friday 12:00, monday" would
+          send a digest on Fridays at 12pm and one on Monday.
+          Consult the PHP date format for more options.'),
+        '#default_value' => $entity_exists ? $digest_entity->getSendAt() : '',
+      ];

This is a super complex form element. Being a text field is simple, but that puts all the complexity on the user instead. Because of that, clear documentation is essential. This description is very good. I think it's missing an explanation of what time on Monday that example would be sent at, and whether you can have a daily or monthly schedule.

It would be a bit easier for the user if it didn't support multiple values this way, and had an "add another time" button to add a separate element for that. It'd also be easier if there were foolproof widgets for it, like selecting "monthly" followed by the day of the month and time, "weekly" followed by the day of the week and time, or "daily" followed by the time.

I think there should be a different format for the schedule - one that's more clearly defined and easier to understand than "whatever it is that PHP's strtotime comes up with". Cron expressions might be a better option. They still have the problem of being cryptic, but they're better defined and not PHP-specific. I'd try to make this form have friendlier widgets, then store the value in a cron format, and probably use something like https://github.com/dragonmantank/cron-expression.

Instead of one Digest per view and multiple schedules, I think you should have one schedule per Digest, and instead of having all the views on this page, saving/updating them all in one spot, you should have a page listing digests, and add/update/delete them individually. I made a drawing of what I mean by that: https://docs.google.com/drawings/d/1P6KNoNJxs98lysDyC-yumRQO-iafr5QCkH-d...

+  /**
+   * Deals with updating and or creating a new digest entity.
+   *
+   * If a digest entity with the given 'id' exists it is updated, otherwise a
+   * new entity is created.
+   * If an entity was enabled and has now been disabled, only the $enabled value
+   * is updated.

The two additional paragraphs should either be separated with a blank line or be one paragraph, wrapped as close to 80 characters as possible. Ie, "new entity is created. If an entity ..." instead of having a line break.

+  /**
+   * Returns the current value of the 'send_at' string.
+   *
+   * @return string
+   *   The current value of 'send_at'.
+   */
+  public function getSendAt();
+
+  /**
+   * Sets the 'send_at' value of the digest.
+   *
+   * This function provides no validation, the '$setTo' is expected to be a
+   * valid format.
+   *
+   * @param string $setTo
+   *   The value to set 'send_at' to.
+   *
+   * @return $this
+   */
+  public function setSendAt($setTo);

The value of send_at should not be a string except when it needs to be serialized. That is, in the config file it would be a string but when it's loaded it should be specific type. If there isn't an appropriate class in PHP's standard library, make one, call it DigestSchedule or whatever, and have Digest::getSendAt and Digest::setSendAt use that type instead of strings.

Smart to make a new issue for the send_at stuff.

I still have a gripe about the digests' IDs. I don't like that they have to match the view. I think the main issue is that you're forced into having multiple schedules in one digest since you can't have two digests referencing the same view. But that's essentially getting back to the send_at discussion.

I don't think you can choose which display of the view to show, which might be an issue if someone wants to send one display on one schedule and another on a different schedule. Ie, one might want their blog newsletter view to be one view with a 'weekly' display that filters by blogs from the last 7 days and a 'monthly' display that filters by the last 30 days, rather than having two separate views that are nearly identical. But that's probably best left as a separate issue too.

Is it too late to discuss not integrating with views? If you selected a block to include in the email instead of a view, people could still send views by adding a block display to the view, and it would be possible for people to get creative by sending any block on a schedule, which could be a custom one that pulls in content from wherever, or shows some other kind of regular report.

I like it a lot more with the list of entities separate from the configuration form. I found a few nitpicky things that could be addressed here.

+++ b/digest.module
@@ -0,0 +1,23 @@
+<?php
+
+/**
+ * @file
+ * The '.module' file for Digest.
+ */

You're not telling anyone anything they didn't already know there. @file should describe the

https://www.drupal.org/docs/develop/standards/api-documentation-samples#... has an example and https://www.drupal.org/docs/develop/standards/api-documentation-and-comm... has another

Some other examples are commerce.module's "Defines common functionality for all Commerce modules." and user.module's "Enables the user registration and login system.".

+/**
+ * Used to build lists of available Digests.
+ */
+class DigestListBuilder extends ConfigEntityListBuilder {

For documenting classes, leave out "Used to" and say "Builds lists of available Digests."

+ * @ConfigEntityType(
+ *   id = "digest",
+ *   label = @Translation("Digest"),
+ *   module = "digest",
+ *   config_prefix = "digest",
+ *   admin_permission = "Administer Digest settings",

The permission should match the permission's machine name, not its title. Make it "administer digest settings".

+    $form['enabled'] = [
+      '#type' => 'checkbox',
+      '#title' => $this->t('Enable'),
+      '#description' => $this->t('Send out notifications using this view?'),
+      '#default_value' => $this->entity->get('enabled'),
+    ];
+
+    $form['title'] = [
+      '#type' => 'textfield',
+      '#title' => 'Digest Title',
+      '#description' => $this->t('This is used in the subscription form for digests.'),
+      '#default_value' => $this->entity->label(),
+      '#required' => TRUE,
+    ];
+
+    $form['description'] = [
+      '#type' => 'textarea',
+      '#title' => 'Digest Description',
+      '#description' => $this->t('This is used in the subscription form for digests.'),
+      '#default_value' => $this->entity->get('description'),
+    ];

There's a few #titles in the forms that need to use t().

Ooooh. I second Travis' idea for using blocks as an abstraction layer. I think this would be worth the effort in replacing the views integration. It also opens the door for fieldable blocks, and many other possibilities.

Looks good to me.