Cannot send on publish if node is new

Thanks.

1. +++ b/simplenews.module
   @@ -111,14 +111,43 @@ function simplenews_node_view(array &$build, NodeInterface $node, $display, $vie
   +  if ($node->id() !== NULL) {
   +    return simplenews_node_send_on_publish($node);
   +  } // else simplenews_node_insert will catch it.
   

   We don't really do comments like that. Should be above the code and explain what the code below is doing.

   Also, I think if (!$node->isNew()) is a bit more self-explanatory.

2. +++ b/simplenews.module
   @@ -111,14 +111,43 @@ function simplenews_node_view(array &$build, NodeInterface $node, $display, $vie
   +  if (simplenews_node_send_on_publish($node)) {
   +    // Need to save again to capture changed simplenews_issue->status.
   +    $has_revs = $node->isNewRevision();
   +    $node->setNewRevision(FALSE);
   +    $node->save();
   

   so this will only do it if it is set to that value.

   That's not very clear from the function name, so we should have some comment that explains when and under which condition this is called.

   Also not sure if it really makes sense to have a helper function here. Having different explicit checks in those two hooks might better explain what the code is doing?

3. +++ b/simplenews.module
   @@ -111,14 +111,43 @@ function simplenews_node_view(array &$build, NodeInterface $node, $display, $vie
   + * @return bool
   + *   True if the node was spooled, or false if the node was not sent as a newsletter.
   

   coding style nitpick: there should be an empty line between the @param's and the @return. descriptions should be kept < 80 characters. initial line of a function should below, descriptions for return/param should be split to multiple lines.

4. +++ b/simplenews.module
   @@ -111,14 +111,43 @@ function simplenews_node_view(array &$build, NodeInterface $node, $display, $vie
   -    return;
   +    return false;
   

   coding style nitpick: FALSE

1. +++ b/simplenews.module
   @@ -111,14 +111,49 @@ function simplenews_node_view(array &$build, NodeInterface $node, $display, $vie
   +  // Send newsletters that were only waiting to be published, unless they do
   +  // not exist in storage or have an id yet - the spool storage needs those.
   +  if (!$node->isNew()) {
   +    return simplenews_node_send_if_published($node);
   

   new function name works for me. comment is also almost fine, the "or have an id yet" part seems confusing since we don' check for that and it also doesn't really seem valid english to me? I think we can just remove that.

2. +++ b/simplenews.module
   @@ -111,14 +111,49 @@ function simplenews_node_view(array &$build, NodeInterface $node, $display, $vie
   +  // simplenews_node_presave will skip any nodes that are published in their
   +  // 1st revision, so we also check if sending should be triggered here, after
   +  // 1st revisions are persisted to storage.
   

   a function reference should have (). I think this comment can be simplified a bit, using revisions here seems confusing to me. What about:

   // simplenews_node_presave() cannot automatically send new nodes, send those here and resave.

   Hm, now that I think about it, I had an idea. To avoid the double save and also possible problems if saving to spool fails (an exception thrown there in presave could interrupt the node from being saved), what if we always send the node in insert() and just change the flag in presave?

   That of course means that we need to save in a different way that we want to send the node. We could use a flag on the node object although that's a bit discouraged (but still quite widely used). Or we could use a pattern with a function that has a static property (or method on a service) that remembers the nid and on insert()/update(), we just check if we had added that/flagged that node?

We should also expand our test coverage for this a bit.

Yes, you're right, that's risky too.

Fair enough, then lets change it so that we always do a double save, it shouldn't happen too often. Add a comment that explains why and we can get this in :)

This makes sense, but we really need a test for such an extra use case.

There are some changes to this problem?