Count error if translation is missing

Please can you describe exactly the sequence of steps to reproduce, what you actually see and what you expect to see.

Best of all would be if you upload a patch that includes a test that fails.

Thanks for the clarification.

1) The problem with counting is definitely a bug. It seems like the same as #3109499: Error when news item doesn't have translate which I have just closed as a duplicate of this. Please can you update the issue summary with some of the info from comment #3 and also the error message from that issue. Then at least we will have a clear description of what the bug is which is a good start.

2) You are asking what should happen if a there isn't a translation available for a subscriber. You suggest that the subscriber should be skipped. Actually the subscriber gets a newsletter - possibly in the default language? I don't really agree that this is a bug; it's just the way the module works. If we change that, then it could create a problem for other sites that are happy with the way it works currently.

If you would like to skip these subscribers you could look into custom code that throws SkipMailException.

Thanks. Yes I understand why you are asking for point 2 - all I'm saying is that we can't necessarily change it now without breaking other sites.

Thanks for the patch. I'm really busy right now but I will have a look as soon as I can.

Test fail was a random - setting back to NR.

I'm hesitating because the way the code works seems really weird. I don't use multiple languages on any sites so it's harder for me to be sure.

1. When the admin sends a newsletter they might not even be aware that it is multi-language. They would expect to press "Send Now" once and it would send all translations. Is that how it works?
2. If the admin checks back to see the status (on the same page where they pressed "Send Now") they would expect to see a count that included all translations. Correct?
3. If so then I don't understand why we are counting separately for each translation.

Same shoes here.

Scenario: a newsletter is XX-xx language is going to be sent. Not every subscriber language is XX-xx

Since this scenario is breaking the code, we definitely have a bug to be fixed soon. Subscribers are waiting for news :)

*
**SELECT**
***What if subscriber language doesn't match items language?***
**** OPT 1 -> don't send if language doesn't match ****
**** OPT 2 -> send to everybody, adding a "This newsletter is written in the XX-xx language" ****
*

I would propose to add this options somewhere inside "/admin/config/services/simplenews/settings/newsletter"

I am the only active maintainer of this module. I am an English native speaker and I have never worked on a multi-language Drupal website. Before I can check in this fix I will need some more help from the community. Please see the questions in #12. They can be answered by anyone - you don't need to be a developer. Except #3 which isn't really a question anyway.

I would propose to add this options somewhere inside "/admin/config/services/simplenews/settings/newsletter"

This would need to be a separate issue - that's a feature not a bug. See #4. It's fine to propose an idea. But nothing will change unless someone writes a patch.

Sounds good, Adam.
Looks like that piece of code is somehow unfinished.

I will post a new feature request as issue, which eventually will fix this bug also.

So, from this issue we see that on a multilanguage installation, simplenews breaks when languages change.

From experience, we know user subscribes from the newsletter title. If I subscribe to the newsletter named "Newsletter sulla cucina inglese" I'm no way expecting to receive news in English and - as publisher - I will write in italian language. Can't imagine nor can remember newsletter written in different languages.

If design is accepted, happy to apply this via patch

pbonnefoi,

Let say I have 4 subscribers
1 in undefined
1 in english
2 in french

is that scenario you describe above real? I mean, do you really have a newsletter with subscribers expecting news in different languages?

From my experience, audience subscribes the newsletter by the "title" field, expressed in a clear language. At that point subscribers expect news in "that" language. As subscriber at "The best English cooking Newsletter ", I expect to receive recipes in English language, even if my preferred language is not English.

I can't see value in supporting newsletter with multilanguage content. And this is also breaking the module.

If the use case you define is real (I believe not very popular though), maybe we should move that in a sub-module.

Find a bit more at https://www.drupal.org/project/simplenews/issues/3153612

@afagioli Please can we try to be more flexible? I don't believe it is right for one person to insist that a certain feature is unpopular and should be removed just because that feature has a bug. If we continue like that then the module will have no features left. Some people are following this issue which suggests they care about the multi-lingual.

Why can't a newsletter have translated titles in multiple languages??

In this very issue, I'm asking pbonnefoi if that real case actually exists.

I have plenty of flexibility, @Adam.
At today, we have very rarely used features (if ever) breaking a module. We have the sub-module approach as friends (already mentioned before).

Let's keep the wider conversation at https://www.drupal.org/project/simplenews/issues/3153612

Postponed (maintainer needs more info)

Please Adam,
remove the postponed status.

We need to fix this soon,
How can I help?

Thank you @pbonnefoi for a very clear problem description. This is something we should support and I agree there is a bug.

@afagioli Thank you for your offer to help. I have updated the issue summary to describe the remaining tasks.

