Updated: Comment #N

Problem/Motivation

    if (!$message->isPersonal()) {
      watchdog('contact', '%sender-name (@sender-from) sent an e-mail regarding %category.', array(
        '%sender-name' => $sender->name,
        '@sender-from' => $sender->getEmail(),
        '%category' => $category->label(),
      ));
    }
    else {
      watchdog('contact', '%sender-name (@sender-from) sent %recipient-name an e-mail.', array(
        '%sender-name' => $sender->name,
        '@sender-from' => $sender->getEmail(),
        '%recipient-name' => $message->recipient->name,
      ));
    }

To reproduce the error: Send a contact mail, go the recent log messages page, you will see php warnings. If you have devel enabled, it will fail with a recoverable fatal error.

Proposed resolution

That should use getUsername() instead of getName().

For tests, make sure that dblog is enabled, and after the contact message is sent, go to admin/reports/dblog (make sure the user has the necessary permission), and verify that the correct message is shown.

Remaining tasks

Needs tests

User interface changes

API changes

CommentFileSizeAuthor
#32 smells_like_random_fail.png28.71 KBberdir
#28 interdiff-2223611-23.txt1.16 KBcs_shadow
#28 contact-php_notices_raised_due_to_incorrect_username-2223611-23.patch3.43 KBcs_shadow
#25 contact-php_notices_raised_due_to_incorrect_username-2223611-24.patch3.26 KBcs_shadow
#25 contact-php_notices_raised_due_to_incorrect_username-2223611-24.patch3.26 KBcs_shadow
#19 contact-php_notices_raised_due_to_incorrect_username-2223611-19.patch3.34 KBvisabhishek
#19 interdiff-2223611-15-19.txt1016 bytesvisabhishek
#16 contact-php_notices_raised_due_to_incorrect_username-2223611-15.patch3.14 KBACF
#15 contact-php_notices_raised_due_to_incorrect_username-2223611-15-test.patch1.95 KBACF
#11 contact-php_notices_raised_due_to_incorrect_username-2223611-11.patch2.98 KBdman
#10 contact-php_notices_raised_due_to_incorrect_username-2223611-10-TEST_ONLY.patch1.83 KBdman
#1 2223611.patch1.14 KBvintorezxxi

Comments

vintorezxxi’s picture

vintorezxxi commented
StatusFileSize
new 2223611.patch1.14 KB

Changed $sender->name, to $sender->getUsername(),
Were thinking about changing to $sender->name->value, but previous solution seems more convenient.

vintorezxxi’s picture

vintorezxxi commented
Status: Active » Needs review
vintorezxxi’s picture

vintorezxxi commented
Status: Needs review » Closed (fixed)
vintorezxxi’s picture

vintorezxxi commented
Status: Closed (fixed) » Needs review
leex’s picture

leex commented
Status: Needs review » Reviewed & tested by the community

Tested, works fine.

dman’s picture

dman commented
Issue summary: View changes

Update summary to : "needs tests" to complete this task. As in the OP, it's not ready until tests exist.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs work

Yep, which means it needs work.

Also, I think $message->recipient->name needs to be updated too.

jonathan_hunt’s picture

jonathan_hunt commented

Also tested, works for me. However tests are a mess: ContactSitewideTest gives 164 passes, 216 fails.

dman’s picture

dman commented

