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

Fix Guest Users #29

Closed
wants to merge 3 commits into from
Closed

Fix Guest Users #29

wants to merge 3 commits into from

Conversation

AIC-BV
Copy link
Contributor

@AIC-BV AIC-BV commented Feb 3, 2023

remove unique:users onRegister to fix User guest accounts
See: #28

remove unique:users onRegister to fix User guest accounts
See: wintercms#28
@mjauvin
Copy link
Member

mjauvin commented Feb 3, 2023

You would need to add the rule conditionally like here under complex conditional validation:

https://wintercms.com/docs/services/validation#conditionally-adding-rules

add sometimes validation
@AIC-BV
Copy link
Contributor Author

AIC-BV commented Feb 3, 2023

Ok I added sometimes

@mjauvin
Copy link
Member

mjauvin commented Feb 3, 2023

I had something more like this in mind:

$rules['email'] = 'required|between:6,255|email';

$validation->sometimes('email', 'required|between:6,255|email|unique', function($input) {
    return \Winter\User\Models\User::where('email', $input->email)->where('is_guest', 0)->first() !== null;
});

Not sure if there's a better way to achieve the same

@bennothommo
Copy link
Member

I'm going to close this and implement the validation here as Rule objects. This implementation is essentially checking the uniqueness of the email twice (once for the "sometimes" callback, and another with the unique rule that it adds), and also doesn't cover when usernames are used instead of email addresses for logins.

Thanks for the submission though, @AIC-BV.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Mar 13, 2023

@bennothommo
Does this mean it won't be implemented?
It is required to make the user plugin work with guest accounts, so it's an important fix, even though its not optimal

@bennothommo
Copy link
Member

I am working on an alternate PR that uses a Rule object instead, so it will be fixed.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 1, 2023

Is there any news on this? 😄

@LukeTowers
Copy link
Member

@bennothommo ping 😉

@bennothommo
Copy link
Member

Thanks for the reminder.

I did have a crack at this months ago, I think I got stopped by the fact we don't support DataAwareRules in Storm (https://laravel.com/docs/9.x/validation#using-rule-objects) yet. I need to include support for that in order to make the new rules work (albeit this would mean people would also need to update Winter and this plugin in order to support guests)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants