Skip to content

Conversation

anilkumarthakur60
Copy link
Contributor

This PR upgrades the codebase to PHPStan level 4, significantly improving static analysis, type safety, and overall code clarity. Key changes include:

✅ Summary of Changes:

  • Replaced phpstan/phpstan with larastan/larastan for better Laravel integration.

  • Raised PHPStan level to 4 and updated phpstan.neon configuration.

  • Improved type annotations throughout:

    • Method docblocks (@param, @return, @var) with explicit types (e.g. Collection<int, Media>)
    • Refined PHP type hints in method signatures.
    • Added @throws annotations where exceptions are expected.
  • Modernized some property and method definitions (e.g., visibility, return types).

  • Cleaned up unnecessary use statements and improved PSR compliance.

These changes are non-breaking and aim to improve maintainability and developer confidence through enhanced static analysis.

Let me know if any adjustments are needed!

- Replaced `phpstan/phpstan` with `larastan/larastan` in `composer.json`.
- Updated PHPStan configuration in `phpstan.neon` to include Larastan extensions and increased analysis level to 2.
- Added PHPStan ignore comments in `ImageManipulator.php` for specific lines.
- Refactored return types and method signatures in `Media.php` and `Mediable.php` for better type safety.
- Enhanced `MediableCollection` methods to accept array|string for tags.
- Added new media loading methods in `SampleMediable.php` for better tag handling.
- Increased PHPStan analysis level from 2 to 3 in `phpstan.neon`.
- Enhanced type annotations in `ImportMediaCommand.php` and `CreateImageVariants.php` for better type safety, specifying `Collection<int, Media>` for existing media collections and models.
…files

- Increased PHPStan analysis level from 3 to 4 in `phpstan.neon`.
- Added an ignore error rule for unused traits in PHPStan configuration.
- Improved type annotations in `HandlesMediaUploadExceptions.php`, `ImageManipulator.php`, `Media.php`, and `MediaUploader.php` for better type safety.
- Refactored method signatures to include return types and exception documentation.
- Updated visibility modifiers for class constants in `MediaUploader.php` and `StreamAdapter.php`.
Introduced a driver check to skip tests when format conversion isn't supported under the current configuration. Updated ImageManipulator to dynamically select between 'gd' and 'imagick' drivers based on availability. This ensures compatibility and prevents test failures in unsupported environments.
Copy link
Collaborator

@frasmage frasmage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! though a few changes are needed

Comment on lines 220 to 222
'^https?://' => Plank\Mediable\SourceAdapters\RemoteUrlAdapter::class,
'^/' => Plank\Mediable\SourceAdapters\LocalPathAdapter::class,
'^[a-zA-Z]:\\\\' => Plank\Mediable\SourceAdapters\LocalPathAdapter::class,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: looks like you reformated the whole codebase? I don't mind many of the changes (e.g. various newlines), but please don't align variables/params/array values/annotations. That's not part of the PSR-12 recommendation and means every new could require reformatting the whitespace of all other lines.

Might I recommend keeping this PR focused on phpstan compatibility and performing formatting changes in another PR, if desired? This PR is now 1.5k lines which makes it much harder to review

Condensed conditional statements for better readability and updated PHPStan annotations for Intervention Image compatibility. Added missing exception annotations to methods and loosened type constraints in `SourceAdapterFactory` for flexibility.
Updated type declarations for better consistency and clarified parameter order in `makeVariantOf`. Simplified `if` condition in `ImageManipulator` and removed redundant newlines in `MediaUploader`. These changes improve readability and maintainability.
@anilkumarthakur60 anilkumarthakur60 marked this pull request as draft May 27, 2025 13:21
Copy link
Collaborator

@frasmage frasmage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through with a fine toothed comb. Just a handful of nits to cleanup (and one merge conflict), then it should be looking good

private function imageToStream(
Image $image,
string $outputFormat,
int $outputQuality
) {
if (class_exists(StreamCommand::class)) {
if (class_exists(Intervention\Image\Commands\StreamCommand::class)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there should be a leading \ on this FQCN

@@ -21,7 +21,7 @@ class CreateImageVariants implements ShouldQueue
*/
private array $variantNames;
/**
* @var Collection<Media>
* @var Collection<int, Media>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @var Collection<int, Media>
* @var Collection<Media>

or array-key

@@ -32,7 +32,7 @@ class CreateImageVariants implements ShouldQueue

/**
* CreateImageVariants constructor.
* @param Media|Collection|Media[] $models
* @param Media|Collection<int, Media>|Media[] $models
Copy link
Collaborator

@frasmage frasmage May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param Media|Collection<int, Media>|Media[] $models
* @param Media|Collection<Media>|Media[] $models

Comment on lines +569 to +570
if (
static::hasGlobalScope(SoftDeletingScope::class)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: no newline after if

@@ -628,7 +630,8 @@ public function load($relations)
$relations = func_get_args();
}

if (array_key_exists('media', $relations)
if (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: no newline after if

@@ -142,6 +145,7 @@ public function createImageVariant(
$image = $this->imageManager->read($media->contents());
} else {
// Intervention Image <3.0
/** @phpstan-ignore-next-line */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch to @phpstan-ignore <error type>

&& $variant->getDiskPath() === $originalVariant->getDiskPath()
) {
): void {
if ($originalVariant && $variant->disk === $originalVariant->disk && $variant->getDiskPath() === $originalVariant->getDiskPath()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: please don't change the multiline codestyle

// Intervention Image <3.0
/** @phpstan-ignore-next-line */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch to @phpstan-ignore

if (static::hasGlobalScope(SoftDeletingScope::class)
&& (!property_exists($this, 'forceDeleting') || !$this->forceDeleting)
) {
if (static::hasGlobalScope(SoftDeletingScope::class) && (!property_exists($this, 'forceDeleting') || !$this->forceDeleting)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: please don't change the multiple line conditional style

if (empty($this->config['image_manipulation'])
|| $model->aggregate_type !== Media::TYPE_IMAGE
) {
if (empty($this->config['image_manipulation'])|| $model->aggregate_type !== Media::TYPE_IMAGE) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: please don't change the multiple line conditional style

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants