Closed pboling closed 3 years ago
@btalbot release 3.0.5 is out with the fix. Please let me know if there are any issues.
Hi @pboling
Thank you for the updates, they are much better than what existing previously. However, there are two issues I've run into while testing code with your changes.
new
method they wished. The updated strategy always attempts to call the model's new
method with the attributes hash even if the strategy might later attempt to call ::create
instead. This breaks old model implementations that do not expect their initializer to be called with a hash. I think that to remain backwards compatible the strategy must only use ::create
and never expect to call the model initializer directly.#saving_instead_of_creating?
will always return true for any model that extends the OmniAuth::Identity::Model since that model does define #save
and #persisted?
methods even if they do raise an NotImplementedError. Maybe just attempting to call the new #save
method and then handling the NotImplementedError with the fallback method would work?
https://github.com/omniauth/omniauth-identity/blob/9f558858bcdb396ad0984dd2045e10d25f546b1c/lib/omniauth/strategies/identity.rb#L144-L146Oops, I didn't see this before I released more fixes in 3.0.7. I will continue working on this @btalbot. Both of the points you make are valid, and haven't been fully resolved by the most recent release.
@btalbot The fix for both issues was fairly simple. It is impossible to use the options[:on_validation]
unless also using new
/save
, so I just made the presence of that option the decider. If the option isn't set, then create
works fine. I think that resolves the issues you found.
It just means that some implementations can't use options[:on_validation]
, which should surprise no-one.
That is simple and does seem to allow the ::create method to be used when #validating? is false. How should validation be configured to use the #save but without actually using the validation feature? Seems like we will need to create a valid on_validation block which just always returns true?
@btalbot No, the default for options[:on_validation]
is nil
. If it is provided by an implementation that implementation has always, since the introduction of the feature a few weeks ago, had to support new
and save
, so there is no change for those users.
What is expected config for an app to use new
and save
but with no (or default) on_validation?
They will not use new
and save
. Wasn't that the point of your initial issue? We can't introduce new and save in a breaking way for existing implementations. Introducing them as part of an entirely new feature, which on_valdiation
is, is not breaking.
In this context, the only reason to use new
and save
instead of create
is to validate before persisting. I also undeprecated create
. We'll just keep this pattern for now.
Yes, the 3.0.8 release does resolve the backward compatibility issue and tests using existing implementations now pass. Thank you.
A future step is to remove the use of the deprecated create
method and use new
and save
instead. When doing that, I'll just also add a lamba for on_validation = lamda { |_| true }
to allow them to be used.
Also, future implementers will need to know that the on_validation call() provides a hash parameter and not env
or req
as most of the other blocks do.
For example
on_login = lambda { |env| puts "env is #{env}" }
on_validation = lambda { |args| puts "env is #{args[:env]}" }
@btalbot - Working on a fix.
Fixes
:on_validation
Added
#save
,#persisted?
and::create
onOmniauth::Identity::Model
@since
YARD tags to interface methodsOmniauth::Strategies::Identity.registration_phase
to supportOmniauth::Identity::Model
-inheriting classes that do not define#save
.