Bugs if user has blank email address

Hi,

I've attached an updated version of the patch we originally provided for https://www.drupal.org/project/simplenews/issues/3028084, which was merged into this issue.
Maybe this will help you to get to a more comprehensive solution.

Regards
Marc-Oliver

I am working on this

Please find the uploaded interdiff and patch file.

For the meantime until we have a working more holistic approach to the issue, I've rerolled my patch against 8.x-2.x for those in need of a quick and dirty fix.

Like above, but this patch applies for 8.x-1.0 beta2

A re-rolled patch against the latest 3.x. Please review.

It needs one more check to pass tests

Proper fix

Hello Sir,
For the issue #3031919 I propose the following solution, I guess this might solve the problem

Changes to be made in the initial code in SubscriberForm.php

1)Added Email Field for Anonymous Users:For anonymous users, an email field is added to the form to capture their email address.
2)Handled Users Without Email Addresses:For logged-in users without an email address, a message is displayed indicating that their subscription will remain inactive until an email address is set.
3)Adjusted Subscription Logic:In the submitForm method, the subscription logic now checks if the email is provided. If not, it creates the subscription with an inactive status.

Changes to be made in the initial code in entity Subscriber.php

1)Email Field Requirements and Unique Constraint:
-Changes: The email field is defined as required (setRequired(TRUE)) and includes a uniqueness constraint (addConstraint('UniqueField', [])).
-Solution: Ensures that every subscriber entity must have a valid and unique email address. This prevents the creation of entities with empty or null email values, which could cause query conditions to fail when querying by email.

2)User and Subscription Synchronization:
-Changes: Implemented fillFromAccount() and copyToAccount() methods to synchronize data between the subscriber entity and user accounts.
-Solution: When a user subscribes with a blank email, these methods ensure that relevant user information (such as email address, preferred language, and status) is correctly synchronized between the user account and the subscriber entity. This synchronization prevents discrepancies that could lead to errors in subscription management.

3)Entity Load and Creation:
-Changes: Added loadByMail() and loadByUid() methods to facilitate loading of subscriber entities based on email or user ID.
-Solution: These methods ensure proper entity creation and retrieval, even in cases where users have blank email addresses. They provide flexibility to create new entities if none exist, ensuring robust entity management regardless of initial user data.

Changes to be made in the initial code in entity ConfirmationController.php

1)Subscriber Email Check: Added checks using $subscriber->getEmail() to ensure the subscriber has a valid email address before performing subscription actions (subscribe or unsubscribe).

2)Immediate Action Handling: Adjusted logic within the else block to include checks before calling $this->subscriptionManager->subscribe() or $this->subscriptionManager->unsubscribe() to ensure operations are only performed if the subscriber's email address exists.

Changes to be made in query condition.php

condition() Method Adjustment:
1)Within the condition() method, added a check specific to simplenews_subscriber.mail field where if the value is empty (NULL or ''), the condition is skipped to prevent the `
2)The check ensures that conditions involving simplenews_subscriber.mail are handled gracefully when the value is empty, aligning with the proposed resolution to allow subscriptions without an email but defer activation until an email is set.

@Aditiup Could you please provide a patch file?

Here’s an example of how to create one: git diff > filename.patch

@Aditiup Please do not mark your MR as a draft, as it will be automatically blocked.

@Aditiup I will try to test it ASAP and set RTBC.

I suppose patch from #23 and MR60 are currently not really a working solution for 4.x (i don't know if they maybe worked for 3.x). But 4.x is badly broken with this patch, basically whole system is dead after applying this patch. It seems this patch attempts to do a lot of refactoring beside fixing the issues with user without mail and maybe this has not kept up with latest changes in Simple news.

It seems to me, starting over here would be better. And for the refactoring part, imho one should focus on fixing the issues with user with no mail address here and do the refactoring in separate issue.

When reading the issue description, I am also asking myself, whether there are not some basic elements missing. It's about "when logged in as a user with blank email address". But on my end, I cannot create any user with blank email address in the first place, because this is already failing. So this would be something to solve before going to problems logging in with such a user.

So this surely "Needs work"

Proposing this patch as a starting point, allowing to add a user without mail address and fixing #2 from issue description (by not showing the newsletters page if user has no mail address).

My use case is to have users without mail address for other purpose than simple news, OPs case was to have users add the mail address later. So this patch is not enough for the latter case obviously.

this is my solution for this issue, I use social auth and Facebook, in some cases the email is missing, and the site crashes at the user login.

But on my end, I cannot create any user with blank email address in the first place, because this is already failing.

Yes, if you as an administrator create a new account, you get an error, and also if you save such a user. Drupal core does not require an email address for an account.

But not all users need to have a subscription. In my opinion the patch here should first and foremost focus on accepting new accounts without an email address.

This should have Priority "Major":

Trigger a PHP error through the user interface, but only under rare circumstances or affecting only a small percentage of all users, even if there is a workaround.

From https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

Adding related discussions.

We experienced an issue when updating a user and received the following error:
TypeError: Drupal\simplenews\Entity\Subscriber::loadByMail():
Argument #1 ($mail) must be of type string, null given, called in /web/modules/contrib/simplenews/simplenews.module on line 507 i Drupal\simplenews\Entity\Subscriber::loadByMail()
(rad 460 av /web/modules/contrib/simplenews/src/Entity/Subscriber.php).

We updated the patch and added change in simplenews_user_presave from #19.

Thanks @mohammed motar, perhaps you can include an interdiff between #19 patch and your patch?

Perfect, thanks!

I combined patches #30 and #41 in a new MR.
I used patch #41 but still got the error while creating a user. The check for the email in simplenews_user_insert() was missing.

I implemented the changes. I hope this is better now. Can you take a look again?

I just commited some changes. I think this will fix "4. "Newsletters" extra field on user entity => exception as below.". The error does not appear anymore if the user create form has the newsletter field.
How can I recreate "1. Page that has newsletter attached"?

Created patch from MR for 4.x-dev

I rebased everything to the latest 4.x commit. Can someone review it?

The comment #52 work it for me! Thanks!

I added a new test function with some test cases:

1. Create new user without email address.
2. Edit user without email address.
3. Access subscription form via account page as user without email.
4. Access subscription form via account page as user with email.
5. Create, edit and view newsletter issue.

Do we need more cases?
I put it in the SimplenewsSubscribeTest class, I hope that's okay. We can move it if there is a better place for it.