Thanks for the recent progress, Adam.

I usually manage multilanguage Drupal websites on regular basis. From my experience, listening users, working for authors, I've developed an assumption where multilanguage content support is not a requirement. For this, pbonnefoi experience would be very interesting to ear.

I re-designed simplenews without MCS here
https://github.com/augustofagioli/d8_simplenews_nomcs where the same spooler bug still lives. So, thank you for your code above. Super-welcome.

Aside that bug, that new design is much cleaner and managing multilanguage newsletter seems to be very easy and effective for subscribers and editors

I'm open to discussion and happy to submit that patch here when usable.

Please find a patch with:

* bugfix
* improved design for sending newsletter in multiple languages
* improved UI for editors

Please find a new patch with interdiff.
Thanks for your patience

Test is failing on features apparently untouched and actually working. At this point, not sure what to do.
Help appreciated.
Thanks

Your patch seems to consist mostly of unrelated changes. Based on the changes in the .info.yml files, it looks like you downloaded a release and then made that into a new repository, now your patch is reverting a lot of changes that happened since tehn.

Don't do that. Use git clone to get the repository and then make changes in branches. It's going to be very tedious to untangle that and remove all the changes that you didn't actually do.

Cool. Thanks. Will do

Please find a patch as per #32 with:

* bugfix;
* improved design for sending newsletter in multiple languages;
* improved UI for editors;

Known BUG: "UI related: Edit Newsletter node > Exposing wrong Newsletter language on select newsletter change.

Suggestions at #32 - applied - did not help.

More help is welcome. Thanks

As reference, please find updated code at https://github.com/augustofagioli/d8_simplenews_nomcs

Trying to clarify what @Berdir understands at https://www.drupal.org/project/simplenews/issues/3106374#comment-13735921
Your patch seems to consist mostly of unrelated changes.

The patch above proposes an improved approach for multilanguage newsletters. Shortly:

. and a new "language" field for newsletter entities is added.
. users subscribe by newsletter's language
. newsletter content types are no more translatable (who needs that?)
. improved UI for editors

more at:
. https://www.drupal.org/project/simplenews/issues/3106374#comment-13710161
. https://www.drupal.org/project/simplenews/issues/3153612

We see at https://www.drupal.org/project/simplenews/issues/3106374#comment-13745208 that automated test failed on buggy text code with a @TODO in it (see SimplenewsSendTest.php)

  // @todo Temporarily strip tags from mail subjects until
  //   https://www.drupal.org/node/2575791 is fixed.

Who will take care of that?

Please find a patch as per #40 with:

* bugfix;
* improved design for sending newsletter in multiple languages;
* improved UI for editors;
* test code check

Known BUG: "UI related: Edit Newsletter node > Exposing wrong Newsletter language on select newsletter change.

Please find a second patch as per #40 with:

* bugfix;
* improved design for sending newsletter in multiple languages;
* improved UI for editors;
* test code check

Known BUG: "UI related: Edit Newsletter node > Exposing wrong Newsletter language on select newsletter change.

Please find yet another patch as per #40 with:

* bugfix;
* improved design for sending newsletter in multiple languages;
* improved UI for editors;
* test code check

Known BUG: "UI related: Edit Newsletter node > Exposing wrong Newsletter language on select newsletter change.

Please find yet another patch as per #40 with:

* bugfix;
* improved design for sending newsletter in multiple languages;
* improved UI for editors;
* test code check

Known BUG: "UI related: Edit Newsletter node > Exposing wrong Newsletter language on select newsletter change.

Automated test finally passed.

Please ask if you need more

Hell Alex and Berdir Mantainers,
the requested patch is there. Test passed.
Do you need more from me to make some further steps forward?
Thanks

ping!

Hi @afagioli, it's great that you are enthusiastic to work on this. It seems that you propose to make each newsletter a single language only. That's not what this issue is about. Please create a new issue if you want to do that.

However in this comment, @Berdir already stated:

No, the current multilingual support will not be removed...

so I don't think it's likely to happen.

Halo @AdamPS,

Simplenews has many issues to be fixed. Some of them are crossing one eachother very frequently.

That's why you opened a specific META issue See your own https://www.drupal.org/project/simplenews/issues/3153612#comment-13716433

As things are, the considerable efforts you 2 active maintainers are making here seems to be not enough. Simplenews is a crucial module for Drupal which deserves more energies.

Please consider opening crucial decisions to more people. Widely shared choices are always the best.

ping

This issue needs work. The proposed work is clearly described in the issue summary. You are welcome to upload a patch.

Decisions are absolutely open for discussion. However please keep the discussion relevant to the issue. This issue is a simple bug, and it is not the place to discuss major changes - not even if your major changes could solve the bug. You should raise a separate issue specific to your change.

