Remove broken revision handling

Could you explain this a bit? Does simple news need to support revisions (from core)? Isn't it possible to load always the latest node revision or something like that? I would have guessed that a node load returns always the current node. Or in other words, a module must be revision aware?

thanks in advance

Many modules have misbehaviour if revisions are used.

It is something that is highly broken for many contrib modules. Even core had some issues in some cases till recently...

Explaining more makes only sense if you're a dev an promise to fix it.
If you're only interested in this topic, you can check some documentation and read about how revisions work and think about the issues of possibly multiple records per node ... when joining a database table and e.g. omitting the condition to just consider the latest revision...

There are many many cases that can result when dealing with revisions, on every line of code.

If I understand correctly :

simplenews supports revisions = fields added by simplenews to nodes can also be put under revision.

I don't see a usecase for that, and I'd vote for removing revision support entirely *when sending a node*, but only if it still allows to use revisions on nodes fields(title, body,...etc) for other use.

Forgive me if I don't have a clue on this.

Since you e.g. can change the newsletter assigned, this field will always be revisionned (term reference field).
Data like sent state, subscriber count are not subject for versioning.
In addition the simplenews_newsletter also stores the tid, which is redundant to the term reference from the node and the first subject of inconsistency. If there once will exist a generalized "list id" that might be more than a term ID only, we'll need such a feature. However looking up the tid will then always need to check for the current term field used.

Additionally however, the issue table {simplenews_newsletter} also stores the vid which should always be the latest vid of the node (nid).
The specs state "The {node}.vid that identifies the version used as newsletter."
So simplenews might store the version used when sending the newsletter. However this adds a few questions then:
- How do people view the sent version?
- What happens if an admin edits the node during sending... (will the system still continue to send the old version?)

I think currently the vid is updated on every node save so it doesn't represent the version sent...

Finally i think for the (current) simplified version of Simplenews, we should completely remove tid and vid and access that information cleanly from the node table or the term reference field.
What do others think?

My uneducated 2 cents :

- use case for people changing the newsletter term while editing is hard to find. Always use the latest term used.

I would not store the version used when sending the newsleter because :

- sending always the latest version is a feature, not a bug : it means you can correct typos while sending the newsletter (on large newsleter, it can take hours for the sending to complete), the latest version will allways be sent.

- subscriber seeing the latest version is also a feature, for the same reason.

I see our usecase at work is to refine as much as we can the newsleter (it can take a day), then press the send button and cross our fingers we made no mistakes. Then we wait for the sending, and if we see a final typo, we correct it asap, and hope to see it corrected for the remaining subscribers.

This long text to say +1 on #5

Do you think this (#5 : remove revision code) would be hard to do ?

I understand Drupal quite a bit and made simple custom modules.

We are using simplenews again on production (17.000 subscribers and counting), so I can put some energy on this.

Assigning it to current bugfixing/feature development cycle.

The attached patch removes revision support from Simplenews. All tests pass but I haven't done much testing apart from that.

There is an update function which removes the vid columns from simplenews_newsletter and simplenews_mail_spool. I've talked with miro about deleting old revision data, but it looks like the table is currently not updated at all with never revisions/tid's, it just always stores the old data again. Which means that shouldn't be any existing sites which have multiple rows for a node.

Todo:

- Since we now officially removed support for revisions and stuff like continue sending nodes of the original revision when you edit it, I want to add a warning message to the node edit form when you're editing a node that is currently being sent.

Not necessary IMHO.

They can use as many revisions as they want to keep track of their changes. Simplenews doesn't care.

It will simply always use the active revision when sending messages. The only big difference is that when they create a new revision while the node is being sent, it will use that revision from that moment on. That's why I want to add the warning message I mentioned above.

Tested #9 on simplenews dev, so far so good, with or without revisions, newsletter get sent with subject in email and all content.

(Have some problem going from -alpha1 to -dev, but that I can live with)

We're moving toward stable with full speed!
Don't bother using dev. We're having growing test coverage and much improved quality.

Attaching a patch with the mentioned message.

#497108: Send newsletter when node is published contains tests for creating newsletters, once that is commited it will be easy to extend those tests and check for this message.

Re-rolled and added test assertion.

Commited!