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
#14 2102441-14.patch2.06 KBsidharthap
#12 2102441-12.patch3 KBsidharthap
#10 2102441-10.patch2.99 KBrteijeiro
#8 drupal-remove_drupal_set_title_in_config_module-2102441-8.patch2.99 KBInternetDevels
#5 drupal8.config-module.2102441-5.patch2.67 KB-enzo-
#1 2102441-config-title-1.patch4.14 KBsidharthap
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap commented
Status: Active » Needs review
FileSize
2102441-config-title-1.patch4.14 KB

Initial patch.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

Thanks for working on this one @sidharthap. one minor space issue. Other than that patch looks great. Can you make sure it works after this patch applied & cc all - please?

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -128,4 +124,15 @@ public function diff($config_file) {
+  ¶
...
+  ¶

Empty space.

Status: Needs review » Needs work

The last submitted patch, 2102441-config-title-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
  1. +++ b/core/modules/config/config.module
@@ -62,33 +62,28 @@ function config_file_download($uri) {
-    'title' => 'Configuration management',
...
-    'title' => 'Configuration file diff',

    These need to be left in until #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit goes in 

  2. +++ b/core/modules/config/config.module
@@ -62,33 +62,28 @@ function config_file_download($uri) {
-    'title' => 'Synchronize',
...
-    'title' => 'Export',
...
-    'title' => 'Import',

    These are blocked by #2102125: Big Local Task Conversion 

  3. +++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -128,4 +124,15 @@ public function diff($config_file) {
+/**
...
+ }
+  ¶

    Trailing whitespace

-enzo-’s picture

-enzo- CreditAttribution: -enzo- commented
FileSize
drupal8.config-module.2102441-5.patch2.67 KB

I did a re-roll to remove the title menu deletion based in @tim.plunkett recommendation

InternetDevels’s picture

InternetDevels CreditAttribution: InternetDevels commented
Status: Needs work » Needs review

Changed status.

Status: Needs review » Needs work

The last submitted patch, drupal8.config-module.2102441-5.patch, failed testing.

InternetDevels’s picture

InternetDevels CreditAttribution: InternetDevels commented
Status: Needs work » Needs review
FileSize
drupal-remove_drupal_set_title_in_config_module-2102441-8.patch2.99 KB

New one.

Status: Needs review » Needs work

The last submitted patch, drupal-remove_drupal_set_title_in_config_module-2102441-8.patch, failed testing.

rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Status: Needs work » Needs review
FileSize
2102441-10.patch2.99 KB

Wrong patch name. It has a whitespace at the beginning O_o

Status: Needs review » Needs work

The last submitted patch, 2102441-10.patch, failed testing.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap commented
Status: Needs work » Needs review
FileSize
2102441-12.patch3 KB

It should pass.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work 
+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -103,6 +100,7 @@ public function diff($config_file) {
+    $build['#title'] = format_string('View changes of @config_file', array('@config_file' => $config_file));

We should really use a translation here

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigExportForm.php
@@ -25,6 +25,7 @@ public function getFormId() {
+    $form['#title'] = $this->t('Export');

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigImportForm.php
@@ -54,6 +54,7 @@ public function getFormId() {
+    $form['#title'] = $this->t('Import');

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
@@ -130,6 +130,7 @@ public function getFormId() {
+    $form['#title'] = $this->t('Synchronize');
     $form['actions'] = array('#type' => 'actions');

These once should be moved to the routing file instead

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap commented
FileSize
2102441-14.patch2.06 KB

Thank you @dawehner
Corrected #13 patch.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap commented
Status: Needs work » Needs review

Thank you @dawehner
Corrected #13 patch.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Thank you

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thank!

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Issue summary: View changes
Parent issue: » #1830588: [META] remove drupal_set_title() and drupal_get_title()

Status: Fixed » Closed (fixed)

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