Skip to content

[13.x] Add mimetypes() instance method to File validation rule#59436

Closed
sumaiazaman wants to merge 3 commits into
laravel:13.xfrom
sumaiazaman:13.x-fix-file-types-chain
Closed

[13.x] Add mimetypes() instance method to File validation rule#59436
sumaiazaman wants to merge 3 commits into
laravel:13.xfrom
sumaiazaman:13.x-fix-file-types-chain

Conversation

@sumaiazaman

Copy link
Copy Markdown
Contributor

Fixes #59242

Summary

File::types() is a static factory method that creates a new instance via new static(). When called in a fluent chain on an existing File or ImageFile instance, it silently discards all prior configuration (max(), min(), image(), etc.) because PHP resolves it as a static call that returns a brand new object.

The Bug

// max() is silently dropped — types() creates a new instance
$rule = File::image()->max(1)->types(['jpg', 'jpeg']);

// An 8KB file passes — max(1) was discarded
$validator = Validator::make(
    ['file' => UploadedFile::fake()->create('photo.jpg', 100)],
    ['file' => [$rule]]
);
$validator->fails(); // false — should be true!

The Fix

Added a mimetypes() instance method that sets MIME types on the existing instance (returning $this), preserving the fluent chain. The static types() factory now delegates to mimetypes() internally, so existing File::types(...) usage continues to work identically.

// Static factory still works (no change)
File::types(['image/jpeg'])->max(1024);

// Instance method preserves chain (new)
File::image()->max(1)->mimetypes(['image/jpeg']);

Why This Doesn't Break Existing Features

  • File::types(...) as a static factory continues to work identically — it now calls (new static())->mimetypes(...) internally
  • All 25 existing File validation tests pass unchanged
  • The new mimetypes() method is purely additive

Changes

  • src/Illuminate/Validation/Rules/File.php — Added mimetypes() instance method, refactored types() to use it
  • tests/Validation/ValidationFileRuleTest.php — Added 2 tests: chain preservation and static factory backward compat

Test Plan

  • testMimetypesInstanceMethodPreservesChainmax(1) constraint is preserved when chaining with mimetypes()
  • testStaticTypesStillWorksAsFactoryFile::types(...) continues to work as before
  • All 25 existing File validation tests pass

File::types() is a static factory that creates a new instance, which
silently discards any prior chain configuration (max, min, etc.) when
called on an existing instance. The new mimetypes() instance method
allows setting MIME types while preserving the fluent chain.

Fixes laravel#59242
@shaedrich

shaedrich commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

I predict, having two methods that essentially do the same thing, will confuse devs as to when to use which.

Therefore, we could solve this the following way, even though, it's a little "magic" (see the alternative implementation #59324):

-   /**
-    * Limit the uploaded file to the given MIME types or file extensions.
-    *
-    * @param  string|array<int, string>  $mimetypes
-    * @return static
-    */
-   public static function types($mimetypes)
-   {
-       return tap(new static(), fn ($file) => $file->allowedMimetypes = (array) $mimetypes);
-   }
+   /**
+    * Set the allowed MIME types or file extensions on an existing instance.
+    *
+    * @param  string|array<int, string>  $mimetypes
+    * @return $this
+    */
+   private function mimetypes($mimetypes)
+   {
+       $this->allowedMimetypes = (array) $mimetypes;
+
+       return $this;
+   }
+
+   /**
+    * @phpstan-return ($name is 'types' ? self : never)
+    *
+    * @throws \BadMethodCallException
+    */
+   public function __callStatic(string $name, mixed $arguments)
+   {
+       if ($name === 'types') {
+           return (new static())->types($arguments);
+       }
+
+       throw new BadMethodCallException(sprintf(
+           'Method %s::%s does not exist.', static::class, $method
+       ));
+   }
+
+   /**
+    * @phpstan-return ($name is 'types' ? $this : never)
+    *
+    * @throws \BadMethodCallException
+    */
+   public function __call(string $name, mixed $arguments)
+   {
+       if ($name === 'types') {
+           return $this->types($arguments);
+       }
+
+       throw new BadMethodCallException(sprintf(
+           'Method %s::%s does not exist.', static::class, $method
+       ));
+   }

@sumaiazaman

Copy link
Copy Markdown
Contributor Author

I predict, having two methods that essentially do the same thing, will confuse devs as to when to use which.

Therefore, we could solve this the following way, even though, it's a little "magic" (see the alternative implementation #59324):

-   /**
-    * Limit the uploaded file to the given MIME types or file extensions.
-    *
-    * @param  string|array<int, string>  $mimetypes
-    * @return static
-    */
-   public static function types($mimetypes)
-   {
-       return tap(new static(), fn ($file) => $file->allowedMimetypes = (array) $mimetypes);
-   }
+   /**
+    * Set the allowed MIME types or file extensions on an existing instance.
+    *
+    * @param  string|array<int, string>  $mimetypes
+    * @return $this
+    */
+   private function mimetypes($mimetypes)
+   {
+       $this->allowedMimetypes = (array) $mimetypes;
+
+       return $this;
+   }
+
+   /**
+    * @phpstan-return ($name is 'types' ? self : never)
+    *
+    * @throws \BadMethodCallException
+    */
+   public function __callStatic(string $name, mixed $arguments)
+   {
+       if ($name === 'types') {
+           return (new static())->types($arguments);
+       }
+
+       throw new BadMethodCallException(sprintf(
+           'Method %s::%s does not exist.', static::class, $method
+       ));
+   }
+
+   /**
+    * @phpstan-return ($name is 'types' ? $this : never)
+    *
+    * @throws \BadMethodCallException
+    */
+   public function __call(string $name, mixed $arguments)
+   {
+       if ($name === 'types') {
+           return $this->types($arguments);
+       }
+
+       throw new BadMethodCallException(sprintf(
+           'Method %s::%s does not exist.', static::class, $method
+       ));
+   }

Good point about the two methods. Your __call/__callStatic approach would work, but it adds complexity — magic methods lose IDE autocomplete and click-through navigation, and need special PHPStan annotations.

The reason I used a separate mimetypes() name is to keep it simple:

  • File::types('pdf') — static factory, creates new instance (existing behavior, no breaking change)
  • ->mimetypes(['image/jpeg']) — instance method, preserves the chain

But I'm open to whatever approach the maintainers prefer. The key issue is that File::image()->max(1)->types(['jpg']) silently drops the max() constraint today, and developers won't know their validation is broken until a file that should be rejected gets through.

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.

File::types() silently discards prior chain configuration when called as instance method

3 participants