Setting to enable token replacement in newsletter issue fields

You are right: if there are no tokens then no metadata is added. However, the node edit page has a token replacement box which encourages a newsletter editor to enter tokens. If that might be a bad idea then we can add an admin setting allowing the site owner to disable it.

it wouldn't add the current user context through that anyway, so in fact it looks like we might be missing cacheability metadata?

The token data is missing user, but contains subscriber, which seems reasonable. When viewing the page that uses tokens for a second authenticated user then the subscriber is different so presumably it triggers a cache miss in the page cache. It could however be a cache hit in the render cache for the node (unless the node itself varies per user).

if you send newsletters to authenticated users then you could implement your own cache implementation that caches for authenticated users too.

Presumably 1000s of sites send newsletters to authenticated users - we can hardly claim that this is an unusual choice requiring custom code. If performance is unacceptable sending to authenticated users then I would count it as a bug. However I would expect render caching of the node is solving the problem - or is that disabled from cron also?

And note that this function isn't being used for sending newsletters, it excludes email view modes.

Yes but there is exactly the same code for sending newsletters in MailEntity::getBodyWithFormat(). The setting would control both places.

The same setting could also disable the user switching. The use case for this seems fairly rare (presumably to display some extra 'restricted access' information in the newsletter to authenticated subscribers, maybe with field_permission module or similar?) and it seems to be contributing to the bugs and performance degradation.

If we make the change in a new major release then we can even default the setting to off. So I'm tempted to add this in soon before a 3.x beta.

@Berdir - any more thoughts/advice very welcome.

Ah of course we can't disable the token replace in getBodyWithFormat() because it replaces the footer tokens such as unsubscribe. Potentially user switching is best kept separate too. So that would leave something like this.

Global newsletter setting: "Show token browser (discouraged)", defaults to FALSE. Description = "Show token browser on the node edit page. Note that in many cases, visitors will see un-replaced token codes when browsing content". Effect of setting when false: remove the token browser, and disable the code in simplenews_node_view().

Some example cases where it doesn't work are:

• Anonymous users
• Unsubscribed users
• Viewing nodes in any way other then their own page: teasers, views, etc.
• Fields that don't render to produce '#markup'
• The [newsletter:*] tokens are not replaced (could easily be fixed)

In fact I'm not sure if I can find a case where it works!!

The documentation for hook_ENTITY_TYPE_view() says that it is called before rendering, so maybe that is why it cannot find a #markup element?

OK, I fixed simplenews_node_view(), now working except obviously not for the case where there is no subscriber.

Thanks Berdir. I agree with you on almost all. I chose the setting rather than permission so it can work for admins.

I debated whether to remove the placeholders, but I saw a potential problem - if a node with newsletter function contains any text that resembles a token then it would get removed. Not especially likely but it could happen - perhaps on a Drupal related site. I could be persuaded to change on this one.

Also, the simplenews subscriber tokens for example specifically support the view-online link that provides the subscriber info in the query.-

I'm not familiar with that token - does it require an extra module? It sounds like something you would put it in the footer part of the template rather than in the node. NB This setting only disables tokens in the newsletter entity - not those in the template.

For sending newsletters, tokens are specifically designed to improve performance, as token replacement happens on the rendered newsletter. That's it's broken for most formatters in D8 is not that surprising, we never really used that part I think.

The tokens work very well for sending newsletters in terms of performance, and being bug-free. The problems I found/fixed were when viewing the node as a webpage. Previously the code was apparently totally broken and it's hard to see how it could have worked in D8.

So that leaves the question of what should be the default setting? The reason I left it disabled is because it seems to break many very common sites such as blogs - anyone reading down the blog would see token codes. If we enable it by default, do you have any ideas what to do about that? Maybe we could pass in a dummy subscriber object??? It could easily crash though😃.

OK I have change the default to TRUE, with some fallback of a dummy subscriber object.

Thanks @Berdir. I feel it's good to make the option so it disables the token replacing also: firstly in case it breaks something people have a solution; secondly, it enforces a site policy in case the editor gets smart and types in a token code even without the browser. So I altered the description as you suggested. The default is on now (both for new sites and upgrades), which feels OK to me now that the code leaves raw token codes behind much less often.

And maybe the token explanation on the node form itself could be extend with the node that depending on the used tokens, it will not work then?

Beg pardon I don't understand that one.

Just a random fail

I raised #3248026: Problems with inserting tokens in newsletter fields to describe the remaining problems with this feature. I updated the setting description.

Fix the config setting name and description to match current behaviour