Merge pull request #102 from CollaboraOnline/phpstan

Phpstan

Author: brummbar

.github/workflows/ci.yaml

@@ -72,6 +72,13 @@ jobs:
         run: |
           docker-compose exec -T web ./vendor/bin/phpcs -s
 
+      - name: PhpStan
+        run: |
+          # PhpStan behaves slightly differently in PHP 8.1.
+          if [ ${{ matrix.php }} != 8.1 ]; then
+            docker-compose exec -T web ./vendor/bin/phpstan -v
+          fi
+
       - name: Drupal site install
         run: |
           docker-compose exec -T web ./vendor/bin/run drupal:site-install

collabora_online.install

@@ -26,6 +26,7 @@ function collabora_online_requirements(string $phase): array {
   }
 
   // The Collabora Online settings key is set.
+  /** @var array $cool_settings */
   $cool_settings = \Drupal::configFactory()->get('collabora_online.settings')->get('cool');
   if (
     empty($cool_settings['key_id']) ||

collabora_online.module

@@ -10,8 +10,8 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-use Drupal\collabora_online\Cool\CoolUtils;
 use Drupal\collabora_online\CollaboraUrl;
+use Drupal\collabora_online\Cool\CoolUtils;
 use Drupal\collabora_online\MediaHelperInterface;
 use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Access\AccessResultInterface;
@@ -32,8 +32,7 @@ use Drupal\media\MediaInterface;
  * If you change this method, clear theme registry and routing table
  * 'drush cc theme-registry' and 'drush cc router'.
  */
-function collabora_online_theme($existing, $type, $theme, $path) {
-
+function collabora_online_theme(): array {
   return [
     'collabora_online' => [
       'render element' => 'children',
@@ -80,16 +79,18 @@ function collabora_online_theme($existing, $type, $theme, $path) {
  * This is used to add the menu entry in the Media content to
  * open the viewer/editor directly.
  */
-function collabora_online_entity_operation(EntityInterface $entity) {
-  if (($entity->getEntityTypeId() != "media") ||
-    ($entity->getSource()->getPluginId() != "file")) {
+function collabora_online_entity_operation(EntityInterface $entity): array {
+  if ($entity->getEntityTypeId() !== 'media') {
     return [];
   }
 
   /** @var \Drupal\media\MediaInterface $media */
   $media = $entity;
 
-  if (!$media->access('preview in collabora')) {
+  if (
+    $media->getSource()->getPluginId() !== 'file' ||
+    !$media->access('preview in collabora')
+  ) {
     return [];
   }
 
@@ -115,7 +116,10 @@ function collabora_online_entity_operation(EntityInterface $entity) {
     ],
   ];
 
-  if (CoolUtils::canEdit($file) && $media->access('edit in collabora')) {
+  if (
+    CoolUtils::canEditMimeType($type) &&
+    $media->access('edit in collabora')
+  ) {
     $entries['collabora_online_edit'] = [
       'title' => t("Edit in Collabora Online"),
       'weight' => 50,
@@ -151,8 +155,8 @@ function collabora_online_media_access(MediaInterface $media, string $operation,
         $access_result = AccessResult::allowed();
       }
       else {
-        $access_result = AccessResult::neutral()
-          ->setReason("The user has the '$preview_own_permission' permission, but is not the owner of the media item.");
+        $access_result = AccessResult::neutral();
+        $access_result->setReason("The user has the '$preview_own_permission' permission, but is not the owner of the media item.");
       }
       return $access_result
         ->cachePerUser()
@@ -171,8 +175,8 @@ function collabora_online_media_access(MediaInterface $media, string $operation,
       // Use '==' because Drupal sometimes loads integers as strings.
       $is_owner = ($account->id() && $account->id() == $media->getOwnerId());
       if (!$is_owner) {
-        $access_result = AccessResult::neutral()
-          ->setReason("The user has the '$edit_own_permission' permission, but is not the owner of the media item.");
+        $access_result = AccessResult::neutral();
+        $access_result->setReason("The user has the '$edit_own_permission' permission, but is not the owner of the media item.");
       }
       else {
         $access_result = AccessResult::allowed();

collabora_online.post_update.php

@@ -17,6 +17,7 @@
  */
 function collabora_online_post_update_add_wopi_proof_setting(): void {
   $config = \Drupal::configFactory()->getEditable('collabora_online.settings');
+  /** @var array $cool_settings */
   $cool_settings = $config->get('cool') ?? [];
   $cool_settings['wopi_proof'] ??= TRUE;
   $config->set('cool', $cool_settings);

modules/collabora_online_group/src/Plugin/Group/RelationHandler/CollaboraAccessControl.php

@@ -16,6 +16,7 @@
 
 use Drupal\Core\Access\AccessResultInterface;
 use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Entity\EntityPublishedInterface;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\group\Plugin\Group\RelationHandler\AccessControlInterface;
 use Drupal\group\Plugin\Group\RelationHandler\AccessControlTrait;
@@ -45,10 +46,18 @@ public function entityAccess(EntityInterface $entity, $operation, AccountInterfa
     // Add support for unpublished operation: preview in collabora.
     $check_published = $operation === 'preview in collabora' && $this->implementsPublishedInterface;
 
-    if ($check_published && !$entity->isPublished()) {
-      $operation .= ' unpublished';
+    if ($check_published) {
+      // The $this->implementsPublishedInterface property indicates that
+      // entities of this type implement EntityPublishedInterface.
+      /* @see \Drupal\group\Plugin\Group\RelationHandlerDefault\AccessControl::entityAccess() */
+      assert($entity instanceof EntityPublishedInterface);
+
+      if (!$entity->isPublished()) {
+        $operation .= ' unpublished';
+      }
     }
 
+    assert($this->parent !== NULL);
     return $this->parent->entityAccess($entity, $operation, $account, $return_as_object);
   }
 

modules/collabora_online_group/src/Plugin/Group/RelationHandler/CollaboraPermissionProvider.php

@@ -25,6 +25,7 @@ class CollaboraPermissionProvider extends GroupMediaPermissionProvider {
    * {@inheritdoc}
    */
   public function buildPermissions(): array {
+    assert($this->parent !== NULL);
     $permissions = $this->parent->buildPermissions();
 
     /* @see \Drupal\group\Plugin\Group\RelationHandlerDefault\PermissionProvider::buildPermissions() */
@@ -77,6 +78,7 @@ public function getPermission($operation, $target, $scope = 'any'): bool|string
       }
     }
 
+    assert($this->parent !== NULL);
     return $this->parent->getPermission($operation, $target, $scope);
   }
 

modules/collabora_online_group/tests/src/Traits/GroupRelationTrait.php

@@ -45,9 +45,11 @@ protected function createPluginRelation(GroupTypeInterface $group_type, string $
       $entity_type_id = 'group_content_type';
     }
 
-    $entity = $this->entityTypeManager()
-      ->getStorage($entity_type_id)
-      ->createFromPlugin($group_type, $plugin_id, $values);
+    $storage = $this->entityTypeManager()->getStorage($entity_type_id);
+    // The storage has a different interface in group v1 than in group v2.
+    // Both of them have a ->createFromPlugin() method.
+    // @phpstan-ignore method.notFound
+    $entity = $storage->createFromPlugin($group_type, $plugin_id, $values);
     $entity->save();
 
     return $entity;

phpstan.neon

@@ -0,0 +1,16 @@
+parameters:
+  level: 9
+  treatPhpDocTypesAsCertain: false
+  paths:
+    - .
+  excludePaths:
+    # For now do not check tests.
+    - tests
+    - modules/collabora_online_group/tests
+    - vendor
+    - web
+  ignoreErrors:
+    # Allow 'new static()', which is common and accepted in Drupal.
+    - "#^Unsafe usage of new static\\(\\).$#"
+    # Allow to omit `<mixed>` as iterable or generic type qualifier.
+    - identifier: missingType.iterableValue

src/Access/WopiProofAccessCheck.php

@@ -61,7 +61,7 @@ public function __construct(
    */
   public function access(Request $request): AccessResultInterface {
     $config = $this->configFactory->get('collabora_online.settings');
-    if (!($config->get('cool')['wopi_proof'] ?? TRUE)) {
+    if (!($config->get('cool.wopi_proof') ?? TRUE)) {
       return AccessResult::allowed()
         ->addCacheableDependency($config);
     }
@@ -176,7 +176,7 @@ protected function getSubject(Request $request): string {
     // This class is not responsible for checking the expiration, but it still
     // needs the WOPI timestamp to build the message for the signature.
     $timestamp_ticks = $request->headers->get('X-WOPI-Timestamp');
-    $token = $request->query->get('access_token', '');
+    $token = (string) $request->query->get('access_token', '');
     $url = $request->getUri();
     return sprintf(
       '%s%s%s%s%s%s',

src/CollaboraMediaPermissions.php

@@ -44,7 +44,7 @@ public function __construct(
   /**
    * {@inheritdoc}
    */
-  public static function create(ContainerInterface $container) {
+  public static function create(ContainerInterface $container): static {
     return new static($container->get('entity_type.manager'));
   }
 

src/Controller/ViewerController.php

@@ -143,6 +143,7 @@ public function editor(MediaInterface $media, Request $request, $edit = FALSE):
    *   The key to use by Collabora is empty or not configured.
    */
   protected function getViewerRender(MediaInterface $media, string $wopi_client, bool $can_write): array {
+    /** @var array $cool_settings */
     $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool');
 
     if (empty($cool_settings['wopi_base'])) {
@@ -187,8 +188,8 @@ protected function getViewerRender(MediaInterface $media, string $wopi_client, b
    *   Expiration timestamp in seconds, with millisecond accuracy.
    */
   protected function getExpireTimestamp(): float {
-    $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool');
-    $ttl_seconds = $cool_settings['access_token_ttl'] ?? 0;
+    $ttl_seconds = $this->configFactory->get('collabora_online.settings')
+      ->get('cool.access_token_ttl') ?? 0;
     // Set a fallback of 24 hours.
     $ttl_seconds = $ttl_seconds ?: 86400;
 

src/Controller/WopiController.php

@@ -75,6 +75,7 @@ public function __construct(
    *   The response with file contents.
    */
   protected function wopiCheckFileInfo(FileInterface $file, UserInterface $user, bool $can_write): Response {
+    assert($file->getChangedTime() !== NULL);
     $response_data = [
       'BaseFileName' => $file->getFilename(),
       'Size' => $file->getSize(),
@@ -88,9 +89,14 @@ protected function wopiCheckFileInfo(FileInterface $file, UserInterface $user, b
       'SupportsRename' => FALSE,
     ];
 
-    $user_picture = $user->user_picture?->entity;
-    if ($user_picture) {
-      $response_data['UserExtraInfo']['avatar'] = $this->fileUrlGenerator->generateAbsoluteString($user_picture->getFileUri());
+    if ($user->hasField('user_picture')) {
+      $user_picture = $user->get('user_picture')->entity;
+      if (
+        $user_picture instanceof FileInterface &&
+        $user_picture->getFileUri() !== NULL
+      ) {
+        $response_data['UserExtraInfo']['avatar'] = $this->fileUrlGenerator->generateAbsoluteString($user_picture->getFileUri());
+      }
     }
 
     return new JsonResponse(
@@ -112,6 +118,7 @@ protected function wopiCheckFileInfo(FileInterface $file, UserInterface $user, b
    * @see \Drupal\system\FileDownloadController::download()
    */
   protected function wopiGetFile(FileInterface $file): Response {
+    assert($file->getFileUri() !== NULL);
     if (!is_file($file->getFileUri())) {
       throw new NotFoundHttpException('The file is missing in the file system.');
     }
@@ -159,6 +166,7 @@ protected function wopiPutFile(MediaInterface $media, FileInterface $file, UserI
       $request_time - $file->getCreatedTime() <= $new_file_interval
     ) {
       // Replace file with new content.
+      assert($file->getFileUri() !== NULL);
       $this->fileSystem->saveData(
         $new_file_content,
         $file->getFileUri(),
@@ -182,6 +190,7 @@ protected function wopiPutFile(MediaInterface $media, FileInterface $file, UserI
         ],
       );
 
+      assert($file->getChangedTime() !== NULL);
       return new JsonResponse(
         [
           'LastModifiedTime' => DateTimeHelper::format($file->getChangedTime()),
@@ -216,6 +225,7 @@ protected function wopiPutFile(MediaInterface $media, FileInterface $file, UserI
       ],
     );
 
+    assert($new_file->getChangedTime() !== NULL);
     return new JsonResponse(
       [
         'LastModifiedTime' => DateTimeHelper::format($new_file->getChangedTime()),
@@ -238,6 +248,7 @@ protected function wopiPutFile(MediaInterface $media, FileInterface $file, UserI
    *   This may have a different uri, but will have the same filename.
    */
   protected function createNewFileEntity(FileInterface $file, string $new_file_content): FileInterface {
+    assert($file->getFileUri() !== NULL);
     // The current file uri may have a number suffix like "_0".
     // For the new file uri, start with the clean file name, to avoid repeated
     // suffixes like "_0_0_0".
@@ -250,10 +261,12 @@ protected function createNewFileEntity(FileInterface $file, string $new_file_con
       FileExists::Rename,
     );
 
-    /** @var \Drupal\file\FileInterface|null $new_file */
+    /** @var \Drupal\file\FileInterface $new_file */
     $new_file = $this->entityTypeManager->getStorage('file')->create([
       'uri' => $new_file_uri,
     ]);
+    // Parameter and return docs on entity owner methods are inconsistent.
+    // @phpstan-ignore argument.type
     $new_file->setOwnerId($file->getOwnerId());
     // Preserve the original file name, no matter the uri was renamed.
     $new_file->setFilename($file->getFilename());
@@ -285,7 +298,10 @@ protected function checkSaveTimestampConflict(Request $request, MediaInterface $
       return NULL;
     }
     $wopi_datetime = \DateTimeImmutable::createFromFormat(\DateTimeInterface::ATOM, $wopi_time_atom);
-    $file_datetime = \DateTimeImmutable::createFromFormat('U', $file->getChangedTime());
+    $file_datetime = \DateTimeImmutable::createFromFormat('U', (string) $file->getChangedTime());
+
+    assert($wopi_datetime !== FALSE);
+    assert($file_datetime !== FALSE);
 
     if ($wopi_datetime == $file_datetime) {
       return NULL;
@@ -354,6 +370,10 @@ public function wopi(string $action, MediaInterface $media, Request $request): R
     if ($token === NULL) {
       throw new AccessDeniedHttpException('Missing access token.');
     }
+    if (!is_string($token)) {
+      // A malformed request could have a non-string value for access_token.
+      throw new AccessDeniedHttpException(sprintf('Expected a string access token, found %s.', gettype($token)));
+    }
     $jwt_payload = $this->verifyTokenForMedia($token, $media);
 
     /** @var \Drupal\user\UserInterface|null $user */

src/Cool/CoolUtils.php

@@ -12,8 +12,6 @@
 
 namespace Drupal\collabora_online\Cool;
 
-use Drupal\file\Entity\File;
-
 /**
  * Class with various static methods.
  */
@@ -29,19 +27,16 @@ class CoolUtils {
   ];
 
   /**
-   * Determines if we can edit that media file.
-   *
-   * There are few types that Collabora Online only views.
+   * Determines if a MIME type is supported for editing.
    *
-   * @param \Drupal\file\Entity\File $file
-   *   File entity.
+   * @param string $mimetype
+   *   File MIME type.
    *
    * @return bool
-   *   TRUE if the file has a file type that is supported for editing.
-   *   FALSE if the file can only be opened as read-only.
+   *   TRUE if the MIME type is supported for editing.
+   *   FALSE if the MIME type can only be opened as read-only.
    */
-  public static function canEdit(File $file) {
-    $mimetype = $file->getMimeType();
+  public static function canEditMimeType(string $mimetype) {
     return !array_key_exists($mimetype, static::READ_ONLY);
   }
 

src/Discovery/DiscoveryFetcher.php

@@ -145,6 +145,8 @@ protected function loadDiscoveryXml(ImmutableConfig $config): string {
     $disable_checks = (bool) $config->get('cool.disable_cert_check');
     $discovery_url = $this->getDiscoveryUrl($config);
     try {
+      // The ->get() method is not part of the interface.
+      // @phpstan-ignore method.notFound
       $response = $this->httpClient->get($discovery_url, [
         RequestOptions::VERIFY => !$disable_checks,
       ]);

src/Form/ConfigForm.php

@@ -45,6 +45,7 @@ public function getEditableConfigNames(): array {
    */
   public function buildForm(array $form, FormStateInterface $form_state): array {
     $config = $this->config(static::SETTINGS);
+    /** @var array $cool_settings */
     $cool_settings = $config->get('cool');
 
     $form['server'] = [

src/Jwt/JwtTranscoder.php

@@ -39,8 +39,9 @@ public function __construct(
    * {@inheritdoc}
    */
   protected function getKey(): string {
-    $cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool');
-    $key_id = $cool_settings['key_id'] ?? '';
+    /** @var string $key_id */
+    $key_id = $this->configFactory->get('collabora_online.settings')
+      ->get('cool.key_id') ?? '';
     if (!$key_id) {
       throw new CollaboraJwtKeyException('No key was chosen for use in Collabora.');
     }