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

Files: 
CommentFileSizeAuthor
#30 2102369-block-title-30.patch5.56 KBJeroenT
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]
#28 2102369-block-title-28.patch5.58 KBJeroenT
PASSED: [[SimpleTest]]: [MySQL] 58,047 pass(es).
[ View ]
#26 interdiff.txt1.51 KBACF
#26 2102369-block-title-26.patch5.56 KBACF
PASSED: [[SimpleTest]]: [MySQL] 59,976 pass(es).
[ View ]
#22 2102369-block-title-22.patch5.55 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 58,691 pass(es).
[ View ]
#22 interdiff.txt2.03 KBrteijeiro
#18 2102369-block-title-18.patch5.58 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,798 pass(es).
[ View ]
#15 2102369-block-title-15.patch73.11 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2102369-block-title-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 2102369-diff-9-15.txt4.61 KBvijaycs85
#13 screenshot 2013-10-04 à 15.51.01.png64 KBJulienD
#9 drupal8.block-module.2102369-9.patch2.95 KB-enzo-
PASSED: [[SimpleTest]]: [MySQL] 58,915 pass(es).
[ View ]
#5 2102369-block-title-5.patch3.58 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 58,704 pass(es), 46 fail(s), and 13 exception(s).
[ View ]
#5 2102369-diff-2-5.txt1.5 KBvijaycs85
#2 2102369-block-title-2.patch3.57 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#2 interdiff.txt3.95 KBtim.plunkett
#1 2102369-block-title-1.patch5.18 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

vijaycs85’s picture

Status:Active» Needs review
StatusFileSize
new5.18 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Initial patch for block & custom block module. We still have 2 instance of drupal_set_title in these modules. Both of them on EntityFormContollers. Not very clear how to make them work.

1. CustomBlockTypeListController->render()
2. CustomBlockFormController->form().

Other than title change, injected t() - called on title.

tim.plunkett’s picture

StatusFileSize
new3.95 KB
new3.57 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

A lot of that isn't needed.

dawehner’s picture

+++ b/core/modules/block/custom_block/custom_block.routing.yml
@@ -16,6 +17,7 @@ custom_block.add_form:
+    _title_callback: 'Drupal\custom_block\Controller\CustomBlockController::geFormTitle'

ge FORM!

Status:Needs review» Needs work

The last submitted patch, 2102369-block-title-2.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.5 KB
new3.58 KB
FAILED: [[SimpleTest]]: [MySQL] 58,704 pass(es), 46 fail(s), and 13 exception(s).
[ View ]

