Add typehints to ImageInterface and adjust the tests

Created on 2 March 2023, about 2 years ago

Problem/Motivation

core/lib/Drupal/Core/Image/Image.php and core/lib/Drupal/Core/Image/ImageInterface.php are missing return typehints.

Steps to reproduce

If return type hint is added to any of existing actions like apply(), convert(), crop(), desaturate(), ... the unit tests will fail with error message:

    1) Drupal\Tests\Core\Image\ImageTest::testRotate
    TypeError: Drupal\Core\Image\Image::rotate(): Return value must be of type
    bool, array returned

Proposed resolution

Add return type hints, adjust the tests.

📌 Task
Status

Active

Version

10.1

Component
Image system 

Last updated 1 day ago

Created by

🇫🇮Finland sokru

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @sokru
  • First commit to issue fork.
  • @_shy opened merge request.
  • Status changed to Needs review over 1 year ago
  • 🇺🇦Ukraine _shy Ukraine, Lutsk 🇺🇦

    Seems like all tests are passed.

  • Status changed to RTBC over 1 year ago
  • 🇺🇸United States smustgrave

    This looks good but my concern is for backwards compatibility. But may not be an issue.

  • Status changed to Needs work over 1 year ago
  • 🇬🇧United Kingdom longwave UK

    Yeah, there are definitely BC concerns here - if someone else has implemented ImageInterface without return types, adding the return types here will break their code. We don't have a good plan for solving this at present except by perhaps using Symfony's debug class loader to notify downstream users of our code.

  • 🇺🇦Ukraine _shy Ukraine, Lutsk 🇺🇦

    @longwave,

    Thanks for the review. In this case, does it make sense to postpone this task for now and add to-dos or comments for the functions to add typehints later on? During some major changes later.

    Because, for now, it looks like it's hard to continue with that task.

  • 🇺🇦Ukraine _shy Ukraine, Lutsk 🇺🇦

    I moved the task to the "Active" for further discussion about what to do here.

  • 🇬🇧United Kingdom longwave UK

    The top level meta for this is 🌱 [Meta] Implement strict typing in existing code Active , adding this as a child and postponing. It is likely that we will do wider changes than interface-by-interface, but until we decide this issue can stay postponed.

Production build 0.71.5 2024