- Issue created by @AstonVictor
- πΊπ¦Ukraine AstonVictor
Added a simple patch for now.
I guess it should be updated to use DI. - πΊπΈUnited States cmlara
It would be a good option to alter user data before saving
can you provide a real life scenario of this being useful?
Everything in the user data that I can think of (TFA metadata, seeds) is essentially self owned/internal (to a plug-in) data that shouldnβt be touched by external actors.
- πΊπ¦Ukraine AstonVictor
basically, I need an option/event to check when the tfa status (1/0) has been changed.
- πΊπΈUnited States cmlara
I suspect β¨ User account should be marked as updated when TFA is updated Active may end up being a dedicated TFA event (as Iβm not aware of a specific user save event we could leverage) however nothing about that has been formally said in that issue.
If you want to rescope this issue: I can see TFA emitting an Event when we enable/disable TFA for a user or a specific plugins is provisioned for a user as making a significant amount of sense without using the hook system (especially not using alter hooks).
- π¬π§United Kingdom scott_euser
@cmlara I think that makes sense thanks! And @astonvictor I believe that should work for us too right? Ie, we do not need to change the data we just need to know it has changed. I updated the issue summary + suggested we take the same data + 'original' that entity update hooks do for convenience as I can imagine responding to changes to plugins but ignoring changes to skipped validation count.
- π¬π§United Kingdom scott_euser
scott_euser β changed the visibility of the branch 3532729-provide-an-alter to hidden.
- πΊπΈUnited States cmlara
Do we need the user data to be passed forward in the event?
That data really is internal, it has no formal API between plugins or even necessarily between patch releases.
In TFA we (generally) know when we change the user in TfaSetupForm.
I was thinking we could be emit either a "TfaStateChange"(enabled/disabled) (or separate events for either case such as TFAEnabled/TfaDisabled) and equivalent for Plugins (TfaPluginStateChange or individual TfaPluginEnabled/TfaPluginDisabled) events (descriptive examples not intended to indicate what events must be called). May be possible to combine the general and plugin events into one, care should be taken on how well they can be used by consumers such as ECA/Rules.
Only reason I can think of to do this in TfaUserDataTrait is if we are worried about a module other than TFA making changes to the data, however in that case it really needs to be a decorator on the core user.data service and not in the TfaUserDataTrait and it could be said to not be indicative of TFA making the change.
- π¬π§United Kingdom scott_euser
Ah I see makes sense; is the general setup here for events + event names okay before we rework? I used the setup from https://www.drupal.org/project/social_auth β as a guide
- π¬π§United Kingdom scott_euser
Okay updated the issue summary, maybe we start with the state change. So:
- dispatch TfaEnabledEvent when switch from 0 to 1
- dispatch TfaDisabledEvent when switch from 1 to 0
Then e.g. specific plugin changes if someone needs can be in a follow-up to keep this smaller.
- πΊπΈUnited States cmlara
Re #11 and #12: Event names sound good and plan to only deal with TFA Enabled/Disabled changes in this issue deferring plugins to a separate if requested by community.
Regarding layout: I would suggest we use the Event Class Name method rather than "string name" method. Given its design should make code management easier long term.
The Class Name method has no need for a matching map name or attempting to determine the correct string to match to the event, the class itself is the definition and dispatch can essentially be
$eventDispatcherInterface->dispatch(new TheEventClass());
- π¬π§United Kingdom scott_euser
Okay I think its there now matching your comments from #15 + the issue summary.
@astonvictor maybe you can check this still allows you to solve the original goal and confirm? Thanks! - π¬π§United Kingdom scott_euser
Okay back to needs review. Added a couple explanatory comments in case. Thanks!
- π¬π§United Kingdom scott_euser
Marking as postponed on π Fix reliability of storing status in user data Active