backdrop / backdrop-issues

Issue tracker for Backdrop core.
145 stars 40 forks source link

user_validate_mail() no longer exists but is referenced in user_account_form() #1416

Open quicksketch opened 8 years ago

quicksketch commented 8 years ago

The user_validate_mail() function no longer exists. Presumably this is because we're using the HTML5 #email element now, and so we don't need a special validator for user forms now.

However there are a few issues:

For starters we just need a PR to remove the @see mention in user_account_form().


~PR by @BWPanda: https://github.com/backdrop/backdrop/pull/2722~

ghost commented 5 years ago

Made a PR: https://github.com/backdrop/backdrop/pull/2722

klonos commented 5 years ago

Shouldn't we be adding a wrapper function and use watchdog_deprecated_function()? ...at least keep that around for the 1.x cycle, while contrib might be using user_validate_mail().

ghost commented 5 years ago

@klonos I think the point of this issue is that user_validate_mail() has already been removed, and we're just removing a left-over reference to it (from a comment). So there's nothing to add a wrapper to...

klonos commented 5 years ago

We have been adding wrapper functions for previously-removed functions, as part of our commitment to better DX (for contrib authors that are porting 7.x projects mainly). See #3716 for example, which has a @since 1.13.1 Function added. and a @deprecated since 1.0.0. Another example is #3279; I'm sure there were more.

klonos commented 5 years ago

...we need a change record, and then a link pointing to it from within watchdog_deprecated_function().

ghost commented 5 years ago

Ah ok, thanks for the explanation :slightly_smiling_face:

klonos commented 2 years ago

I've created and published this change record to go with the watchdog_deprecated_function() function: https://docs.backdropcms.org/change-records/function-user_validate_mail-has-been-removed

@BWPanda do you have capacity to update your PR?

ghost commented 2 years ago

Happy for someone else to take over.

klonos commented 2 years ago

Grabbing this then ✋🏼

klonos commented 2 years ago

PR up for review: https://github.com/backdrop/backdrop/pull/3990

Nothing to test.

argiepiano commented 2 years ago

Code looks good to me - RTBC. Thanks, @quicksketch for reporting and @klonos and @BWPanda for providing the fix. I'm not sure who needs to do the change record. (the change record is already published)

jenlampton commented 2 years ago

A added a comment to the PR.

Adding back the function as only a wrapper, but not actually providing the previous validation, would lead to a scenario where a drupal module that relied on this function would simply not validate at all anymore. This could be a security issue.

If we're going to deprecate the function, let's copy the whole previous function. Otherwise, let's not add it back so that sites that were using it before will get a fatal error, and will need to actually address the problem (which they can do now, thanks do the change record).

avpaderno commented 1 year ago

Backdrop core implements valid_email_address(), which is used by user_account_form_validate(), the validation handler for the user_account_form() form.
Should not user_account_form() eventually have @see valid_email_address()?

herbdool commented 10 months ago

If the function no longer exists, then it's not even deprecated. We shouldn't bring it back just so it can have a deprecated function. We've got a change record, that's enough. And the reference in the docblock can be removed.