Yep. We were sprinting ( #Drupal8NZ) so I took the opportunity to do a short in-person introduction to what it means to write tests before batting it back immediately with a terse "Needs Work". We are up to speed with that now. In progress.

dman’s picture

dman commented
Status: Needs work » Needs review
StatusFileSize
new contact-php_notices_raised_due_to_incorrect_username-2223611-10-TEST_ONLY.patch1.83 KB

Following up from @vintorezxxi's work here...
Here is what a TEST that replicates the described error looks like.
This TEST ONLY patch is expected to return red, as it highlights the fact that : visiting the reports page after sending a message triggers a PHP warning.

dman’s picture

dman commented
StatusFileSize
new contact-php_notices_raised_due_to_incorrect_username-2223611-11.patch2.98 KB

And here is the re-roll of #1, with the patch included. This is expected to run green again, as it proves the problem, AND fixes it at the same time.

(@vintorezxxi : Note the patch naming conventions we looked at on Monday.)

dman’s picture

dman commented

In related testing here, I noticed that it's now legal to create a user with no email. For reasons.
Yet there is no check for this when sending a mail to that user through the contact form - which triggers an error.
If I can't find an existing issue for that, I guess I'll open a new one.

The last submitted patch, 10: contact-php_notices_raised_due_to_incorrect_username-2223611-10-TEST_ONLY.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
@@ -85,6 +85,13 @@ function testSendPersonalContactMessage() {
+    // Simply visiting the log page was enough to raise some warnings.
+    $this->drupalGet('/admin/reports/dblog');

The comment only makes sense in the context of this patch, as soon as it is committed, it is no longer clear what "was enough ..." refers to.

Instead I would add simple assertText() assertion and look for the message that we expect, including the correctly displayed username. Then we also have test coverage for what the code is actually doing, this would also pass if we would remove the watchdog() call completely.

Then add a comment that explains what we do ("Verify that the correct watchdog message has been logged." ?).

ACF’s picture

ACF commented
Status: Needs work » Needs review
StatusFileSize
new contact-php_notices_raised_due_to_incorrect_username-2223611-15-test.patch1.95 KB
ACF’s picture

ACF commented
StatusFileSize
new contact-php_notices_raised_due_to_incorrect_username-2223611-15.patch3.14 KB

Made the change to the test suggested. In the process I spotted that the recipient name wasn't being passed to watchdog so i've made that change.

The last submitted patch, 15: contact-php_notices_raised_due_to_incorrect_username-2223611-15-test.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
@@ -85,6 +85,13 @@ function testSendPersonalContactMessage() {
+    $this->assertText($this->web_user->getUsername() . ' (' . $this->web_user->getEmail() . ') sent ' . $this->contact_user->getUsername() . ' an e-mail.');

This should use String::format('@username (@mail)....', $placeholders). Looks good otherwise.

visabhishek’s picture

visabhishek commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2223611-15-19.txt1016 bytes
new contact-php_notices_raised_due_to_incorrect_username-2223611-19.patch3.34 KB

i am updating patch as per #18

Status: Needs review » Needs work

The last submitted patch, 19: contact-php_notices_raised_due_to_incorrect_username-2223611-19.patch, failed testing.

vastav’s picture

vastav commented
Status: Needs work » Needs review

19: contact-php_notices_raised_due_to_incorrect_username-2223611-19.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 19: contact-php_notices_raised_due_to_incorrect_username-2223611-19.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

That is not what I meant, you should pass a single string to String::format() with multiple placeholders.

ACF’s picture

ACF commented
Status: Needs work » Needs review
StatusFileSize
new contact-php_notices_raised_due_to_incorrect_username-2223611-23.patch3.43 KB
new interdiff-2223611-23.txt1.16 KB

Suggested change made to contact test.

cs_shadow’s picture

cs_shadow commented
StatusFileSize
new contact-php_notices_raised_due_to_incorrect_username-2223611-24.patch3.26 KB
new contact-php_notices_raised_due_to_incorrect_username-2223611-24.patch3.26 KB

Did changes to the patch in #16 as suggested in #18.

The last submitted patch, 25: contact-php_notices_raised_due_to_incorrect_username-2223611-24.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: contact-php_notices_raised_due_to_incorrect_username-2223611-24.patch, failed testing.

cs_shadow’s picture

cs_shadow commented
StatusFileSize
new contact-php_notices_raised_due_to_incorrect_username-2223611-23.patch3.43 KB
new interdiff-2223611-23.txt1.16 KB

Attaching files in #24 as they were overwritten somehow since I was also updating at the same time.

cs_shadow’s picture

cs_shadow commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Looks good, thanks all.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: contact-php_notices_raised_due_to_incorrect_username-2223611-23.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new smells_like_random_fail.png28.71 KB

Yeah right testbot. looking for current timestamp fail *has* to be a random fail :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

28: contact-php_notices_raised_due_to_incorrect_username-2223611-23.patch queued for re-testing.

dman’s picture

dman commented

@Berdir - could be worse, over here today is a test that asserts that 4 things really have been sorted 'randomly'. :-?

  • Commit 1c11d16 on 8.x by alexpott: 
    Issue #2223611 by cs_shadow, ACF, dman, visabhishek, vintorezxxi:...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 1c11d16 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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