Thanks for your reply and for your declaration to be so open, AdamPS.

A sign of life from the pbonnefoi who arose this issue would have be very useful. not received yet. Shame.

Matter of fact, in this issue you find clear direction from me as multilanguage newsletter user from years. And a patch also.

In the most concise way, in the newsletter's real world there is no fixed connection between user language and newsletter language.
From experience, we know user most of times subscribes from the newsletter title. If I subscribe to the newsletter named "Newsletter sulla cucina inglese" I'm no way expecting to receive news in English and - as publisher - I will write posts in italian language. Can't imagine nor can remember newsletter written in different languages. We have so many prove around of this [ https://www.drupal.org/project/simplenews/issues/3153612#comment-13801877 ]

Sad to say, this issue would fix a bad designed (far too old?) feature.

This is my simplenews fork where multilanguage pain is removed.
If don't need a strict relation between user language and newsletter language, you are ready to go with this
https://github.com/augustofagioli/d8_simplenews_better_ui

If you do instead, please share with us your user case. Very interested in knowing more.

This issue also causes #3178885: Missing langcode causes send to fail without actionable information

I ran into this to with 2.x branch. The logger exception with type "cron" as googlefood:
InvalidArgumentException: Invalid translation language (en) specified. in Drupal\Core\Entity\ContentEntityBase->getTranslation() (Zeile 873 in /var/www/html/web/core/lib/Drupal/Core/Entity/ContentEntityBase.php).

Thanks @AdamPS and @Berdir for keeping the issue on track. The code review is concise and makes sense.

OK i worked on this, created an issue fork and pushed to it. Setting NR.

Created an MR, not testbot is running.

In fact i need the 2.x backport.

OK i boldly made a backport MR !3: https://git.drupalcode.org/project/simplenews/-/merge_requests/3

(i only had to change the hook_update_n)

...but seems like d.o does not support a backport displaying in the issue and triggering testbot yet.

OK 3.x patch !1 looks goodd, but testbot seems still stuck on #2990645: "Build Successful" is treated as a test failure which i don't grok. Or is it #3099061: Can't run PHPUnit tests since Drupal 8.8.0 on PHP 7.3+?

Thanks for popping in here, @geek-merlin
So nice to see more activity on simplenews. On the edge of the contest, I'd really like to know more about how to handle multilanguage subscription. As you read above, to me, this feature deserves some improvment.
Cheers

@AdamPS

OK i fully agree with your directions and coded it to the issue branch / MR.

I do NOT understand why i have to trigger a custom php7.2 test to get results (the branch tests are on 7.3 and green).

Also i do NOT understand why this change makes which tests fail.

So passing to someone else to work on this.

Thanks @geek-merlin. I'm not yet familiar with the new merge request system. I see you have made several commits, but the later ones appear to revert some of the earlier ones. Did you end up with anything that will be useful for someone else to continue from? Do you have any questions I can help with?

@AdamPS
> I'm not yet familiar with the new merge request system.

It's a real blessing, but as you see i bumped on edges when i added a backport branch.

> I see you have made several commits, but the later ones appear to revert some of the earlier ones.

Yah, that's part of the branch concept, you can only add commits so history is retained. (Later in the merge there is an option to squash though.)

> Did you end up with anything that will be useful for someone else to continue from? Do you have any questions I can help with?

Well, i coded down your suggested approach from the IS, and added a migrate hook that makes the field untranslatable (Re-thinking it, i did not test if that changes the test outcomes), so that this convoluted issue has a good direciton again, and in the hope it's easy win. But it isn't, some tests are red.

Next step is to debug what happens here and why tests go red. (I currently have no time to do that though.)

So thanks for asking, if you look at the tests and have an idea, great, if not, the quest is up to someone doing the debugging.

I am willing to look but I don't understand the new system well enough to decode your work. If you would like my view please can you either upload a patch the old-fashioned way or start over again with a new merge request that only has the real changes, not unrelated coding standards, reverting or backports.

@AdamPS: Great! If you click on MR1 and then on the "changes" tab, you see the complete resulting diff. Does this help?

Great! If you click on MR1 and then on the "changes" tab, you see the complete resulting diff. Does this help?

I got it thanks. Potentially I could leave comments directly on your branch but I don't know how so I will write them here the old-fashioned way:

You need a few more changes in sendSpool(), something like this:

1. In the section starting with comment "Calculate counts", remove the code [$row->langcode].
2. In the section starting with comment "Update subscriber count", the loop should be foreach ($ids as $entity_id => $counts)

@AdamPS #84: OMG how could i miss this. Thanks for the review.
Implemented everything and am confident that this is the way to go.
Unfortunately tests still give neither red nor green, but i'v got no bandwidth to look into this.
Also leaving NW so to decide if / what automated tests we need to add.

When there is a multi-language newsletter, the number of emails sent is showing wrong:

• The number of subscribed users shows the same number for each language when the user subscribes in a specific language.
• In the number of submissions, the number of submissions of the original language is shown for all languages.

This patch is a workaround, but I think the entire multi-language part of the module should be revised.

This patch is a workaround, but I think the entire multi-language part of the module should be revised.

Good to ear that. I've been shouting this out lot for a while now :)

Since Simplenews is such a crucial module for Drupal today. I believe once more of us will reach this point, a more open conversation about how to improve simplenews will start.

Thanks a lot

I'm uploading a new patch to fix a cache count error.

@ cbccharlie There is already an issue fork that I believe is the right solution - it simplifies the code, reducing the likelihood of multi-language bugs. The status is "needs work" because of the tests: some are failing and there should have a new test that covers this bug. For anyone who would like to help then fixing the tests is the next step.

There are several other issues for conversations about wider changes to simplenews and anyone is welcome to join the discussion there. This issue is for discussing this specific bug and the comments here should be specific to that.

There is already an issue fork that I believe is the right solution

That's sounds so good, AdamPS.
Can you link us the that fork, please?
Thanks

#90: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

hth!

Thanks for confimirng, #91

At #60 we have:

This is my simplenews fork where multilanguage pain is removed.
If don't need a strict relation between user language and newsletter language, you are ready to go with this
https://github.com/augustofagioli/d8_simplenews_better_ui

If you do instead, please share with us your user case. Very interested in knowing more.

Waiting to know whether mantaioners are on the same page

There are several other issues for conversations about wider changes to simplenews and anyone is welcome to join the discussion there. This issue is for discussing this specific bug and the comments here should be specific to that.

This was in #89. Maybe it wasn't unclear? This issue is for a specific bug. Please use a different issue for discussing major redesigns.

Thanks for clarifing, AdamPS.
Could you link us to the fork you mentioned at #89 ?
Thanks

@afagioli, I think @adamps refers to the following merge requests:

• 8.2: https://git.drupalcode.org/project/simplenews/-/merge_requests/3
• 3.0: https://git.drupalcode.org/project/simplenews/-/merge_requests/1

Thanks @cbccharlie, that looks like a good start for me, really.
I'd been on this for a while, now. A few other good starts similar to this got lost in confusion.

Before moving forward, I'd prefer to wait for the mantainer preference, which is moving the focus somewhere else.

Please use a different issue for discussing major redesigns.

@AdamPS - as maintainer - could you please clearly share with us what you expect from contributors.

I read also on project home page

The Drupal 8 version is maintained by AdamPS.

previous mantainers called themselves out of this. Does it means we can safely ignore their "No way" to the "we-dont-translate-newsletter" approach is we are proposing here?

I apologise for typing all these words. At this stage, not easy to do without.

Thanks

This link comes handy to have the wider scenario of the "we-dont-translate-newsletters" thing:

Simplenews issues about multilanguage

@AdamPS - as maintainer - could you please clearly share with us what you expect from contributors.

Sure. Basically there are lots of issues about specific bugs. The comments there should be only about those bugs. Please don't put comments there saying "we should make a major change to how multi-language works" - that just creates a distraction for all the people trying to solve those bugs.

Instead you should raise a new issue for your proposal to make a major change. All the people who are interested then can join you.

previous mantainers called themselves out of this. Does it means we can safely ignore their "No way" to the "we-dont-translate-newsletter" approach is we are proposing here?

I'm sorry but I'm not going to answer that here:-) As above, this issue is for discussing this specific bug.

Instead you should raise a new issue for your proposal to make a major change. All the people who are interested then can join you.

https://www.drupal.org/project/simplenews/issues/3181267

I'm sorry but I'm not going to answer that here:-)

No answer expected from you, Adam :)
Thanks

@AdamPS
> I'm not yet familiar with the new merge request system.

It's a real blessing, but as you see i bumped on edges when i added a backport branch.

Well to me it is a real curse. I have just spent an hour and accidentally pushed the entire messy MR to 3.x rather than to the merge branch and this is impossible to undo. The merge branch is now in a mess because it is behind 3.x so I closed it. If anyone is willing to upload a patch in the old fashioned way then I will look at it.

Sorry @geek-merlin I'll stick with the ways I know for now - here's a patch that puts the changes in the MR back. Getting the results from the testbot now.

Fixed the tests, now ready to go. I will commit in 1 week. Any review or comments welcome before then.

Rebase