Bugs if user has blank email address

AdamPS → credited Berdir → .

Copied from 🐛 Subscriber::loadByMail(): Argument #1 ($mail) must be of type string, null given Needs review by Berdir. Still needs tests, and probably doesn't cover all of the cases in the IS yet.

It needs one more check to pass tests

Proper fix

Thanks. Back to needs work because several of the cases in the IS are still failing AFAICS. Also would be great to have tests.

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 → closed merge request !61

@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 ready 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.

AdamPS → changed the visibility of the branch 3031919-bugs-if-user to hidden.

Thanks I agree and I hid the MR/patch with lots of refactoring that is not part of this issue.

I believe the patch from #19 is still good. It contains some extra changes to what you have in #30. It would be great to merge these together and convert to a MR (Drupal.org is stopping using patches now).

This IS describes 4 cases so it would be good to track which ones we have done.

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.

But not all users need to have a subscription.

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.

Thanks I made some comments in the MR. I agree, we also need some code in simplenews_user_insert().

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

Thanks it's good. Setting back to "needs work" because still some cases aren't covered.

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"?

Great thanks.

How can I recreate "1. Page that has newsletter attached"?

Just view the node.

I believe you will not see any bug. Here is the patch posted on the other issue. https://www.drupal.org/files/issues/2892047-query-condition-8.patch → . See there was a call to simplenews_subscriber_load_by_mail(), which was causing the bug. That call no longer exists, the code now has a call to Subscriber::loadByUid().