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
- Check out
5.0.x. - Run
make lint. - PHPStan reports 37 errors, and PHPCS reports
DrupalPractice.Objects.GlobalClasswarnings for the same static calls.
Proposed resolution
Fix the underlying causes rather than adding a baseline file or ignore entries:
TextimageHookstakes its six services by constructor injection, replacing thirteen static\Drupal::service()calls.TextimageFactory,TextimageTextFieldFormatterandTextimageImageFieldFormattertake the entity type manager and callgetStorage()at the call site, replacing staticImageStyle::load(),ImageStyle::loadMultiple()andEntityViewDisplay::load()calls.- Classes that use
new static()in acreate()factory declare@phpstan-consistent-constructor. - Missing generics, return types and parameter types are declared.
FALSEreturns fromStreamWrapperManager::getTarget(),getScheme()andgetViaScheme()are handled instead of assumed.- Image effect plugin instances are checked with
instanceofbefore 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.
TextimageHooksgains a constructor takingStreamWrapperManagerInterface,ImageFactory,TextimageFactoryInterface,FileSystemInterface,TextimageLoggerandConfigFactoryInterface.TextimageFactory::__construct()takesEntityTypeManagerInterfaceas a public readonly promoted property. The protected$userStorageproperty is removed.TextimageTextFieldFormatter::__construct()andTextimageImageFieldFormatter::__construct()takeEntityTypeManagerInterfacein place ofImageStyleStorageInterface.TextimageInterface::setSourceImageFile()andTextimage::setStyle()keep their behaviour; only type declarations are tightened.
Data model changes
None.
Issue fork textimage-3569464
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
Comment #4
decipheredPushed to the MR, with mondrake's commit kept at the base. The scaffold in 5.0.x moved
phpstan.neonto level 7 and dropped the oldignoreErrorsentries, 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 useDependencySerializationTraitdirectly so they hold up on PHP 8.3, and the remaining types, generics andFALSEreturns 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-deprecationwith a.deprecation-ignore.txt, which is not what the other modules I maintain do, they use the scaffold'sphpunit.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.Comment #5
nitinkumar_7 commentedThis 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?
Comment #6
decipheredRight 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
ImageEffectManagerdoes not overridecreateInstance(), so it inherits the untypedobjectreturn fromDefaultPluginManagerand PHPStan cannot tell what comes back. I have swapped both guards forassert(), which states the type for static analysis without changing what runs. Every effect is processed exactly as before. Pipeline is green.Comment #8
mondrakeMerged, 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?
Comment #10
deciphered@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.
Comment #11
mondrakeTIL drupal extension scaffold! Thanks @deciphered, I will take a good look at that.