Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#42 | Screenshot 2014-07-11 06.41.29.png | 41.36 KB | larowlan |
#42 | Screenshot 2014-07-11 06.39.19.png | 42.66 KB | larowlan |
#42 | Screenshot 2014-07-11 06.37.36.png | 13.54 KB | larowlan |
#42 | Screenshot 2014-07-11 06.37.21.png | 33.83 KB | larowlan |
#42 | Screenshot 2014-07-11 06.37.03.png | 33.01 KB | larowlan |
Comments
Comment #1
sunComment #2
andypostThis 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.Comment #4
andypostfix tests
Comment #5
sunLovely patch :)
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.
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
?Comment #6
andypost1. 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
Comment #7
andypostNow
MessageViewBuilder
needs clean-upComment #8
andypostSeems we can get rid of view builder because "non-processed" text items always do
nl2br(check_plain($text))
see TextProcessed.phpEDIT label is hidden by default because core does not allow labels for extra fields
So let's see
Comment #9
dawehnerI don't really see how this was required before?
Comment #10
andyposthere it is
Comment #11
BerdirNote 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.
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?
Ah, is it to replace this? strange, really confused why we do this.
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? :(
Comment #12
BerdirSee #2198917: Use the string field type for the node title field
Comment #13
BerdirAlso, the issue summary needs to be updated, sorry for the comment spam.
Comment #14
andypostfix #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 fieldComment #16
BerdirUh oh, that fail in DependencyTest isn't good at all, worried that we introduced a random fail there by making text.module optional.
@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.
Comment #17
sunI 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.
Comment #18
andypostIssue #2198917: Use the string field type for the node title field is in. So re-roll and fix for test.
Also we can remove dependency on text module after #2181549: Provide a StringLong field item with schema type 'text' without a 'format' column
Comment #19
andypostNo reason in text_plain formatter here too
Comment #21
andypostCombined patch with #2181549: Provide a StringLong field item with schema type 'text' without a 'format' column also adds
StringTextareaWidget
and remove dependency on text moduleComment #22
tstoecklerIs there any reason for that 'value' dance here? I already don't really understand it in TextareaWidget but here it seems completely superfluous, no?
Comment #24
andypost@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
Comment #25
tstoecklerAhh 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.
Comment #26
BerdirThis is the default implementation, so we can remove that method completely.
Comment #27
tstoecklerRe #26: Which method do you mean? errorElement() is being removed completely?!
Comment #28
BerdirOh, I failed at reading the patch. Yes sorry, that looks fine :)
Comment #29
andypostRe-roll after #2181549: Provide a StringLong field item with schema type 'text' without a 'format' column
Comment #30
BerdirOverall, 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:
Then on HEAD, you get:
Now, with the patch, you get:
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.
Comment #31
andypost@berdir, You are wrong here,
drupal_html_to_text()
is needed because thisdrupal_html_to_text()
coverts this HTML entities (encoded bycheck_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:
after:
Yes,
\Drupal\Core\Mail\Plugin\Mail\PhpMail::format()
callsdrupal_html_to_text()
again but this is not changed by this patch.See the old code flow in view builder:
encodes entities
decodes them back
Comment #32
andypostI think that changes in message formating is out of the scope of the conversion to base fields, so let's left
MessageViewBuilder
as is hereComment #33
BerdirInteresting.
@andypost: You are correct that it converts back but it doesn't convert then, only on the second call:
The result is:
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.
Comment #34
andypostFiled follow-up for that #2223967: Do not decode a contact message twice
Comment #35
andypostre-roll
Comment #36
Berdir35: 1856562-contact-fields-35.patch queued for re-testing.
Comment #37
andypostre-roll
Comment #38
andypostAnything left here except final review?
PS: I got issue with force wrapping of the message body in russian language...
Comment #39
andypostre-roll
Comment #40
andypostre-roll for psr-4
Comment #41
andypostmore clean-up
Comment #42
larowlanDid 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.
Comment #43
larowlanActually, needs work because of the Needs tests tag
We don't have tests for the new features.
Comment #44
andypostThere's follow-up to fix mail and remove builder #2223967: Do not decode a contact message twice
Probably needs change notice about new widget
Comment #45
larowlanDiscussed with andypost in irc, 'needs tests' tag was for stuff reverted in #32 (formatter)
So back to rtbc
Comment #46
alexpottCommitted 158863a and pushed to 8.x. Thanks!