Updated: Comment 0

Problem/Motivation

Now that _title can be added to the route definition we realized that it does not work in connection with forms.
The problem is that it does not use the same code path.

Proposed resolution

For now add the _title to the separate codepath as well.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
765 bytes

.

dawehner’s picture

Issue tags: +WSCCI

.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Fixes the problems I had

webchick’s picture

Status: Reviewed & tested by the community » Needs work

No tests...

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
2.33 KB

I had added the tag :p

aspilicious’s picture

:s srry webchick. I talked about those with dawehner but totally forgot.

Dawehner it's the route one that will fail not the one build with the render array.

dawehner’s picture

Oh, this is maybe just broken on old menu router items? Does your route implement hook_menu and page callback?

pwolanin’s picture

per #2032535: Resolve 'title' using the route and render array we shoudl also have _title_callback. Also, the code was in the wrong section of the conditional.

however, parallel work is hapenning on the vire controller yu Crell, so postponing any test addition

dawehner’s picture

Sorry but the title callback is out of scope of this issue, let's do one step after the other.

dawehner’s picture

FileSize
2.79 KB

.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +blocker

Looks great and blocks converting things to remove drupal_set_title() from core (and contribs). Eg. I ran into this with #2070055: drupal_set_title() is deprecated. Agreed we don't need to support title callback as core does not do it either.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.8 KB

The problem is that the title issue was not tested with overlay enabled. I think we have to add an explicit test for that.

dawehner’s picture

FileSize
6.3 KB
1.31 KB

Adding a test for overlay to show the failure.

YesCT’s picture

Issue tags: -Needs tests

I think it has tests now. Any others?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for covering overlay. Looks good for me :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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