bossanova808 / CommerceRegisterOnCheckout

ARCHIVED: SEE README Allow user registration during Craft Commerce V1 checkouts.
Other
33 stars 3 forks source link

Check for password in controller #13

Closed msbit closed 7 years ago

msbit commented 7 years ago

Check for existence of password in POST vars for actionSaveRegistrationDetails and return a 400 with error if not found.

msbit commented 7 years ago

No rush on this one, I just noticed it when we had accidentally duplicated the password field on our submission form, so I'd managed to POST an empty password.

bossanova808 commented 7 years ago

I feel like there might be a more Craft like way to do this? e.g.

throw new HttpException(400, Craft::t('Error here'));

..that seems to be what BaseController.php does...might be good to follow that?

msbit commented 7 years ago

@bossanova808 that would definitely clean that up :)

Might be a bit odd for the expected JSON response in the AJAX case, but I'll take a look.

bossanova808 commented 7 years ago

I'd dust wrap this in an is ajax thingy, return json if it is, otherwise this....

msbit commented 7 years ago

Just means that the AJAX would return a 200, so errors get handled in the success handler on the browser. This is exactly what BaseController::returnErrorJson does though, so maybe this is more idiomatic Craft.

bossanova808 commented 7 years ago

That is, apparently, indeed the Craft way - in a bunch of other plugins they don't set the 400, they just returnErrorJson. I have always interprested it that the success/error stuff was about the ajax call (i..e no server errors), and the result handling was always handled therefore in success. But I am absoltuely no ajax expert, that's just the pattern I have noticed (and I only noticed it because it was confusing!)

bossanova808 commented 7 years ago

See, even that comment is confusing!

msbit commented 7 years ago

Ha, the crazy world of interop :)

I'll get rid of the manual 400 setting and squash commits. Should get a chance later this afternoon.

msbit commented 7 years ago

Okay, there you go, cleaned about as much as it can be.

msbit commented 7 years ago

Thanks @bossanova808 👍