Follow-up to: #1825044: Turn contact form submissions into full-blown Contact Message entities, without storage

Problem

  • The "Subject" and "Message" fields on contact forms are currently hard-coded and exposed via hook_field_extra_fields().
  • At minimum there's not really a reason for why the "Message" field should be required for a contact message bundle.

Proposed

  • Turn the "Subject" and "Message" fields into base fields of Message entity.
  • Remove them from hook_field_extra_field().
  • "Message" should be used to render a email body.
  • Take care about sanitization for field values in emails.
CommentFileSizeAuthor
#42 Screenshot 2014-07-11 06.41.29.png41.36 KBlarowlan
#42 Screenshot 2014-07-11 06.39.19.png42.66 KBlarowlan
#42 Screenshot 2014-07-11 06.37.36.png13.54 KBlarowlan
#42 Screenshot 2014-07-11 06.37.21.png33.83 KBlarowlan
#42 Screenshot 2014-07-11 06.37.03.png33.01 KBlarowlan
#42 Screenshot 2014-07-11 06.33.41.png40.31 KBlarowlan
#42 Screenshot 2014-07-11 06.29.43.png35.78 KBlarowlan
#41 1856562-contact-fields-41.patch16.92 KBandypost
#41 interdiff.txt1.52 KBandypost
#40 1856562-contact-fields-40.patch16.07 KBandypost
#39 1856562-contact-fields-39.patch16.54 KBandypost
#37 1856562-contact-fields-37.patch16.58 KBandypost
#35 1856562-contact-fields-35.patch16.47 KBandypost
#32 1856562-contact-fields-32.patch16.35 KBandypost
#32 interdiff.txt3.95 KBandypost
#31 1856562-contact-fields-31.patch19.83 KBandypost
#31 interdiff.txt606 bytesandypost
#29 1856562-contact-fields-29.patch19.83 KBandypost
#24 1856562-contact-fields-textarea-2.patch21.23 KBandypost
#24 interdiff.txt1.49 KBandypost
#21 1856562-contact-fields-textarea-1.patch22.39 KBandypost
#21 interdiff.txt10.19 KBandypost
#19 1856562-contact-fields-19.patch15.23 KBandypost
#19 interdiff.txt617 bytesandypost
#18 1856562-contact-fields-18.patch15.23 KBandypost
#18 interdiff.txt3.45 KBandypost
#14 1856562-contact-fields-14.patch13.5 KBandypost
#14 interdiff.txt1.19 KBandypost
#8 1856562-contact-fields-8.patch13.23 KBandypost
#8 interdiff.txt4.18 KBandypost
#6 1856562-contact-fields-6.patch9.76 KBandypost
#6 interdiff.txt849 bytesandypost
#4 1856562-contact-fields-4.patch9.83 KBandypost
#4 interdiff.txt6.31 KBandypost
#2 1856562-contact-fields-2.patch4.1 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Postponed » Active
andypost’s picture

This make contact module depends on text module's widgets.
Also removed "Message" from entity display because there's no entities saved to display.
So if any contrib will try to save messages to make em display it will need to implement hook_entity_field_info() and extend definition of "message" to be configurable with some formatter.

Status: Needs review » Needs work

The last submitted patch, 2: 1856562-contact-fields-2.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
6.31 KB
9.83 KB

fix tests

sun’s picture

