Skip to content

Conversation

@kesselb
Copy link
Contributor

@kesselb kesselb commented Jul 6, 2025

Summary

Makes it possible to validate an email address, without injecting the mailer.

Todo

  • Step through testSetUserSettings to ensure everything's working as before

Checklist

@kesselb kesselb added this to the Nextcloud 32 milestone Jul 6, 2025
@kesselb kesselb requested review from ChristophWurst and susnux July 6, 2025 19:33
@kesselb kesselb self-assigned this Jul 6, 2025
@kesselb kesselb requested a review from a team as a code owner July 6, 2025 19:33
@kesselb kesselb requested review from icewind1991, leftybournes and provokateurin and removed request for a team July 6, 2025 19:33
@kesselb kesselb added 3. to review Waiting for reviews feature: emails labels Jul 6, 2025
/**
* @param string $email Email address to be validated
* @return bool True if the mail address is valid, false otherwise
* @deprecated 32.0.0 use IEmailValidator.isValid instead
Copy link
Contributor

Choose a reason for hiding this comment

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

This was already deprecated since 26 we undeprecated it for 32 but if you add a replacement, please readd the deprecation since 26.

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Code looks good, my comment about the deprecation is still valid

@kesselb
Copy link
Contributor Author

kesselb commented Jul 7, 2025

Code looks good, my comment about the deprecation is still valid

I only change the interface 🙈

@kesselb
Copy link
Contributor Author

kesselb commented Jul 7, 2025

What do you think about the trait to obtain an instance for usage in tests?

@kesselb
Copy link
Contributor Author

kesselb commented Jul 20, 2025

The tests are now using the preconfigured instance from the trait. I think it’s a plus that the validation is now done with the actual implementation and not a test double. The downside is that we’re losing the check that the validator was called.

@kesselb kesselb force-pushed the feat/imailaddressvalidator branch from e92dd99 to 6be9132 Compare July 20, 2025 18:05
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 1, 2025
@kesselb kesselb force-pushed the feat/imailaddressvalidator branch 2 times, most recently from b6d28ce to 93db1d6 Compare August 14, 2025 16:30
@kesselb kesselb force-pushed the feat/imailaddressvalidator branch from 93db1d6 to 336c6d2 Compare August 14, 2025 16:34
@kesselb
Copy link
Contributor Author

kesselb commented Aug 14, 2025

Moved the refactoring commits, to use the validator, to another branch because it's external API freeze today, and checking the test needs more time.

@susnux
Copy link
Contributor

susnux commented Aug 14, 2025

Moved the refactoring commits, to use the validator, to another branch because it's external API freeze today, and checking the test needs more time.

But that does not change anything here, no? We do not remove the old way and just refactor internal usage.

@kesselb
Copy link
Contributor Author

kesselb commented Aug 14, 2025

But that does not change anything here, no? We do not remove the old way and just refactor internal usage.

Fair enough, technically we do not break anything and just add a new API. I'm running a bit out of time this week and thus wanted to make sure that the new API lands in 32. There was a conflicting merge meanwhile, and also one of the tests needs a closer look (if we are still testing the right thing). I hope to finish it next week and have everything migrated for 32.

Okay for you to merge this one without the refactorings, or do you want to keep it together?

@kesselb kesselb merged commit 09607f4 into master Aug 19, 2025
212 of 217 checks passed
@kesselb kesselb deleted the feat/imailaddressvalidator branch August 19, 2025 07:38
@susnux
Copy link
Contributor

susnux commented Aug 19, 2025

Its fine.
Maybe you have time to do so after branch off?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants