Need for TrustedCallbackInterface not detected

[joachim-n]

I had the following class in a custom module in my Drupal 8.9 codebase. upgrade_status didn't detect that this needed to implement TrustedCallbackInterface for the lazy builder callback.

<?php

namespace Drupal\time_tracking\Controller;

use Drupal\Component\Datetime\TimeInterface;
use Drupal\Core\Datetime\DrupalDateTime;
use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\Form\FormState;
use Drupal\Core\Form\FormBuilderInterface;
use Drupal\Core\Session\AccountProxyInterface;
use Drupal\Core\StringTranslation\StringTranslationTrait;
use Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
 * Controller for the time tracking page.
 *
 * This allows the user to start and end time periods, and shows an overview of
 * time periods already created for the current day.
 *
 * The time tracking plugin specified in the route parameter adds functionality
 * to the page specific to its operation.
 */
class TimeTrackingController implements ContainerInjectionInterface {

  use StringTranslationTrait;

  /**
   * The entity type manager.
   *
   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   */
  protected $entityTypeManager;

  /**
   * The form builder service.
   *
   * @var \Drupal\Core\Form\FormBuilderInterface
   */
  protected $formBuilder;

  /**
   * The time service.
   *
   * @var \Drupal\Component\Datetime\TimeInterface
   */
  protected $datetimeTime;

  /**
   * The current active user.
   *
   * @var \Drupal\Core\Session\AccountProxyInterface
   */
  protected $currentUser;

  /**
   * Creates a TimeTrackingController instance.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   * @param \Drupal\Core\Form\FormBuilderInterface $form_builder
   *   The form builder service.
   * @param \Drupal\Component\Datetime\TimeInterface $datetime_time
   *   The time service.
   * @param \Drupal\Core\Session\AccountProxyInterface $current_user
   *   The current active user.
   */
  public function __construct(
    EntityTypeManagerInterface $entity_type_manager,
    FormBuilderInterface $form_builder,
    TimeInterface $datetime_time,
    AccountProxyInterface $current_user
  ) {
    $this->entityTypeManager = $entity_type_manager;
    $this->formBuilder = $form_builder;
    $this->datetimeTime = $datetime_time;
    $this->currentUser = $current_user;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('entity_type.manager'),
      $container->get('form_builder'),
      $container->get('datetime.time'),
      $container->get('current_user')
    );
  }

  /**
   * Callback for the time_tracking.time-tracking route.
   */
  public function content($type) {
    // Redirect anonymous users to log in.
    if ($this->currentUser->isAnonymous()) {
      $url = \Drupal::urlGenerator()->generate('user.login',['destination' => \Drupal::request()->getRequestUri()]);
      $response = new \Symfony\Component\HttpFoundation\RedirectResponse($url);
      return $response;
    }

    $build = [
      '#lazy_builder' => [
        static::class . '::contentLazyBuilder',
        [
          $type,
        ],
      ],
      '#create_placeholder' => TRUE,
    ];

    return $build;
  }

  /**
   * Lazy builder callback for the content of the tracking page.
   *
   * Most of the page is dependent on the current user, so not cacheable. It
   * doesn't make sense to use individual lazy builders, as the whole thing
   * needs to be alterable by the tracking type plugin.
   *
   * @param string $type
   *   The tracking type plugin ID.
   *
   * @return
   *   The render array.
   */
  public static function contentLazyBuilder($type) {
    $build = [];

    $time_tracker_type_plugin = \Drupal::service('plugin.manager.time_tracker_type')->createInstance($type);

    $time_period_storage = \Drupal::service('entity_type.manager')->getStorage('tracking_period');

    // Timeslots today view.
    $build['list'] = [
      '#type' => 'view',
      // Get the view name from the plugin.
      // TODO! Bug! The view can't be used for different types, as we don't pass
      // it an argument! In particular, the view specified by the 'simple'
      // plugin will return ALL entities!
      '#name' => $time_tracker_type_plugin->getViewName(),
      '#display_id' => 'default',
      '#arguments' => [],
    ];

    $build['current_period'] = [
      '#type' => 'fieldset',
      '#title' => t('Current period'),
    ];

    $request_timestamp = \Drupal::time()->getRequestTime();
    $request_datetime = DrupalDateTime::createFromTimestamp($request_timestamp);

    // Curent time period close.
    $current_period = $time_period_storage->loadCurrentPeriodForAccount($type, \Drupal::service('current_user'));
    if ($current_period) {
      // Warn for an old time period that's not been closed. I mean, you
      // probably didn't work all night, right???
      // The entity's value has to be forced into the current user's timezone,
      // as it is stored in UTC.
      if ($current_period->start->date->format('Y-m-d', ['timezone' => date_default_timezone_get()]) != $request_datetime->format('Y-m-d')) {
        $build['current_period']['old_period_notice'] = [
          '#markup' => t("The current period is older than today."),
        ];
      }

      // Hackily get us back into an instance of this class!
      $self = \Drupal::service('class_resolver')->getInstanceFromDefinition(static::class);
      $build['current_period']['close_form'] = $self->getEntityForm($current_period, 'close');
    }

    $build['current_period']['add_form'] = $time_tracker_type_plugin->getStartForm();

    $time_tracker_type_plugin->alterTrackingPageBuild($build);

    return $build;
  }

  /**
   * Gets the entity form for a given entity and operation.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   The entity.
   * @param string $operation
   *   The name of the operation. If there is no form class for this, then the
   *   'default' operation form class is used.
   *
   * @return array
   *   The form render array.
   */
  protected function getEntityForm(EntityInterface $entity, $operation) {
    $entity_type = $this->entityTypeManager->getDefinition('tracking_period');

    if (!$entity_type->getFormClass($operation)) {
      $operation = 'default';
    }

    $form_object = $this->entityTypeManager->getFormObject('tracking_period', $operation);
    $form_object->setEntity($entity);
    $form_state = new FormState();

    return $this->formBuilder->buildForm($form_object, $form_state);
  }

}

