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

Call before/after_create_account if before/after_omniauth_create_account is not provided #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

enescakir
Copy link
Contributor

Since it uses the login plugin, our after_login continues to work as expected when we introduce the rodauth-omniauth plugin to our application.

However, since rodauth-omniauth defines its own new_account and save_account methods, we need to override
before/after_omniauth_create_account.

I think it would be better if the rodauth-omniauth plugin called before/after_create_account when
before/after_omniauth_create_account is not provided.

This change will help integrate the rodauth-omniauth plugin into existing applications without extra work.

In the next steps, rodauth-omniauth may also call new_account and save_account from Rodauth. These methods look very similar to the original new_account and save_account methods in Rodauth.

…ccount` is not provided

Since it uses the `login` plugin, our `after_login` continues to work as
expected when we introduce the `rodauth-omniauth` plugin to our
application.

However, since `rodauth-omniauth` defines its own `new_account` and
`save_account` methods, we need to override
`before/after_omniauth_create_account`.

I think it would be better if the `rodauth-omniauth` plugin called
`before/after_create_account` when
`before/after_omniauth_create_account` is not provided.

This change will help integrate the `rodauth-omniauth` plugin into
existing applications without extra work.

In the next steps, `rodauth-omniauth` may also call `new_account` and
`save_account` from Rodauth. These methods look very similar to the
original `new_account` and `save_account` methods in Rodauth.
@enescakir
Copy link
Contributor Author

Hi @janko, I found it useful, but what do you think?

Comment on lines +166 to +168
def _before_omniauth_create_account
before_create_account
end
Copy link
Contributor Author

@enescakir enescakir Dec 27, 2024

Choose a reason for hiding this comment

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

Hi @jeremyevans, is this the correct way to provide default methods for before and after hooks in Rodauth world?

Choose a reason for hiding this comment

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

You would want to remove the existing method first, but that that is the correct method to override. I don't think it is a good idea to call hook method explicitly inside other hook methods by default, though.

The approach I would recommend, if the behavior in this pull request is desired, is for the omniauth code that creates accounts to call the before_create_account and after_create_accounts methods separately (before or after calling the before_omniauth_create_account and after_omniauth_create_account methods). For backwards compatibility, that would likely have to be conditional and off by default, though.

@janko
Copy link
Owner

janko commented Dec 27, 2024

I'm not sure it's a good idea to call {before,after}_create_account hooks on OmniAuth login. I think people implementing these hooks expect to be in context of a registration form submit, so I imagined existing hooks won't automatically work when OmniAuth is added to the mix, that you would have to have different branches anyway. I'm not entirely against it, but it would at the very least be a breaking change.

FWIW, one feature I would like to add to rodauth-omniauth eventually is redirecting to the create account form after OmniAuth login, to require the account to be created explicitly and allow for any additional account fields that might be required by the app. In that case, that would happen through the classic create account form, so it would trigger the {before,after}_create_account hooks. However, I don't know when I'll have time to work on this, and whether rodauth-omniauth should still continue to support automatic creation.

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