Provide an alter for user settings

Created on 27 June 2025, about 1 month ago

It would be a good option to alter user data before saving.
It should be updated in the tfaSaveTfaData() method.

✨ Feature request
Status

Active

Version

2.0

Component

Code

Created by

πŸ‡ΊπŸ‡¦Ukraine AstonVictor

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • 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 Kingdom scott_euser
  • Pipeline finished with Failed
    about 1 month ago
    Total: 447s
    #533653
  • πŸ‡¬πŸ‡§United Kingdom scott_euser
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 244s
    #533663
  • Pipeline finished with Success
    about 1 month ago
    Total: 472s
    #533665
  • πŸ‡ΊπŸ‡Έ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());

  • Pipeline finished with Canceled
    about 1 month ago
    Total: 123s
    #534921
  • Pipeline finished with Success
    about 1 month ago
    Total: 349s
    #534925
  • πŸ‡¬πŸ‡§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!

  • πŸ‡ΊπŸ‡¦Ukraine AstonVictor

    it works for me.
    thanks a lot

  • πŸ‡ΊπŸ‡ΈUnited States cmlara
  • Pipeline finished with Canceled
    27 days ago
    Total: 124s
    #539446
  • Pipeline finished with Failed
    27 days ago
    Total: 333s
    #539448
  • Pipeline finished with Success
    27 days ago
    Total: 389s
    #539459
  • πŸ‡¬πŸ‡§United Kingdom scott_euser
  • Pipeline finished with Failed
    24 days ago
    Total: 366s
    #540694
  • Pipeline finished with Failed
    24 days ago
    Total: 329s
    #540712
  • Pipeline finished with Success
    24 days ago
    Total: 447s
    #540713
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Okay back to needs review. Added a couple explanatory comments in case. Thanks!

  • Pipeline finished with Success
    24 days ago
    Total: 375s
    #540799
  • Pipeline finished with Success
    24 days ago
    Total: 259s
    #540832
  • Pipeline finished with Success
    24 days ago
    Total: 277s
    #540838
  • Pipeline finished with Canceled
    24 days ago
    Total: 82s
    #541277
  • Pipeline finished with Success
    24 days ago
    Total: 381s
    #541280
  • Pipeline finished with Failed
    23 days ago
    Total: 341s
    #541499
  • Pipeline finished with Success
    23 days ago
    Total: 375s
    #541506
  • Pipeline finished with Failed
    23 days ago
    Total: 451s
    #541511
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Marking as postponed on πŸ“Œ Fix reliability of storing status in user data Active

Production build 0.71.5 2024