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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
3.28 KB
1.67 KB

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.

Gábor Hojtsy’s picture

Issue tags: +Regression
andypost’s picture

Gábor Hojtsy’s picture

Issue tags: +Regression
Gábor Hojtsy’s picture

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.

andypost’s picture

  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

Gábor Hojtsy’s picture

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

andypost’s picture

+++ 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

Gábor Hojtsy’s picture

With checkplain :)

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

#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.

tim.plunkett’s picture

+++ 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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
1.67 KB

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? :)

andypost’s picture

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()

Gábor Hojtsy’s picture

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.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Now it's amazing ;)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

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