Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: fix issue where default admin without valid email can not be saved #10774

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

sunnysideup
Copy link
Contributor

@sunnysideup sunnysideup commented May 11, 2023

It is possible to have a default admin without a valid email. In this case, you can not save Member as it throws an error (email is tried being sent without a valid email address).

Issue

Comment on lines 777 to 778
&& $this->Email
&& filter_var($this->Email, FILTER_VALIDATE_EMAIL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can just be this, filter_var will correctly handle empty strings and nulls etc.

Suggested change
&& $this->Email
&& filter_var($this->Email, FILTER_VALIDATE_EMAIL)
&& filter_var($this->Email, FILTER_VALIDATE_EMAIL)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use Email::is_valid_address($this->Email ?? '') instead

@GuySartorelli GuySartorelli changed the base branch from 5 to 5.0 May 12, 2023 08:07
@GuySartorelli
Copy link
Member

I've retargetted this at 5.0 since it's a bug fix - can you please reset the PR to remove the extra commits?

It is possible to have a member record without a valid email address. In this case, you can not save Member as it throws an error (email is tried being sent without a valid email address).
@GuySartorelli
Copy link
Member

I've reset the PR to not include the extra commits and have made the recommended change.

Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MOG

@GuySartorelli GuySartorelli merged commit 848768d into silverstripe:5.0 Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants