- ๐จ๐ญSwitzerland berdir Switzerland
AdamPS โ credited Berdir โ .
- ๐ฌ๐งUnited Kingdom adamps
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.
- last update
about 1 year ago 2 pass, 26 fail - last update
about 1 year ago 56 pass, 1 fail - Status changed to Needs review
about 1 year ago 4:58pm 16 January 2024 - last update
about 1 year ago 61 pass - Status changed to Needs work
about 1 year ago 9:49am 17 January 2024 - ๐ฌ๐งUnited Kingdom adamps
Thanks. Back to needs work because several of the cases in the IS are still failing AFAICS. Also would be great to have tests.
- ๐ฎ๐ณIndia Aditiup
Hello Sir,
For the issue #3031919 I propose the following solution, I guess this might solve the problemChanges 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. - ๐น๐ณTunisia azizos
@Aditiup Could you please provide a patch file?
Hereโs an example of how to create one:
git diff > filename.patch
- Merge request !60Fix: Handle subscriptions for users with blank email addresses โ (Open) created by Aditiup
- Merge request !61Fix: Handle subscriptions for users with blank email addresses โ (Closed) created by Aditiup
- ๐น๐ณTunisia azizos
@Aditiup Please do not mark your MR as a draft, as it will be automatically blocked.
- Status changed to Needs review
8 months ago 2:37pm 3 July 2024 - Status changed to Needs work
7 months ago 12:43pm 6 August 2024 - ๐ฉ๐ชGermany stefan.korn Jossgrund
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"
- ๐ฉ๐ชGermany stefan.korn Jossgrund
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.
- ๐ฌ๐งUnited Kingdom adamps
AdamPS โ changed the visibility of the branch 3031919-bugs-if-user to hidden.
- ๐ฌ๐งUnited Kingdom adamps
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.
- ๐ฒ๐ฉMoldova nick.murza Moldova
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.
- ๐ฉ๐ฐDenmark ressa Copenhagen
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-... โ
- ๐ธ๐ชSweden mohammed motar
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.
- ๐ฉ๐ฐDenmark ressa Copenhagen
Thanks @mohammed motar, perhaps you can include an interdiff between #19 patch and your patch?
- First commit to issue fork.
- ๐ฉ๐ชGermany lmoeni
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. - ๐ฌ๐งUnited Kingdom adamps
Thanks I made some comments in the MR. I agree, we also need some code in simplenews_user_insert().
- ๐ฉ๐ชGermany lmoeni
I implemented the changes. I hope this is better now. Can you take a look again?
- ๐ฌ๐งUnited Kingdom adamps
Thanks it's good. Setting back to "needs work" because still some cases aren't covered.
- ๐ฉ๐ชGermany lmoeni
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"? - ๐ฌ๐งUnited Kingdom adamps
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 toSubscriber::loadByUid()
.