The MessageForm class on save() first sends the MailMessages and then it saves the message entity.

I need to send in the emails a URL to see the message entity, but I cannot do it because the message is not persisted yet (it doesn't have an ID).

CommentFileSizeAuthor
#12 Send-before-saving-2687507-6-12.patch1.18 KBsidharthap
#6 Send-before-saving-2687507-6-8.patch1.2 KBajalan065
#3 Send-before-save-2687507-3-8.patch1.2 KBajalan065
#2 contact-message_form_save-2687507-1.patch1.32 KBnicobot
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nicobot created an issue. See original summary.

nicobot’s picture

nicobot CreditAttribution: nicobot as a volunteer commented
Version: 8.1.0-beta1 » 8.1.x-dev
FileSize
contact-message_form_save-2687507-1.patch1.32 KB

Please find attached my proposal.

ajalan065’s picture

ajalan065 CreditAttribution: ajalan065 as a volunteer and at Innoraft commented
FileSize
Send-before-save-2687507-3-8.patch1.2 KB

Hope this passes

ajalan065’s picture

ajalan065 CreditAttribution: ajalan065 as a volunteer and at Innoraft commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: Send-before-save-2687507-3-8.patch, failed testing.

ajalan065’s picture

ajalan065 CreditAttribution: ajalan065 as a volunteer and at Innoraft commented
Status: Needs work » Needs review
FileSize
Send-before-saving-2687507-6-8.patch1.2 KB

Go Test Bot..

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

I assume you're using contact storage here?

nicobot’s picture

nicobot CreditAttribution: nicobot as a volunteer commented

Yes, I'm also using contact_storage

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Makes sense to me, but I'm wondering if we can somehow test this. We have a test to store messages, but we have no token support in core to put the Id into the label, without that, I guess it's not really possible to write a test, except writing a unit test.

Not sure if it's worth it, I'd say no :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Version: 8.2.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs work 
git ac https://www.drupal.org/files/issues/Send-before-saving-2687507-6-8.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1230  100  1230    0     0   4415      0 --:--:-- --:--:-- --:--:--  5371
warning: core/modules/contact/src/MessageForm.php has type 100644, expected 100755
error: patch failed: core/modules/contact/src/MessageForm.php:210
error: core/modules/contact/src/MessageForm.php: patch does not apply

Needs a reroll.

I'm happy to commit this code move without a test. Let's get this in 8.3.x first. It would seem eligible for 8.2.x as well to me.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap at TATA Consultancy Services for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
Send-before-saving-2687507-6-12.patch1.18 KB

Reroll patch as per #11

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC then.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Category: Bug report » Task
Status: Reviewed & tested by the community » Fixed

I'd say this is a task - not really a bug here. Hence only committing to 8.3.x. Committed 9b7b2ee and pushed to 8.3.x. Thanks!

  • alexpott committed 9b7b2ee on 8.3.x 
    Issue #2687507 by ajalan065, nicobot, sidharthap, Berdir: Send emails...

  • alexpott committed 9b7b2ee on 8.4.x 
    Issue #2687507 by ajalan065, nicobot, sidharthap, Berdir: Send emails...

  • alexpott committed 9b7b2ee on 8.4.x 
    Issue #2687507 by ajalan065, nicobot, sidharthap, Berdir: Send emails...

Status: Fixed » Closed (fixed)

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