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
#9 2102449-follow-up-9.patch707 bytesamateescu
#1 2102449-2.patch1.8 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Active » Needs review
FileSize
2102449-2.patch1.8 KB
vijaycs85’s picture

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

All good, except the minor note below:

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.php
@@ -69,10 +69,11 @@ public function buildForm(array $form, array &$form_state, FieldInstanceInterfac
       '%bundle' => $bundles[$entity_type][$bundle]['label'],

Not sure how are we going to handle PASS_THROUGH in D8.

RTBC from my side.

swentel’s picture

swentel CreditAttribution: swentel commented

Well, we don't want check_plain here, so we're good. I guess if we need it, we'll have to call check_plain() again.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Closed (fixed) » Needs work 
+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldEditForm.php
@@ -92,8 +92,6 @@ public function buildForm(array $form, array &$form_state, FieldInstanceInterfac
-    drupal_set_title($this->instance->label());
-

Pretty sure this change was wrong. I.e. the title needs to be added back somewhere else. This currently only works because the hook_menu() menu item has a 'title' and if '#title' is not set and the title resolver doesn't return anything either, drupal_get_title() is called which checks the menu item title. I guess simply adding $form['#title'] would be the easiest.

swentel’s picture

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

I don't see what's wrong with it for now - there's a 'title callback' => 'entity_page_label' in hook_menu() - that's just fine no ?
Having to add '#title' everywhere just sounds plain ridiculous.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Well hook_menu() is only for menu links now. That should not be relied on for the actual page output. The fact that it currently works is only an artefact of legacy code. I'm not going to judge whether that is ridiculous or not, but that's simply how things are at the moment. Unless I'm completely missing something, needless to say.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Issue summary: View changes
FileSize
2102449-follow-up-9.patch707 bytes

@tstoeckler is right, we need a $form['#title'] because that hook_menu() entry that has a title callback is going away soon in #2111823: Convert field_ui / Entity local tasks to YAML definitions.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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