Problem/Motivation

TitleResolver service's getTitle() should return consistent type of object. If we look at the getTitle() implementation then we found that there is if and elseif statement in which it returns different type of objects.

Steps to reproduce

In the current route object check the default values by $route->getDefault(), if we have default _title_callback then it returns object of different type but if we have default _title only then it returns object of type TranslatableMarkup

Proposed resolution

Inside the getTitle() of TitleResolver service, for both the cases it should return object of same type.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hemuvyas97 created an issue. See original summary.

Hemuvyas97’s picture

Assigned: Hemuvyas97 » Unassigned
Status: Active » Needs review
FileSize
716 bytes
Hemuvyas97’s picture

Wim Leers’s picture

Interface docs:

   * @return array|string|null
   *   The title for the route.
   */
  public function getTitle(Request $request, Route $route);

string as a typehint tends to imply that as long as a Stringable object is returned, it's fine. But https://www.php.net/manual/en/class.stringable.php only exists since PHP 8, which is why the interface docs indicate just string.

In Drupal 10, we'll be able to make this better though!

What do you think about this as an alternative? 😊

cilefen’s picture

Why is this a support request?

Wim Leers’s picture

Category: Support request » Task
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense to me. And my ide will thank you!

Chi’s picture

Will there be any problems if we add native PHP typehints to the method?

Wim Leers’s picture

@Chi If we don't have to care about PHP 7 support, then we indeed could … if we did a BC break: https://3v4l.org/WYop7

Code in that experiment:

<?php

class Request {}
class Route {}
// Before: docs only: array|string|\Stringable|null
/*
interface FooInterface {
    public function getTitle(Request $request, Route $route);
}
*/
// If we use native typehints instead
interface FooInterface {
    public function getTitle(Request $request, Route $route): null|array|string|\Stringable;
}

// This is an implementation written against the original, i.e. with no return types specified. We should not break it.
class SomeFoo implements FooInterface {
    public function getTitle(Request $request, Route $route) {
        return 'yay';
    }
}

$test = new SomeFoo();
var_dump($test->getTitle(new Request(), new Route()));

… so AFAICT, we unfortunately can't do that.

Chi’s picture

@Wim Leers, aren't BC breaks still acceptable for Drupal 10?

Wim Leers’s picture

Drupal 10 is in beta, so … no. The API is frozen.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 3310346-4.patch, failed testing. View results

smustgrave’s picture

Status: Needs work » Reviewed & tested by the community

Moving back to RTBC seemed ot be a random failure.

alexpott’s picture

Title: Title Resolver service should have consistent return object » Improve Title Resolver return documentation

Fixing the issue title to match what we're doing

alexpott’s picture

Version: 9.4.x-dev » 10.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 6d34fc2b74 to 10.1.x and efeb6f0ec2 to 10.0.x. Thanks!

  • alexpott committed 6d34fc2 on 10.1.x
    Issue #3310346 by Hemuvyas97, Wim Leers: Improve Title Resolver return...

  • alexpott committed efeb6f0 on 10.0.x
    Issue #3310346 by Hemuvyas97, Wim Leers: Improve Title Resolver return...

Status: Fixed » Closed (fixed)

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