Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to: #1825044: Turn contact form submissions into full-blown Contact Message entities, without storage
Problem
- The user contact form is not a contact message bundle currently, because it has a "dynamic recipient."
- For the same reason, it is also not fieldable.
Goal
- Turn the user contact form into a (locked) category/bundle.
Details
- Technically, the recipient of the user contact form is delivered through an external data source/context; an argument in the route. In modern blocks/layout/services speak, it could be expressed with an $account context, and the contact category configuration would use a recipient value that is a token, [recipient:mail].
- However, at this point it's unclear whether and how this could be implemented, so it is probably better to simply introduce a new,
contact.category.user
bundle that is "locked" (non-deletable) and special-case it.
Proposed solution
- Add a
contact.category.user
bundle. - Add a
locked: 1
property to the user bundle; adjust the category admin UI to disallow deletion of locked categories. - Add a condition to CategoryFormController to not expose the recipients field for the user bundle.
- Automatically populate the message $recipients from the passed-in $account->mail context.
Blocked by
- #1849158: Expose contact category-specific forms and/or their URLs somewhere — The user bundle cannot/should not appear as a regular category tab on the contact form.
Comment | File | Size | Author |
---|---|---|---|
#36 | 1856556-contact-user-36.patch | 14.56 KB | Berdir |
#36 | 1856556-contact-user-36-interdiff.txt | 699 bytes | Berdir |
#35 | 1856556-contact-user-35-interdiff.txt | 1.42 KB | Berdir |
#35 | 1856556-contact-user-35.patch | 14.57 KB | Berdir |
#33 | Contact form categories.png | 21.36 KB | das-peter |
Comments
Comment #1
sunComment #2
amateescu CreditAttribution: amateescu commentedThis problem is biting us in #1875992: Add EntityFormDisplay objects for entity forms, so it's actually a bug.
Comment #3
andypost@amateescu PLease describe a problem with pointed patch? Is there possibility to make a stab or this one a hard dependency?
Comment #4
webchickThat's just a normal task, which makes this just a normal bug.
Comment #5
amateescu CreditAttribution: amateescu commentedRe #3: You can see the problem here: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
There is no hard dependency, this one just needs to be fixed :)
Comment #6
andypostAlso we could use "enabled" property of the configurable to allow-disallow usage of personal form
Comment #7
andypostInitial patch with tests.
I also disable machine name change for 'user' category.
Comment #8
andypostRe-roll
- new access check
- added accessController
Comment #9
andypostThis will have collisions with #1983548: Convert contact message entities to the new Entity Field API
Comment #10
BerdirI named mine personal, initially considered user as well but like personal better as it matches what it's called in e.g. the page callback.
Needs a re-roll due to the refactored access controller, this is now checkAccess() with $operation.
That said, I like using entity access for this. See below, we should probably block edit es well.
I've completely hidden the edit form as well, not sure what's better. Being able to edit this seems rather useless as none of the options make any sense (no label/description is ever displayed, it can't be the default category).
Not here, but we should do this and edit access checks by default in the parent implementation. That will require an access controller for all config entities but we want that anyway I think.
Comment #11
Berdir#8: 1856556-contact-user-8.patch queued for re-testing.
Comment #12
BerdirThis is blocking #1983548: Convert contact message entities to the new Entity Field API, which is part of a critical meta, so this has to happen anyway, whether it's major or not ;)
The entityInfo() hack can be removed now that personal/user is a real category/bundle, see the interdiff in my patch over there. And some checks that should probably be a == personal check now instead of checking the Same for two todo's although I didn't dynamically preset the category recipients as that seems hackish to me.
Maybe add an isPersonal() method to the class as well, for the few checks that we need?
Comment #13
andypostSo here's re-roll
Comment #14
andypostAnd another place
Comment #15
andypostNice idea, so approach to hide Edit isPersonal() seems cleaner until we get #1831928: Support a 'locked' property on ConfigEntities
Comment #17
andypostFinally, should be green
Comment #18
BerdirAs mentioned above, I'd suggest the same check for the update operation and hide edit complete.
Then these changes aren't necessary anymore.
Then add the same check here with update/edit and add a @todo for moving that check to the parent implementation.
And as mentioned above, let's add isPersonal(), port over the removal of entityInfo() and remove the @todo's/update checks in the form controller.
Comment #19
BerdirThe access check stuff is happening in #1995048: EntityListController::getOperations() should respect access checks. Don't think it's a blocker though, will just need a re-roll once one issue got in.
Comment #20
BerdirSomething like this.
Comment #21
BerdirThis is blocking the contact message NG conversion, tagging accordingly.
Comment #22
das-peter CreditAttribution: das-peter commentedI reviewed the patch, but didn't manually test the functionality.
As I don't really know all this stuff, the only thing I can say is that the patch looks clean and I don't see anything that would speak against setting it to RTBC.
Comment #23
andypostNice
Comment #24
catchThe isPersonal() method is added to the interface and implemented, but it's never called?
Comment #25
BerdirHm, yes, a few checks were on $message->recipient but some where wrong now that there is a category for personal messages.
Comment #26
BerdirHm, yes, a few checks were on $message->recipient but some where wrong now that there is a category for personal messages.
Comment #27
BerdirOh, would help to upload the actual patch as well.Oh, I did above and double-posted the interdiff? WTH?
Comment #29
andypost#27: 1856556-contact-user-25.patch queued for re-testing.
Comment #30
BerdirOk, fixed the testfail and added test coverage for actually sending personal contact messages, we don't have that now. The added test coverage fails without the previous fix.
Comment #31
BerdirTo manually test this, you need a system that can send out mails. Not sure if simplytest.me supports that?
Apply the patch, enable the contact module if it's not yet already. Then make sure you can use the send contact messages on the personal contact tab on a user, make sure it's using the configured e-mail address of the user to send the e-mail to and there aren't any errors.
Have a look at the contact categories (below structure), make sure that you can not edit or delete the personal contact category but you can add, edit and delete others.
Comment #32
BerdirTagging.
Comment #33
das-peter CreditAttribution: das-peter commentedI set the permission so that the anonymous user was able to use the personal contact forms:
Sending mails seems to work, and a changed e-mail address is reflected too.
I can't access the personal contact category in the UI but manually
admin/structure/contact/manage/personal/edit
works and I can make and save changes (as administrator).However, the changes don't seem to have an effect when a personal contact form is used - e.g. the recipient of the mail is still the one of the user and not the new value from the changed category settings.
So I think that's ok that way, right?
Comment #34
andypost@das-peter Thanx a lot
Now it's ready
Comment #35
BerdirAlmost ;)
Didn't apply anymore and that page should actually not be accessible. Which can be fixed easily by specifying the correct route access definition as we already do for delete.
Also added tests for that and noticed strange fails where it accidently used personal as the category to test stuff on and that obviously failed. Just removed the random $id there and rely on the id that we create earlier in the same test, not visible in the interdiff.
Comment #36
Berdir@andypost mentioned that we don't need the ' there.
Comment #37
andypostNice clean-up
Not sure I get the reason of this. But it was introduced in #1588422-60: Convert contact categories to configuration system
PS: Probably it was introduced to make sure that category exists
Comment #38
alexpottCommitted 99861e8 and pushed to 8.x. Thanks!
We need to address #1849158: Expose contact category-specific forms and/or their URLs somewhere / #1997692: Create contact form block before too much more work on contact categories occurs.
Comment #39
BerdirYay, removing from sprint.
Comment #40.0
(not verified) CreditAttribution: commentedUpdated issue summary.