Lovely patch :)

  1. +++ b/core/modules/contact/lib/Drupal/contact/Entity/Message.php
    @@ -154,13 +154,42 @@ public static function baseFieldDefinitions($entity_type) {
    +      ->setSettings(array(
    +        'default_value' => '',
    ...
    +      ->setSettings(array(
    +        'default_value' => '',
    

    Can we omit the default_value or is that an enforced requirement by Field API?

    Specifying an empty default value doesn't really make sense to me, especially given that both fields are required.

  2. +++ b/core/modules/contact/lib/Drupal/contact/Entity/Message.php
    @@ -154,13 +154,42 @@ public static function baseFieldDefinitions($entity_type) {
    +        'text_processing' => 0,
    ...
    +        'text_processing' => 0,
    

    Don't we have to update the Message entity form validation + submission code accordingly for these changes?

    I.e., if the site admin changes the field to allow users to use formatted text (e.g., to send contact message e-mails in HTML), then the submitted value actually needs to be consumed via $message->message->processed ?

andypost’s picture

1. fixed, sure
2. to change the settings users will need to implement hook_entity_field_info() and here override this definition.
The hook name will change in #2114707: Allow per-bundle overrides of field definitions

PS: for field validation we slowly moving to #2002180: Entity forms skip validation of fields that are edited without widgets
PPS: rest makes progress with step like #2002158: Convert form validation of comments to entity validation

andypost’s picture

Now MessageViewBuilder needs clean-up

andypost’s picture

FileSize
4.18 KB
13.23 KB

Seems we can get rid of view builder because "non-processed" text items always do nl2br(check_plain($text)) see TextProcessed.php

EDIT label is hidden by default because core does not allow labels for extra fields

So let's see

dawehner’s picture

+++ b/core/modules/contact/contact.module
@@ -142,27 +142,11 @@ function contact_field_extra_fields() {
-    $fields['contact_message'][$bundle]['form']['subject'] = array(
-      'label' => t('Subject'),
-      'description' => t('Text'),
-      'weight' => -10,
-    );

+++ b/core/modules/contact/lib/Drupal/contact/Entity/Message.php
@@ -154,13 +154,41 @@ public static function baseFieldDefinitions($entity_type) {
+    $fields['subject'] = FieldDefinition::create('text')
+      ->setLabel(t('Subject'))
+      ->setRequired(TRUE)

I don't really see how this was required before?

andypost’s picture

Issue summary: View changes
+++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
@@ -82,19 +82,6 @@ public function form(array $form, array &$form_state) {
-    $form['subject'] = array(
-      '#type' => 'textfield',
-      '#title' => t('Subject'),
-      '#maxlength' => 100,
-      '#required' => TRUE,

here it is

Berdir’s picture

  1. +++ b/core/modules/contact/contact.info.yml
    @@ -4,4 +4,6 @@ description: 'Enables the use of both personal and site-wide contact forms.'
     package: Core
     version: VERSION
     core: 8.x
    +dependencies:
    +  - text
    

    Note that this overlaps a bit with #2191285: Text module is not required, but is marked as required. It will add the dependecy to one of the contact tests, which won't be necessary anymore with this.

    Also, the dependency is added in light of that issue, if you're wondering why this adds a dependency on a required module.

  2. +++ b/core/modules/contact/contact.module
    @@ -217,7 +201,7 @@ function contact_mail($key, &$message, $params) {
           $message['subject'] .= t('[!category] !subject', $variables, $options);
           $message['body'][] = t("!sender-name (!sender-url) sent a message using the contact form at !form-url.", $variables, $options);
           $build = entity_view($contact_message, 'mail', $language->id);
    -      $message['body'][] = drupal_render($build);
    +      $message['body'][] = drupal_html_to_text(drupal_render($build));
           break;
     
         case 'page_autoreply':
    @@ -236,7 +220,7 @@ function contact_mail($key, &$message, $params) {
    
    @@ -236,7 +220,7 @@ function contact_mail($key, &$message, $params) {
           $message['body'][] = t("!sender-name (!sender-url) has sent you a message via your contact form at !site-name.", $variables, $options);
           $message['body'][] = t("If you don't want to receive such e-mails, you can change your settings at !recipient-edit-url.", $variables, $options);
           $build = entity_view($contact_message, 'mail', $language->id);
    -      $message['body'][] = drupal_render($build);
    +      $message['body'][] = drupal_html_to_text(drupal_render($build));
           break;
       }
    

    This seems strange?

    PhpMail::format() calls this too for the body, repeating this here means that it is impossible to send out HTML mails? So why do it?

  3. +++ /dev/null
    @@ -1,60 +0,0 @@
    -      // Convert field labels into headings.
    -      // @todo Improve drupal_html_to_text() to convert DIVs correctly.
    -      foreach (element_children($build) as $key) {
    -        if (isset($build[$key]['#label_display']) && $build[$key]['#label_display'] == 'above') {
    -          $build[$key] += array('#prefix' => '');
    -          $build[$key]['#prefix'] = $build[$key]['#title'] . ":\n";
    -          $build[$key]['#label_display'] = 'hidden';
    -        }
    -      }
    -      $build = array(
    -        '#markup' => drupal_html_to_text(drupal_render($build)),
    -      );
    

    Ah, is it to replace this? strange, really confused why we do this.

  4. +++ b/core/modules/contact/lib/Drupal/contact/Entity/Message.php
    @@ -154,13 +154,41 @@ public static function baseFieldDefinitions($entity_type) {
    +    // The subject of the contact message.
    +    $fields['subject'] = FieldDefinition::create('text')
    +      ->setLabel(t('Subject'))
    +      ->setRequired(TRUE)
    +      ->setSettings(array(
    +        'max_length' => 100,
    +        'text_processing' => 0,
    +      ))
    +      ->setDisplayOptions('form', array(
    +        'type' => 'text_textfield',
    +        'weight' => -10,
    +      ))
    +      ->setDisplayConfigurable('form', TRUE);
    

    Also, using text here just to get the widgets and formatters for it is just as wrong as it is for node.title, can we stop doing this? :(

Berdir’s picture

Berdir’s picture

Also, the issue summary needs to be updated, sorry for the comment spam.

andypost’s picture

Title: Convert "Subject" and "Message" into real Field API fields » Convert "Subject" and "Message" into Message base fields
Issue summary: View changes
FileSize
1.19 KB
13.5 KB

fix #11.1 after #2191285: Text module is not required, but is marked as required
#11.2-3 that was here already, also the loop is removed because labels now controlled from field_ui screen, module just shipped with sane defaults
#11.4 currently there's no way, do you think I need to include text_field_info_alter() from #2198917: Use the string field type for the node title field

Status: Needs review » Needs work

The last submitted patch, 14: 1856562-contact-fields-14.patch, failed testing.

Berdir’s picture

Uh oh, that fail in DependencyTest isn't good at all, worried that we introduced a random fail there by making text.module optional.

Value array ( 0 =&gt; &#039;filter&#039;, 1 =&gt; &#039;text&#039;, 2 =&gt; &#039;xmlrpc&#039;, 3 =&gt; &#039;ban&#039;, 4 =&gt; &#039;node&#039;, 5 =&gt; &#039;datetime&#039;, 6 =&gt; &#039;comment&#039;, 7 =&gt; &#039;history&#039;, 8 =&gt; &#039;number&#039;, 9 =&gt; &#039;options&#039;, 10 =&gt; &#039;taxonomy&#039;, 11 =&gt; &#039;forum&#039;, ) is identical to value array ( 0 =&gt; &#039;filter&#039;, 1 =&gt; &#039;xmlrpc&#039;, 2 =&gt; &#039;ban&#039;, 3 =&gt; &#039;text&#039;, 4 =&gt; &#039;node&#039;, 5 =&gt; &#039;datetime&#039;, 6 =&gt; &#039;comment&#039;, 7 =&gt; &#039;history&#039;, 8 =&gt; &#039;number&#039;, 9 =&gt; &#039;options&#039;, 10 =&gt; &#039;taxonomy&#039;, 11 =&gt; &#039;forum&#039;, ).	Other	DependencyTest.php	156	Drupal\system\Tests\Module\DependencyTest->testModuleEnableOrder()	<img src="/misc/watchdog-error.png" alt="" title="" width="18" height="18" />

@andypost: I would prefer to postpone this on the node title issue, so that we can use string for the field type and formatter, still need the text.module dependency for widget but that's ok, although a string widget would be nice as well.

sun’s picture

I don't think it's random. This test failure is the exact reverse of the test failure that we had in #2191285: Text module is not required, but is marked as required before we switched from the dependency definition in the info file to just changing the test $modules.

→ The mere existence of any declared dependencies (as opposed to none) seems to change the order of modules, even when completely unrelated to the module being installed.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.45 KB
15.23 KB
andypost’s picture

No reason in text_plain formatter here too

The last submitted patch, 18: 1856562-contact-fields-18.patch, failed testing.

andypost’s picture

Combined patch with #2181549: Provide a StringLong field item with schema type 'text' without a 'format' column also adds StringTextareaWidget and remove dependency on text module

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/StringTextareaWidget.php
@@ -67,7 +66,7 @@ public function settingsSummary() {
+    $element['value'] = $element + array(

Is there any reason for that 'value' dance here? I already don't really understand it in TextareaWidget but here it seems completely superfluous, no?

Status: Needs review » Needs work

The last submitted patch, 21: 1856562-contact-fields-textarea-1.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
21.23 KB

@tstoeckler This element is needed to allow field api extract value, the key name should be equal to type property

Fix tests:
1) text module used in test to add text field to contact form
2) revert dependency test

tstoeckler’s picture

Ahh thanks @andypost, I wasn't aware of that. But it's true, all of the core widget include the 'value' form key.
Couldn't find anything to complain about. Could use another set of eyes, though.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/StringTextareaWidget.php
@@ -75,28 +74,6 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
-  /**
-   * {@inheritdoc}
-   */
-  public function errorElement(array $element, CoanstraintViolationInterface $violation, array $form, array &$form_state) {
-    if ($violation->arrayPropertyPath == array('format') && isset($element['format']['#access']) && !$element['format']['#access']) {
-      // Ignore validation errors for formats if formats may not be changed,
-      // i.e. when existing formats become invalid. See filter_process_format().
-      return FALSE;
-    }
     return $element;
   }

This is the default implementation, so we can remove that method completely.

tstoeckler’s picture

Re #26: Which method do you mean? errorElement() is being removed completely?!

Berdir’s picture

Oh, I failed at reading the patch. Yes sorry, that looks fine :)

andypost’s picture

Berdir’s picture

Overall, this looks great. I've been playing around with this to figure out how and why we need the drupal_html_to_mail() call there.

I noticed this:

Assume you enter the following:

<strong>HIIIII</strong>
<h1>Here I am</h1>

Then on HEAD, you get:

         Message
         *HIIIII*
======== HERE I AM 
===========================================================

Now, with the patch, you get:

&lt;strong&gt;HIIIII&lt;/strong&gt;
&lt;h1&gt;Here I am&lt;/h1&gt;

That's because we run it through the string formatter now, which does a check_plain(), before we call drupal_html_to_mail()

I have no idea what's better, the fact that you can use HTML right now and it's transformed into that crazy stuff seems quite weird.. Maybe contact or Core should provide a "Strip all tags" formatter? On the other side, we need to be careful to not start to re-implement the filter system ;)

However, what is different now is that the label is missing, is that by design? There's currently no separation at all between different fields if you add multiple files. Maybe each field should be added as a separate body element?

Either way, the drupal_html_to_text() call is unnecessary. Right now, we definitely run it through through that function twice and we have now the check_plain() for added security, so even when a different mail backend is used, we're safe. And not doing it there hardcoded means that someone is free to use mimemail or something for html mails and has a change to format them in a useful way. So I'd recommend to just leave that away.

andypost’s picture

Issue tags: +Needs tests
FileSize
606 bytes
19.83 KB

@berdir, You are wrong here, drupal_html_to_text() is needed because this drupal_html_to_text() coverts this HTML entities (encoded by check_plain() in formatter) back to HTML so any mail backend will get allowed HTML.

Other part makes sense, because affects body of the message! So needs tests.

before:

Message
       <strong>HIIIII</strong>
<h1>Here I am</h1>

after:

Message<strong>HIIIII</strong>
<h1>Here I am</h1>

Yes, \Drupal\Core\Mail\Plugin\Mail\PhpMail::format() calls drupal_html_to_text() again but this is not changed by this patch.

See the old code flow in view builder:

  1. +++ /dev/null
    @@ -1,61 +0,0 @@
    -  public function buildContent(array $entities, array $displays, $view_mode, $langcode = NULL) {
    ...
    -          '#type' => 'item',
    -          '#title' => t('Message'),
    -          '#markup' => String::checkPlain($entity->getMessage()),
    

    encodes entities

  2. +++ /dev/null
    @@ -1,61 +0,0 @@
    -  public function view(EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {
    ...
    -    if ($view_mode == 'mail') {
    ...
    -      $build = array(
    -        '#markup' => drupal_html_to_text(drupal_render($build)),
    

    decodes them back

andypost’s picture

I think that changes in message formating is out of the scope of the conversion to base fields, so let's left MessageViewBuilder as is here

Berdir’s picture

Interesting.

@andypost: You are correct that it converts back but it doesn't convert then, only on the second call:

<?php

use Drupal\Component\Utility\String;

$string = '<strong>wCe2a8mt</strong>';
print "Original: $string\n";
$string = String::checkPlain($string);
print "check plain: $string \n";
$string = drupal_html_to_text($string);
print "First run: $string \n";
$string = drupal_html_to_text($string);
print "Second run: $string \n";
$string = drupal_html_to_text($string);
print "Third run: $string \n";

The result is:

Original: <strong>wCe2a8mt</strong>
check plain: &lt;strong&gt;wCe2a8mt&lt;/strong&gt; 
First run: <strong>wCe2a8mt</strong>
 
Second run: *wCe2a8mt*
 
Third run: *wCe2a8mt*

So yes, we kind of need to call it twice but that's super weird? And honestly, dropping all tags would make more sense, but we can look into that in a follow-up.

And about the label, the view builder seems to try and hide the label too but apparently it's not working. So the field definition was kind of correct as this seems to be the expected result.

andypost’s picture

andypost’s picture

FileSize
16.47 KB

re-roll

Berdir’s picture

andypost’s picture

FileSize
16.58 KB

re-roll

andypost’s picture

Anything left here except final review?

PS: I got issue with force wrapping of the message body in russian language...

andypost’s picture

re-roll

andypost’s picture

FileSize
16.07 KB

re-roll for psr-4

andypost’s picture

FileSize
1.52 KB
16.92 KB

more clean-up

larowlan’s picture

Did a code review, looks good - reviews have occurred already from @berdir too.
Manually tested, worked as designed, only one issue - a dblog error about theme_contact_message not existing.
But I tested that against HEAD and same error exists - will open a new issue.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Actually, needs work because of the Needs tests tag
We don't have tests for the new features.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

There's follow-up to fix mail and remove builder #2223967: Do not decode a contact message twice
Probably needs change notice about new widget

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Discussed with andypost in irc, 'needs tests' tag was for stuff reverted in #32 (formatter)
So back to rtbc

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 158863a and pushed to 8.x. Thanks!

  • alexpott committed 158863a on 8.x
    Issue #1856562 by andypost | sun: Convert "Subject" and "Message" into...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.