Problem/Motivation

Before: the contact category title you set was used as the page title for the contact form.
Currently: not anymore.

This now fails some of our fine tests in https://drupal.org/project/config_translation, so we cannot move forward with building out that module and ensure tests pass. Therefore marked blocker.

Proposed resolution

Make the label appear as the title again.

Files: 
CommentFileSizeAuthor
#20 contact-title-fix-20-tests-only.patch1.66 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,348 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#20 contact-title-fix-20.patch3.08 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,344 pass(es).
[ View ]
#18 contact-title-fix-18-tests-only.patch1.67 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,220 pass(es), 40 fail(s), and 24 exception(s).
[ View ]
#18 contact-title-fix-18.patch3.08 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,713 pass(es).
[ View ]
#11 contact-title-fix-10-tests-only.patch1.67 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,299 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#11 contact-title-fix-10.patch3.59 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,627 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#1 contact-title-fix-test-only.patch1.67 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 59,565 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#1 contact-title-fix.patch3.28 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,962 pass(es), 63 fail(s), and 55 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,962 pass(es), 63 fail(s), and 55 exception(s).
[ View ]
new1.67 KB
FAILED: [[SimpleTest]]: [MySQL] 59,565 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Here are quick untested fixes. The problem is #2089635: Convert non-test non-form page callbacks to routes and controllers and #2032535: Resolve 'title' using the route and render array in combination broke this because now the title is provided in the route, so the title callback is not considered, and the fixed title is used at all times. We should use #title in the page callbacks. Also made that change for the personal contact form.

Issue tags:+regression

Issue tags:+regression

Feel free to incorporate this change there, but this fails config_translation tests and therefore blocks any chance that could land in core, so I prefer not to depend on a patch going on since March. :/ Sorry. Trying to be agile here.

Status:Needs review» Needs work

The last submitted patch, contact-title-fix.patch, failed testing.

  1. +++ b/core/modules/contact/contact.pages.inc
    @@ -53,7 +53,9 @@ function contact_site_page(Category $category = NULL) {
    +  $form['#title'] = $entity->label();
    +++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.php
    @@ -126,6 +126,9 @@ function testSiteWideContact() {
    +    $this->drupalGet('contact/' . $id);
    +    $this->assertText($label);

    check_plain()?

  2. +++ b/core/modules/contact/contact.pages.inc
    @@ -79,13 +81,13 @@ function contact_personal_page($recipient) {
    -  drupal_set_title(t('Contact @username', array('@username' => user_format_name($recipient))), PASS_THROUGH);
    ...
    +  $form['#title'] = t('Contact @username', array('@username' => user_format_name($recipient)));
    +++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
    @@ -94,6 +94,8 @@ function testPersonalContactAccess() {
    +    $this->assertText(t('Contact @username', array('@username' => user_format_name($this->web_user))));

    PASS_THROUGH was lost

Right, how do you do passthrough with #title?! I'd have done it if it would be evident to me :D

+++ b/core/modules/contact/contact.pages.inc
@@ -53,7 +53,9 @@ function contact_site_page(Category $category = NULL) {
     'category' => $category->id(),
...
+  $form['#title'] = $entity->label();

$form['#title'] = String::checkPlain($category->label());
//not $entity

StatusFileSize
new3.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,627 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.67 KB
FAILED: [[SimpleTest]]: [MySQL] 58,299 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

With checkplain :)

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:-regression, -D8MI, -sprint, -language-config, -blocker

The last submitted patch, contact-title-fix-10.patch, failed testing.

Status:Needs work» Needs review

#11: contact-title-fix-10.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+regression, +D8MI, +sprint, +language-config, +blocker

The last submitted patch, contact-title-fix-10.patch, failed testing.

+++ b/core/modules/contact/contact.module
@@ -86,8 +86,6 @@ function contact_menu() {
-    'title callback' => 'entity_page_label',
-    'title arguments' => array(1),

You still need this for the breadcrumb, for now.

Status:Needs work» Needs review
StatusFileSize
new3.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,713 pass(es).
[ View ]
new1.67 KB
FAILED: [[SimpleTest]]: [MySQL] 58,220 pass(es), 40 fail(s), and 24 exception(s).
[ View ]

Updated!

- Removed the .module hunk as pointed out by @tim.plunkett this is needed for the breadcrumb (see #17)
- The personal page test was checking the wrong username, we should check the admin user who's contact form is loaded.
- Changed the assert in the personal test to raw since we escape the username with @ already

This passes fine for me :) Any other concerns? :)

Added the changes to #1938390-108: Convert contact_site_page and contact_person_page to a new-style Controller

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
@@ -94,6 +94,8 @@ function testPersonalContactAccess() {
+    $this->assertRaw(t('Contact @username', array('@username' => user_format_name($this->admin_user))));

please do not use deprecated functions - should be $this->admin_user->getUsername()

StatusFileSize
new3.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,344 pass(es).
[ View ]
new1.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,348 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

It is fascinating how much we can refine 10 lines of code. Amazing.

I of course used user_format_name() because the original code used that too so I just copy-pasted that out of the drupal_set_title() (which was not even broken, so just wanted to double up to fix that in this regression while I was at it). Updated that in code and test.

Status:Needs review» Reviewed & tested by the community

Now it's amazing ;)

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Issue tags:-sprint

Yay, thanks!

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