Subtask of #1830588: [META] remove drupal_set_title() and drupal_get_title()

Problem/Motivation

Using procedural drupal_set_title() inside controller class is not encouraged.

Proposed resolution

Replace drupal_set_title() with #title in page return array.

Remaining tasks

Issue patch

User interface changes

Refer parent issue at #1830588: [META] remove drupal_set_title() and drupal_get_title()

API changes

Refer parent issue at #1830588: [META] remove drupal_set_title() and drupal_get_title()

CommentFileSizeAuthor
#17 interdiff-2102479-7-16.txt1.04 KBdipen chaudhary
#17 2102479-ajax-test-title-16.patch2.1 KBdipen chaudhary
#15 interdiff-2102479-7-15.txt1.15 KBdipen chaudhary
#15 2102479-ajax-test-title-15.patch2.27 KBdipen chaudhary
#7 2102479-ajax-test-title-7.patch1.92 KBdipen chaudhary
#5 drupal-remove_drupal_set_title_in_system_ajax_test_module_controllers-2102479-5.patch1.91 KBinternetdevels
#3 drupal-remove_drupal_set_title_in_system_ajax_test_module_controllers-2102479-3.patch1.91 KBinternetdevels
#1 drupal-remove_drupal_set_title_in_system_ajax_test module_controllers-2102479-1.patch1.91 KBinternetdevels

Comments

internetdevels’s picture

internetdevels commented
Status: Active » Needs review
StatusFileSize
new drupal-remove_drupal_set_title_in_system_ajax_test module_controllers-2102479-1.patch1.91 KB

Status: Needs review » Needs work

The last submitted patch, drupal-remove_drupal_set_title_in_system_ajax_test module_controllers-2102479-1.patch, failed testing.

internetdevels’s picture

internetdevels commented
Status: Needs work » Needs review
StatusFileSize
new drupal-remove_drupal_set_title_in_system_ajax_test_module_controllers-2102479-3.patch1.91 KB

Status: Needs review » Needs work

The last submitted patch, drupal-remove_drupal_set_title_in_system_ajax_test_module_controllers-2102479-3.patch, failed testing.

internetdevels’s picture

internetdevels commented
Status: Needs work » Needs review
StatusFileSize
new drupal-remove_drupal_set_title_in_system_ajax_test_module_controllers-2102479-5.patch1.91 KB

New one.

The last submitted patch, drupal-remove_drupal_set_title_in_system_ajax_test_module_controllers-2102479-5.patch, failed testing.

dipen chaudhary’s picture

dipen chaudhary commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 2102479-ajax-test-title-7.patch1.92 KB

Order of _title matters, tested locally.

Status: Needs review » Needs work

The last submitted patch, 7: 2102479-ajax-test-title-7.patch, failed testing.

dipen chaudhary’s picture

dipen chaudhary commented
Status: Needs work » Needs review

Not sure why this failed on the testbot, local test shows that it returns the correct title in the command array. Re-queuing to see if its pertinent.

7: 2102479-ajax-test-title-7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2102479-ajax-test-title-7.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
commented

@dipen chaudhary, I do remember we used to face this issue with test server before. as we can see both strings are same, but test fails on assertEqual(). Not sure what we did to make it work.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Needs work » Needs review

7: 2102479-ajax-test-title-7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2102479-ajax-test-title-7.patch, failed testing.

dipen chaudhary’s picture

dipen chaudhary commented

@Vijay,

The testbot results shows command array to have the title "Home" and not "Ajax Form contents" for some reason, whereas local test shows it to be correct :(

What do we do in such scenarios where there is discrepancy between local test and testbot? My local environment is uptodate.

dipen chaudhary’s picture

dipen chaudhary commented
StatusFileSize
new 2102479-ajax-test-title-15.patch2.27 KB
new interdiff-2102479-7-15.txt1.15 KB

After some investigation and looking at some other code found out --

$form['#title'] = t('');

was not the right way to set modal dialog title, _title in routing.yml helped.

vijaycs85’s picture

vijaycs85
Location London, UK
commented

Yep, if it is static title and not changing inside callback, we can set it in routing file.

+++ b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/AjaxTestForm.php
@@ -38,7 +37,7 @@
+    ¶

Minor: Empty spaces or tab character.

dipen chaudhary’s picture

dipen chaudhary commented
Status: Needs work » Needs review
StatusFileSize
new 2102479-ajax-test-title-16.patch2.1 KB
new interdiff-2102479-7-16.txt1.04 KB

Removed some unnecessary whitespaces.

The last submitted patch, 17: 2102479-ajax-test-title-16.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

17: 2102479-ajax-test-title-16.patch queued for re-testing.

The last submitted patch, 17: 2102479-ajax-test-title-16.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

17: 2102479-ajax-test-title-16.patch queued for re-testing.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Needs review » Reviewed & tested by the community

Good to go. still applies with code base...

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical
dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Looks good! Committed to 8.x. Thanks.

Status: Fixed » Closed (fixed)

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