Updated the method name here...

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Ha

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 2102369-block-title-5.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/block/custom_block/custom_block.module
@@ -99,11 +99,9 @@ function custom_block_menu() {
-    'title' => 'Add custom block',
     'route_name' => 'custom_block.add_page',
   );
   $items['block/add/%custom_block_type'] = array(
-    'title' => 'Add custom block',

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

-enzo-’s picture

StatusFileSize
new2.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,915 pass(es).
[ View ]

I re-roll the patch to remove the deletion of menu entries based in #8 by @tim.plunkett

tim.plunkett’s picture

Status:Needs work» Needs review

This looks good to me.

Status:Needs review» Needs work
Issue tags:-WSCCI Conversions

The last submitted patch, drupal8.block-module.2102369-9.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI Conversions

#9: drupal8.block-module.2102369-9.patch queued for re-testing.

JulienD’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new64 KB

The patch works fine. Blocks titles are still displayed in the menu link or on the admin block pages.

JulienD’s picture

Status:Reviewed & tested by the community» Needs work

As said in #1 there are 2 left drupal_set_title function.

  1. CustomBlockTypeListController->render()
  2. CustomBlockFormController->form().
vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new4.61 KB
new73.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2102369-block-title-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

sorry for missing them and thanks for review @JulienD. Here is a patch that fix them as well. I haven't tested it locally yet. Still submitting to get it validated by @tim.plunkett or @dawehner for the approach.

Status:Needs review» Needs work

The last submitted patch, 2102369-block-title-15.patch, failed testing.

JulienD’s picture

Hi @vijaycs85,

I think there is too much things in your patch (39 files changed, 428 insertions, 360 deletions).

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new5.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,798 pass(es).
[ View ]

Sorry wrong commit range. This should work.

Status:Needs review» Needs work
Issue tags:-WSCCI Conversions

The last submitted patch, 2102369-block-title-18.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI Conversions

#18: 2102369-block-title-18.patch queued for re-testing.

dawehner’s picture

  1. --- a/core/lib/Drupal/Core/Entity/EntityListController.php
    +++ b/core/lib/Drupal/Core/Entity/EntityListController.php

    @@ -249,4 +250,15 @@ public function setTranslationManager(TranslationInterface $translation_manager)
    +   * Returns the title of the form.

    It seems odd to talk about forms even we are on a listing.

  2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeListController.php
    @@ -51,9 +51,14 @@ public function buildRow(EntityInterface $entity) {
        * {@inheritdoc}
        */
       public function render() {
    -    // @todo Remove this once https://drupal.org/node/2032535 is in.
    -    drupal_set_title(t('Custom block types'));
         return parent::render();
       }

    If you remove the drupal_set_title() call all you have is return parent::render() so it can be dropped.

rteijeiro’s picture

StatusFileSize
new2.03 KB
new5.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,691 pass(es).
[ View ]

Re-rolled and applied @dawehner suggestions on #21.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

This seems fine for me now.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/block/custom_block/custom_block.routing.yml
@@ -16,6 +17,7 @@ custom_block.add_form:
+    _title_callback: 'Drupal\custom_block\Controller\CustomBlockController::getFormTitle'

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.php
@@ -98,10 +99,6 @@ public function add(Request $request) {
-    // @todo Remove this when https://drupal.org/node/1981644 is in.
-    drupal_set_title(t('Add %type custom block', array(
-      '%type' => $custom_block_type->label()
-    )), PASS_THROUGH);

@@ -114,4 +111,17 @@ public function addForm(CustomBlockTypeInterface $custom_block_type, Request $re
+  /**
+   * Provides the page title for this controller.
+   *
+   * @param \Drupal\custom_block\CustomBlockTypeInterface $custom_block_type
+   *   The custom block type being added.
+   *
+   * @return string
+   *   The page title.
+   */
+  public function getFormTitle(CustomBlockTypeInterface $custom_block_type) {

How about getAddFormTitle? The function doc block and name are not consistent. Also can't we just return the title on the form array?

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityListController.php
@@ -202,6 +202,7 @@ public function render() {
+      '#title' => $this->getTitle(),

@@ -249,4 +250,15 @@ public function setTranslationManager(TranslationInterface $translation_manager)
+  /**
+   * Returns the title of the page.
+   *
+   * @return string
+   *   A string title of the page.
+   *
+   */
+  protected function getTitle() {
+    return;
+  }

Also, this is way bigger than block module. Can we either reduce the scope or retitle the issue?

ACF’s picture

Title:Remove drupal_set_title in block module controllers» Remove drupal_set_title in custom block module controllers and entitylist controllers
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new5.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,976 pass(es).
[ View ]
new1.51 KB

Changed the title to getAddFormTitle. Can't add the page title directly to the form as the form is called both for /block/add and /block/add/{custom_block_type} but they have two different page titles and no way as far as I can see of distushing the two different times the form is called particularly if the custom_block_type is basic.

benjy’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Needs reroll.

JeroenT’s picture

Status:Needs work» Needs review
StatusFileSize
new5.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,047 pass(es).
[ View ]

Rerolled patch #26.

vijaycs85’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.php
@@ -113,5 +110,18 @@ public function addForm(CustomBlockTypeInterface $custom_block_type, Request $re
+  ¶

Minor: Empty space...

JeroenT’s picture

StatusFileSize
new5.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]

Woops. Fixed the whitespace.

Patch attached.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 30: 2102369-block-title-30.patch, failed testing.

JeroenT’s picture

Status:Needs work» Needs review

30: 2102369-block-title-30.patch queued for re-testing.

vijaycs85’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs reroll

This is good to go.

catch’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityListController.php
@@ -249,4 +250,15 @@ public function setTranslationManager(TranslationInterface $translation_manager)
+  protected function getTitle() {

Why add this on the controller but not the interface? It's only used internal to the class so maybe that's OK and we say this an internal implementation detail of the base class, but I don't think this was discussed. It could also be protected if it's not in the interface.

catch’s picture

Status:Reviewed & tested by the community» Needs review
tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

this an internal implementation detail of the base class

I don't think it was explicitly discussed, but I agree with this statement. It's fine as is.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Works for me. Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

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