Change 'on update' setting to mean update of email address, not entire user.

Thanks @darvanen that's true. On the other hand I still wonder if it wouldn't make even more sense to only validate on registration and on email change?

I think that is what is typically expected, as all updates on the user profile that are not email related should be out of scope of this module.

So perhaps the next refactoring should be a breaking change (2.x?) for better UX and should simply rename the "Update" option to "Email address changed"? or something like that.

Just my 2 cents.

Anyway it's most important to have that major blocker removed now. :)

I'm very interested in your 2c, so thanks for chiming in. I see in my rush among other work I missed the fact that you had named the other issue to do exactly what this issue says rather than what I ended up creating.

I've got a working prototype in the MR for this issue which has to load the existing user from the database to check against the existing value. Now I'm thinking I probably need to have a generic constraint for people to use on any field that they can extend with their own configuration and make the user account constraint a subclass of that, or some other way to make it as extensible as possible if we're going to release a 2.x

I also need to think about (and am very open to suggestions for) any other changes that might be valuable for a 2.x version.

Maybe two separate single-purpose constraints would be better.

Thank you @darvanen this clearly goes straight into the right direction!

One thing I noticed:

$this->valueChanged($entity)

can be checked much earlier, I think. If the email address did not change, we have nothing to verify and skip early.

That's also the most expensive check as it requires loading the entity. I'm going to split the constraint into two, and move the configuration responsibility to the entity alter hook rather than within the constraints so they're more generically usable. Kicking off 2.x to support this effort.

darvanen → credited chx → .

Adding credit for help in slack.

Ready for review if anyone's keen, please and thank you.

Alright, I made a few adjustments based on my review. Otherwise, this LGTM! Ready to merge if you agree with the changes!

Thanks very much @Grevil, I was convinced DI doesn't work in form classes, that must be something else, like inside an Entity. I agree with all the changes, I went through so many iterations getting it to work that I missed all of those opportunities, thank you.

Looks like it's passing testing. Do you think we need to adjust the tests that were created in 🐛 Add option to only validate new registrations and address changes, exclude existing addresses (do not block user profile save) Fixed now the scope of the settings is a bit different?

I've had a bit more of a think and realised that you reduced the scope of the 'changed' validator so it will only work on user entities, and only on the primary email field. This module is intended to be as much developer tool as an easy-to-use validator for user accounts. I've changed that particular alteration back.

I'm still pondering the tests.

I've had a bit more of a think and realised that you reduced the scope of the 'changed' validator so it will only work on user entities, and only on the primary email field.

Yes, indeed! Sorry, I wanted to comment on that, but apparently I forgot to do so.

For me personally, lowering the scope here would've been fine, since this module's main focus is to add further email validation to user email fields. I didn't think about "getEmail()" only getting the value of the "main" email field. I think this would be still fine for most cases, but yea there might be instances where there is a different mail field, which should be validated.

From the module page:

automatically applies the configured rule-set to user email addresses

So, I guess, we could lower the scope to user entities? But if you want the current scope as is, that would be fine (it is your module in the end 😁).

The tests are fine, but we could add a new test changing another field apart from the mail address, checking that the constraint doesn't fire.

All in all, I'd say we can merge this, if we do not want to lower the scope!

Just because the constraint is used by the module for "automatically applies the configured rule-set to user email addresses" doesnt mean people won't want to use it for other purposes. I'm trying to makes something extensible, there's already a webform validation handler for instance.

Yeah we could go nuts with the tests but I think you're right, as-is is fine for now.

Really appreciate your feedback, and some of your improvements stayed so it was worth writing them so I could have a different perspective on the file. Thanks again :)
Agree this is ready to merge.

Done.

I want to get these two done before cutting a 2.x release:

• 📌 PHPStan - enable and fix Active
• 📌 Change rules config from strings to booleans Active

You're welcome! Glad I could help again!

Automatically closed - issue fixed for 2 weeks with no activity.