[mglaman]

Fix your callback to not use a string concatenation with static. There may be a core bug for this though. Try using self over static

[mglaman]

This should be documented already on Drupal.org for PHPStan. I think. PHPStan can't resolve static, especially when using concatenation this way for a callback.

[joachim-n]

Could it give a general warning about the '#lazy_builder' part at least, and flag it as something that may need checking?

I used static rather than self as I'd understood it was more desirable in case of a child class overriding.

[mglaman]

I'll have to revisit and see. I don't think this code can even know. static::class comes back as something, and it's in concat... I'll see. There are a few issues floating around.

It works fine if array but not concat string

[static::class, 'callBack']

But Drupal core assumes the callback is always a string, primarily a service.

[joachim-n]

What I meant is that the analysis could just spot an array key that has the value '#lazy_builder' and say 'Hey this might need checking, but we don't know for sure.'

[mglaman]

This should be doing that

https://github.com/mglaman/phpstan-drupal/blob/main/src/Rules/Drupal/RenderCallbackRule.php#L127-L137

[mglaman]

Okay, at computer. We have this fixture:

            '#pre_render' => [
                [self::class, 'preRenderCallback'],
                [static::class, 'preRenderCallback'],
                self::class . '::preRenderCallback',
                static::class . '::preRenderCallback',
                [$this, 'preRenderCallback'],
                static function(array $element): array {
                    return $element;
                }
            ]

Gives:

            __DIR__ . '/../../fixtures/drupal/modules/pre_render_callback_rule/src/RenderArrayWithPreRenderCallback.php',
            [
                [
                    "#pre_render value 'non-empty-string' at key '3' is invalid.",
                    19,
                    "Refactor concatenation of `static::class` with method name to an array callback: [static::class, 'preRenderCallback']"
                ]
            ]

So it catches #pre_render but not #lazy_builder it seems

[mglaman]

I wrote a test and pushed a PR: https://github.com/mglaman/phpstan-drupal/pull/426, locally this is working as expected.

[mglaman]

This should also be fixed by https://github.com/mglaman/phpstan-drupal/pull/436

[mglaman]

@joachim-n can you try this again with the latest release?