contact.module allows you to send emails to users with no email address - producing an error

An ajax event is added to the mail form element. The personal contact form settings are disabled/enabled depending on the mail field.
I used #ajax to change the disabled element message too in order to give a feedback for the current user.

I don't think ajax is the answer here. I think a 404 for said users at user/{uid}/contact make more sense

Ie we need both

something like this

and some tests

Na you're right, need to go before the admin bit. Also, tab access might be different, will have to take a look.

tabs (local tasks) are differerent plugins, so probably re-use menu-links access checking

Patch no longer applies. I'd say an error that can be produced through the UI qualifies as a bug rather than a task.

I tried to work on a bit of a reroll of this, but a few things came up that persuaded me to stop and see if there was a better way to proceed. This will give a couple of hints if someone else wants to take a stab at this.

1. If the ajax callback is to be kept, note that valid_email_address() is on its way to the graveyard and we should be using the email validation service instead:

return \Drupal::service('email.validator')->isValid($mail);

See: #2343043: valid_email_address() should use egulias/EmailValidator and become deprecated

2. The return values for access() have changed. The following works, which I would insert between the 'self-contact' test and the administrator override:

   // It is not possible to contact users with no email address defined.
    if (!$contact_account->getEmail()) {
      return AccessResult::forbidden();
    }

Now to the part that my relatively novice knowledge of the routing system, etc. has not prepared me for.

I tend to agree that, if we are going to throw an error on visiting user/{uid}/contact for someone without an email address defined, it seems more logical to return 404 (not found) rather than 403 (access denied). The latter is a more accurate description of the error.

Ergo, it seems to me a little bit of a kludge to implement this at the 'access' level. ContactController::contactSitePage() throws a NotFoundHttpException() if a user tries to visit a non-existent default site form, and we can do the same in ContactController::contactPersonalPage(). That works as expected.

However, that doesn't hide the Contact tab on the user profile. This was done in prior patches by denying access to the tab. But is there a better way to disable this without hacking a check into the access control layer?

The problem with doing it in the access check is that this fires before the controller callback, resulting in a 403 Forbidden error being returned to the client when visiting user/{uid}/contact, rather than a "not found" error (since ContactController::contactPersonalPage() never executes to throw the more meaningful 404 error). Throwing that error in the access() callback is not an option because that presumably breaks the user profile page, too.

(Working on this...)

Added beta evaluation.

Two patches are attached.

The first patch fixes the problem described in the issue description:

1. the "Contact" tab is hidden on user profiles where the user has no email address (for all users);
2. if someone indirectly finds their way to user/{nid}/contact for a user with no e-mail address, a 404 error is returned

This does not yet deal with the account profile form or include the Ajax code provided above, as this has to be completely re-factored by the looks of it, and quite simply, I've not figured out even how to add additional validation to the form never mind Ajax validation.

The tests check for both of these conditions, and I expect two failures to be reported.

Before moving on to 'enhancements' with respect to the account edit forms, are we satisfied that this at least prevents the identified bug from manifesting?

@tibbsa, can you roll a patch that includes the tests and the fix in a single file, to illustrate that the fix truly addresses the failing tests?

Combined patch + tests.

This does not yet deal with the account profile form or include the Ajax code provided above, as this has to be completely re-factored by the looks of it, and quite simply, I've not figured out even how to add additional validation to the form never mind Ajax validation.

I think it is OK that this doesn't take the ajax approach discussed above. Simply removing the tab and setting a 404 for the page makes sense to me.

Can maybe be fixed pre-commit so not setting to "needs work":

1. +++ b/core/modules/contact/contact.module
   @@ -85,6 +86,27 @@ function contact_entity_extra_field_info() {
   + * Hide the 'Contact' tab on the user profile if the user does not have
   + * an email address configured.
   

   I think it would be more consistent to say "Hides", instead of "Hide". Also, this breaks before 80 chars.

2. +++ b/core/modules/contact/contact.module
   @@ -85,6 +86,27 @@ function contact_entity_extra_field_info() {
   +function contact_menu_local_tasks_alter(&$data, $route_name)
   +{
   

   The opening brace should be on the same line as the function definition

Fixed the minor issues identified in #24, and re-rolled.

I think this is back to RTBC.

Committed f067cd5 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.