Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
5.18 KB

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

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
FileSize
1.5 KB
3.58 KB

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

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
JulienD’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
64 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
FileSize
4.61 KB
73.11 KB

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
FileSize
5.58 KB

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

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
FileSize
5.56 KB
1.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
FileSize
5.58 KB

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

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.