Problem/Motivation

The module was analysed by PHPStan at level 5, with broad ignoreErrors entries that silenced two whole classes of finding: Unsafe usage of new static and Drupal calls should be avoided in classes, use dependency injection instead. Those entries hid real problems rather than fixing them.

Applying drupal_extension_scaffold v4.18.0 replaced the configuration with level 7 and no such ignores, which exposed 37 errors. The largest group was static \Drupal::service() and static entity calls inside classes, which make the code harder to test and prevent the service container from being used properly.

Steps to reproduce

  1. Check out 5.0.x.
  2. Run make lint.
  3. PHPStan reports 37 errors, and PHPCS reports DrupalPractice.Objects.GlobalClass warnings for the same static calls.

Proposed resolution

Fix the underlying causes rather than adding a baseline file or ignore entries:

  • TextimageHooks takes its six services by constructor injection, replacing thirteen static \Drupal::service() calls.
  • TextimageFactory, TextimageTextFieldFormatter and TextimageImageFieldFormatter take the entity type manager and call getStorage() at the call site, replacing static ImageStyle::load(), ImageStyle::loadMultiple() and EntityViewDisplay::load() calls.
  • Classes that use new static() in a create() factory declare @phpstan-consistent-constructor.
  • Missing generics, return types and parameter types are declared.
  • FALSE returns from StreamWrapperManager::getTarget(), getScheme() and getViaScheme() are handled instead of assumed.
  • Image effect plugin instances are checked with instanceof before their methods are called.
  • Outstanding Rector rules are applied.
  • Kernel test coverage is added for the hooks, the factory and the path processor, so the code these changes touch is actually exercised.

Remaining tasks

  • Review the merge request.
  • Decide whether the level should stay at 7 or be pinned lower.

User interface changes

None.

API changes

Constructor signatures change for code that instantiates these classes directly. Plugins and services built through the container are not affected.

  • TextimageHooks gains a constructor taking StreamWrapperManagerInterface, ImageFactory, TextimageFactoryInterface, FileSystemInterface, TextimageLogger and ConfigFactoryInterface.
  • TextimageFactory::__construct() takes EntityTypeManagerInterface as a public readonly promoted property. The protected $userStorage property is removed.
  • TextimageTextFieldFormatter::__construct() and TextimageImageFieldFormatter::__construct() take EntityTypeManagerInterface in place of ImageStyleStorageInterface.
  • TextimageInterface::setSourceImageFile() and Textimage::setStyle() keep their behaviour; only type declarations are tightened.

Data model changes

None.

Issue fork textimage-3569464

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mondrake created an issue. See original summary.

deciphered made their first commit to this issue’s fork.

deciphered’s picture

Title: Increase code quality to PHPStan level 6 » Increase code quality to PHPStan level 7
Issue summary: View changes
Status: Active » Needs review

Pushed to the MR, with mondrake's commit kept at the base. The scaffold in 5.0.x moved phpstan.neon to level 7 and dropped the old ignoreErrors entries, which surfaced 37 errors. Those are now fixed at level 7 with no baseline: the hooks, the factory and both formatters use dependency injection instead of static \Drupal::service() and entity calls, the readonly promoted properties use DependencySerializationTrait directly so they hold up on PHP 8.3, and the remaining types, generics and FALSE returns are handled properly. I added kernel coverage for the hooks, the factory and the path processor while I was in there, which takes that suite from 9 tests to 26. Constructor signatures change for anyone instantiating those classes directly, though services and plugins built through the container are unaffected.

One part is worth a separate look. The phpunit job used --fail-on-deprecation with a .deprecation-ignore.txt, which is not what the other modules I maintain do, they use the scaffold's phpunit.gitlab-ci.xml. I have aligned this one and removed the ignore file, which is now unused. Happy to split that into its own issue if you would rather keep this to the static analysis. Pipeline is green.

nitinkumar_7’s picture

This changes the existing behavior by silently skipping any image effect that is not a TextOverlayImageEffect. Previously, every configured effect was processed here. Since this issue is focused on PHPStan/type improvements, could we avoid introducing this behavioral change and retain the existing processing behavior?

deciphered’s picture

Right you are, nitinkumar_7. Skipping was the wrong call, it swallows anything unexpected instead of processing it, and that is not what this issue is for.

The narrowing was only there because ImageEffectManager does not override createInstance(), so it inherits the untyped object return from DefaultPluginManager and PHPStan cannot tell what comes back. I have swapped both guards for assert(), which states the type for static analysis without changing what runs. Every effect is processed exactly as before. Pipeline is green.

  • mondrake committed 3b6d4803 on 5.0.x
    task: #3569464 Increase code quality to PHPStan level 7
    
    By: mondrake
    By...
mondrake’s picture

Status: Needs review » Fixed

Merged, thank you. Updated the contribution record.

@deciphered just a question on your commit https://git.drupalcode.org/project/textimage/-/commit/bbe011dca20e63338c...

it adds quite a lot of files that I think are related to local dev environment, are they all needed?

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

deciphered’s picture

@mondrake, it’s using https://github.com/AlexSkrypnyk/drupal_extension_scaffold, and while some files could likely be trimmed the general idea is so offload the maintenance to the scaffold project. I’ve been using the approach on all my contrib modules, as it helps development and test on my local gitlab ci and personal GitHub actions before putting it on D.o.

mondrake’s picture

TIL drupal extension scaffold! Thanks @deciphered, I will take a good look at that.