alihan/awesome-reviewers
1
0
Fork 0
mirror of https://github.com/baz-scm/awesome-reviewers.git synced 2025-08-20 18:58:52 +03:00
Code Issues Packages Projects Releases Wiki Activity

Add 17 awesome reviewers for laravel/framework

Browse Source
This commit is contained in:
internal-baz-ci-app[bot]
2025-07-03 01:13:20 +00:00
committed by GitHub
parent e894ffbee6
commit c1d711e4e5
17 changed files with 4256 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

299
_reviewers/framework-cache-expensive-operations.md Normal file
View File

@@ -0,0 +1,299 @@
---
title: Cache expensive operations
description: Identify and cache results of expensive operations that may be called
repeatedly during a request lifecycle, especially recursive functions or operations
performed in loops. This reduces redundant processing and can significantly improve
application performance.
repository: laravel/framework
label: Performance Optimization
language: PHP
comments_count: 6
repository_stars: 33763
---
Identify and cache results of expensive operations that may be called repeatedly during a request lifecycle, especially recursive functions or operations performed in loops. This reduces redundant processing and can significantly improve application performance.
For example, when working with trait detection or class hierarchy traversal:
```php
// Instead of repeatedly calling an expensive function
public function isPrunable(): bool
{
return in_array(Prunable::class, class_uses_recursive(static::class)) || static::isMassPrunable();
}
// Cache the result for the lifetime of the request
protected static $classTraitsCache = [];
public function isPrunable(): bool
{
$class = static::class;
// Only perform the expensive operation once per class
if (!isset(static::$classTraitsCache[$class])) {
static::$classTraitsCache[$class] = class_uses_recursive($class);
}
return in_array(Prunable::class, static::$classTraitsCache[$class]) || static::isMassPrunable();
}
```
Similarly, when selecting methods for array operations, favor efficient approaches. For instance, use `array_search()` over `array_flip()` followed by key lookup for better performance with large arrays.
When implementing expensive transformations (like `toArray()` on models), use memoization techniques to avoid recalculating the same values multiple times during a single request.
[
{
"discussion_id": "2152151153",
"pr_number": 56060,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"created_at": "2025-06-17T12:34:12+00:00",
"commented_code": "return $this;\n }\n\n /**\n * Determine if the given model class is soft deletable.\n */\n public static function isSoftDeletable(): bool\n {\n return in_array(SoftDeletes::class, class_uses_recursive(static::class));\n }\n\n /**\n * Determine if the given model class is prunable.\n */\n protected function isPrunable(): bool\n {\n return in_array(Prunable::class, class_uses_recursive(static::class)) || static::isMassPrunable();",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2152151153",
"repo_full_name": "laravel/framework",
"pr_number": 56060,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"discussion_id": "2152151153",
"commented_code": "@@ -2513,6 +2513,30 @@ public function escapeWhenCastingToString($escape = true)\n return $this;\n }\n \n+ /**\n+ * Determine if the given model class is soft deletable.\n+ */\n+ public static function isSoftDeletable(): bool\n+ {\n+ return in_array(SoftDeletes::class, class_uses_recursive(static::class));\n+ }\n+\n+ /**\n+ * Determine if the given model class is prunable.\n+ */\n+ protected function isPrunable(): bool\n+ {\n+ return in_array(Prunable::class, class_uses_recursive(static::class)) || static::isMassPrunable();",
"comment_created_at": "2025-06-17T12:34:12+00:00",
"comment_author": "cosmastech",
"comment_body": "**thought:** It would be great if there were a ClassUsesRecursive once helper that would store these values for the lifetime of a request.\r\n\r\nWritten out as you have here, we can see that we're going to loop through all the traits of the class and its parent-classes twice if the class is MassPrunable but not Prunable.",
"pr_file_module": null
},
{
"comment_id": "2152406875",
"repo_full_name": "laravel/framework",
"pr_number": 56060,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"discussion_id": "2152151153",
"commented_code": "@@ -2513,6 +2513,30 @@ public function escapeWhenCastingToString($escape = true)\n return $this;\n }\n \n+ /**\n+ * Determine if the given model class is soft deletable.\n+ */\n+ public static function isSoftDeletable(): bool\n+ {\n+ return in_array(SoftDeletes::class, class_uses_recursive(static::class));\n+ }\n+\n+ /**\n+ * Determine if the given model class is prunable.\n+ */\n+ protected function isPrunable(): bool\n+ {\n+ return in_array(Prunable::class, class_uses_recursive(static::class)) || static::isMassPrunable();",
"comment_created_at": "2025-06-17T14:20:12+00:00",
"comment_author": "shaedrich",
"comment_body": "Good point \ud83d\udc4d\ud83c\udffb I had a similar idea\u2014looks like there's an appetite for this \ud83d\ude80",
"pr_file_module": null
},
{
"comment_id": "2157457993",
"repo_full_name": "laravel/framework",
"pr_number": 56060,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"discussion_id": "2152151153",
"commented_code": "@@ -2513,6 +2513,30 @@ public function escapeWhenCastingToString($escape = true)\n return $this;\n }\n \n+ /**\n+ * Determine if the given model class is soft deletable.\n+ */\n+ public static function isSoftDeletable(): bool\n+ {\n+ return in_array(SoftDeletes::class, class_uses_recursive(static::class));\n+ }\n+\n+ /**\n+ * Determine if the given model class is prunable.\n+ */\n+ protected function isPrunable(): bool\n+ {\n+ return in_array(Prunable::class, class_uses_recursive(static::class)) || static::isMassPrunable();",
"comment_created_at": "2025-06-19T17:51:26+00:00",
"comment_author": "fgaroby",
"comment_body": "@cosmastech : [something like this](https://github.com/laravel/framework/pull/56079)?",
"pr_file_module": null
},
{
"comment_id": "2157698190",
"repo_full_name": "laravel/framework",
"pr_number": 56060,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"discussion_id": "2152151153",
"commented_code": "@@ -2513,6 +2513,30 @@ public function escapeWhenCastingToString($escape = true)\n return $this;\n }\n \n+ /**\n+ * Determine if the given model class is soft deletable.\n+ */\n+ public static function isSoftDeletable(): bool\n+ {\n+ return in_array(SoftDeletes::class, class_uses_recursive(static::class));\n+ }\n+\n+ /**\n+ * Determine if the given model class is prunable.\n+ */\n+ protected function isPrunable(): bool\n+ {\n+ return in_array(Prunable::class, class_uses_recursive(static::class)) || static::isMassPrunable();",
"comment_created_at": "2025-06-19T21:55:02+00:00",
"comment_author": "cosmastech",
"comment_body": "Yeah! Nice.",
"pr_file_module": null
}
]
},
{
"discussion_id": "2152154653",
"pr_number": 56060,
"pr_file": "src/Illuminate/Database/Eloquent/MassPrunable.php",
"created_at": "2025-06-17T12:35:55+00:00",
"commented_code": "$total = 0;\n\n $softDeletable = in_array(SoftDeletes::class, class_uses_recursive(get_class($this)));\n\n do {\n $total += $count = $softDeletable\n $total += $count = static::isSoftDeletable()",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2152154653",
"repo_full_name": "laravel/framework",
"pr_number": 56060,
"pr_file": "src/Illuminate/Database/Eloquent/MassPrunable.php",
"discussion_id": "2152154653",
"commented_code": "@@ -23,10 +26,8 @@ public function pruneAll(int $chunkSize = 1000)\n \n $total = 0;\n \n- $softDeletable = in_array(SoftDeletes::class, class_uses_recursive(get_class($this)));\n-\n do {\n- $total += $count = $softDeletable\n+ $total += $count = static::isSoftDeletable()",
"comment_created_at": "2025-06-17T12:35:55+00:00",
"comment_author": "cosmastech",
"comment_body": "**note:** This was explicitly moved outside of the loop so we wouldn't have to recursively scan the model again and again. https://github.com/laravel/framework/pull/55274",
"pr_file_module": null
},
{
"comment_id": "2152409906",
"repo_full_name": "laravel/framework",
"pr_number": 56060,
"pr_file": "src/Illuminate/Database/Eloquent/MassPrunable.php",
"discussion_id": "2152154653",
"commented_code": "@@ -23,10 +26,8 @@ public function pruneAll(int $chunkSize = 1000)\n \n $total = 0;\n \n- $softDeletable = in_array(SoftDeletes::class, class_uses_recursive(get_class($this)));\n-\n do {\n- $total += $count = $softDeletable\n+ $total += $count = static::isSoftDeletable()",
"comment_created_at": "2025-06-17T14:21:26+00:00",
"comment_author": "shaedrich",
"comment_body": "Thanks for the hint\u2014I'll adjust that \ud83d\udc4d\ud83c\udffb",
"pr_file_module": null
}
]
},
{
"discussion_id": "1938570236",
"pr_number": 54442,
"pr_file": "src/Illuminate/Bus/PendingBatch.php",
"created_at": "2025-02-02T20:14:09+00:00",
"commented_code": "return $batch;\n }\n\n private function checkJobIsBatchable(object|array $job): void\n {\n foreach (Arr::wrap($job) as $job) {\n if ($job instanceof PendingBatch) {\n $this->checkJobIsBatchable($job->jobs->all());\n\n return;\n }\n\n if (! in_array(Batchable::class, class_uses_recursive($job))) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1938570236",
"repo_full_name": "laravel/framework",
"pr_number": 54442,
"pr_file": "src/Illuminate/Bus/PendingBatch.php",
"discussion_id": "1938570236",
"commented_code": "@@ -414,4 +412,19 @@ protected function store($repository)\n \n return $batch;\n }\n+\n+ private function checkJobIsBatchable(object|array $job): void\n+ {\n+ foreach (Arr::wrap($job) as $job) {\n+ if ($job instanceof PendingBatch) {\n+ $this->checkJobIsBatchable($job->jobs->all());\n+\n+ return;\n+ }\n+\n+ if (! in_array(Batchable::class, class_uses_recursive($job))) {",
"comment_created_at": "2025-02-02T20:14:09+00:00",
"comment_author": "cosmastech",
"comment_body": "I wonder if it wouldn't be useful to cache the job's class so you don't have to call class_uses_recursive for multiple instances of the same class.",
"pr_file_module": null
},
{
"comment_id": "1938615259",
"repo_full_name": "laravel/framework",
"pr_number": 54442,
"pr_file": "src/Illuminate/Bus/PendingBatch.php",
"discussion_id": "1938570236",
"commented_code": "@@ -414,4 +412,19 @@ protected function store($repository)\n \n return $batch;\n }\n+\n+ private function checkJobIsBatchable(object|array $job): void\n+ {\n+ foreach (Arr::wrap($job) as $job) {\n+ if ($job instanceof PendingBatch) {\n+ $this->checkJobIsBatchable($job->jobs->all());\n+\n+ return;\n+ }\n+\n+ if (! in_array(Batchable::class, class_uses_recursive($job))) {",
"comment_created_at": "2025-02-02T23:32:52+00:00",
"comment_author": "shaedrich",
"comment_body": "While it's not possible with `class_uses_recursive`, this could be taken one step further, by implementing something similar to `class_uses_recursive`: Caching all classes in the recursive chain \ud83e\udd14 ",
"pr_file_module": null
}
]
},
{
"discussion_id": "1714821791",
"pr_number": 52461,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"created_at": "2024-08-13T07:39:12+00:00",
"commented_code": "*/\n public function toArray()\n {\n return array_merge($this->attributesToArray(), $this->relationsToArray());\n return $this->once(\n fn () => array_merge($this->attributesToArray(), $this->relationsToArray()),\n $this->attributesToArray(),\n );",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1714821791",
"repo_full_name": "laravel/framework",
"pr_number": 52461,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"discussion_id": "1714821791",
"commented_code": "@@ -1644,7 +1647,10 @@ public function callNamedScope($scope, array $parameters = [])\n */\n public function toArray()\n {\n- return array_merge($this->attributesToArray(), $this->relationsToArray());\n+ return $this->once(\n+ fn () => array_merge($this->attributesToArray(), $this->relationsToArray()),\n+ $this->attributesToArray(),\n+ );",
"comment_created_at": "2024-08-13T07:39:12+00:00",
"comment_author": "Tofandel",
"comment_body": "```suggestion\r\n $attributes = $this->attributesToArray();\r\n return $this->once(\r\n fn () => array_merge($attributes, $this->relationsToArray()),\r\n $attributes,\r\n );\r\n```\r\n\r\nThis kind of methods are a bit expensive so I would avoid calling it twice\r\n\r\nIdeally `default` could be a callback",
"pr_file_module": null
},
{
"comment_id": "1715004249",
"repo_full_name": "laravel/framework",
"pr_number": 52461,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"discussion_id": "1714821791",
"commented_code": "@@ -1644,7 +1647,10 @@ public function callNamedScope($scope, array $parameters = [])\n */\n public function toArray()\n {\n- return array_merge($this->attributesToArray(), $this->relationsToArray());\n+ return $this->once(\n+ fn () => array_merge($this->attributesToArray(), $this->relationsToArray()),\n+ $this->attributesToArray(),\n+ );",
"comment_created_at": "2024-08-13T09:45:54+00:00",
"comment_author": "samlev",
"comment_body": "I've updated the default to accept a callback, and cache that value.\r\n\r\nYour suggested change wouldn't prevent `attributesToArray()` from being called on each recursive call to `toArray()` - it would just ignore the result on subsequent calls.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1669441618",
"pr_number": 52012,
"pr_file": "src/Illuminate/Collections/Collection.php",
"created_at": "2024-07-08T23:32:53+00:00",
"commented_code": "return new static(array_filter($this->items));\n }\n\n /**\n * Run a map and filter over each of the items.\n *\n * @template TMapValue\n *\n * @param callable(TValue, TKey): TMapValue $callback\n * @param (callable(TValue, TKey): bool)|null $reject\n * @return static<TKey, TMapValue>\n */\n public function filterMap($callable, $filter = null)\n {\n $filter = $filter === null\n ? fn ($value) => (bool) $value\n : match ((new ReflectionFunction($filter))->getNumberOfParameters()) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1669441618",
"repo_full_name": "laravel/framework",
"pr_number": 52012,
"pr_file": "src/Illuminate/Collections/Collection.php",
"discussion_id": "1669441618",
"commented_code": "@@ -372,6 +373,35 @@ public function filter(?callable $callback = null)\n return new static(array_filter($this->items));\n }\n \n+ /**\n+ * Run a map and filter over each of the items.\n+ *\n+ * @template TMapValue\n+ *\n+ * @param callable(TValue, TKey): TMapValue $callback\n+ * @param (callable(TValue, TKey): bool)|null $reject\n+ * @return static<TKey, TMapValue>\n+ */\n+ public function filterMap($callable, $filter = null)\n+ {\n+ $filter = $filter === null\n+ ? fn ($value) => (bool) $value\n+ : match ((new ReflectionFunction($filter))->getNumberOfParameters()) {",
"comment_created_at": "2024-07-08T23:32:53+00:00",
"comment_author": "Tofandel",
"comment_body": "This reflection seems unnecessary; why not just use `$filter` as is?",
"pr_file_module": null
},
{
"comment_id": "1669518971",
"repo_full_name": "laravel/framework",
"pr_number": 52012,
"pr_file": "src/Illuminate/Collections/Collection.php",
"discussion_id": "1669441618",
"commented_code": "@@ -372,6 +373,35 @@ public function filter(?callable $callback = null)\n return new static(array_filter($this->items));\n }\n \n+ /**\n+ * Run a map and filter over each of the items.\n+ *\n+ * @template TMapValue\n+ *\n+ * @param callable(TValue, TKey): TMapValue $callback\n+ * @param (callable(TValue, TKey): bool)|null $reject\n+ * @return static<TKey, TMapValue>\n+ */\n+ public function filterMap($callable, $filter = null)\n+ {\n+ $filter = $filter === null\n+ ? fn ($value) => (bool) $value\n+ : match ((new ReflectionFunction($filter))->getNumberOfParameters()) {",
"comment_created_at": "2024-07-09T01:25:38+00:00",
"comment_author": "timacdonald",
"comment_body": "PHP's built-in methods throw an exception when you pass too many arguments to them.\r\n\r\nWe check the number of arguments here to ensure that we don't hit that exception if the function only accepts a single argument (as we pass the value and the key to the filter method).\r\n\r\nWe take a similar approach in the `Collection:map` and `Arr::map` functions:\r\n\r\nhttps://github.com/laravel/framework/blob/0e9053da9d709c544200260cc0be5e223ea8ff93/src/Illuminate/Collections/Arr.php#L600-L610\r\n\r\nHowever, I benchmarked this approach with my approach and found that using reflection was faster than allowing the exception to be thrown and the impact of using reflection is negligible.\r\n\r\nThe tests fail without this work.",
"pr_file_module": null
},
{
"comment_id": "1669955879",
"repo_full_name": "laravel/framework",
"pr_number": 52012,
"pr_file": "src/Illuminate/Collections/Collection.php",
"discussion_id": "1669441618",
"commented_code": "@@ -372,6 +373,35 @@ public function filter(?callable $callback = null)\n return new static(array_filter($this->items));\n }\n \n+ /**\n+ * Run a map and filter over each of the items.\n+ *\n+ * @template TMapValue\n+ *\n+ * @param callable(TValue, TKey): TMapValue $callback\n+ * @param (callable(TValue, TKey): bool)|null $reject\n+ * @return static<TKey, TMapValue>\n+ */\n+ public function filterMap($callable, $filter = null)\n+ {\n+ $filter = $filter === null\n+ ? fn ($value) => (bool) $value\n+ : match ((new ReflectionFunction($filter))->getNumberOfParameters()) {",
"comment_created_at": "2024-07-09T08:04:01+00:00",
"comment_author": "Tofandel",
"comment_body": "Ah yes, the good ol' php inbuilt functions that always destroy all your hopes \ud83d\ude05 I forgot about those\r\n\r\nReflection is indeed better than try, catch, because try catch means you need to call the function which if it had some side effects could be very undesirable, and reflection is always super fast in php (I don't know why this myth came to be that it's slow and needs caching) but the advantage of the try catch is that it works even if you use this kind of things\r\n\r\n`$this->filterMap($callable, fn () => ! $reject(...func_get_args()))`",
"pr_file_module": null
}
]
},
{
"discussion_id": "1642109271",
"pr_number": 51809,
"pr_file": "src/Illuminate/Database/Eloquent/Relations/Relation.php",
"created_at": "2024-06-17T03:40:13+00:00",
"commented_code": "return static::$morphMap[$alias] ?? null;\n }\n\n /**\n * Get the alias associated with a custom polymorphic class.\n *\n * @param string $className\n * @return int|string|null\n */\n public static function getMorphAlias(string $className)\n {\n foreach (static::$morphMap as $alias => $morphedClass) {\n if ($morphedClass === $className) {\n return $alias;\n }\n }\n\n return null;",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1642109271",
"repo_full_name": "laravel/framework",
"pr_number": 51809,
"pr_file": "src/Illuminate/Database/Eloquent/Relations/Relation.php",
"discussion_id": "1642109271",
"commented_code": "@@ -501,6 +501,23 @@ public static function getMorphedModel($alias)\n return static::$morphMap[$alias] ?? null;\n }\n \n+ /**\n+ * Get the alias associated with a custom polymorphic class.\n+ *\n+ * @param string $className\n+ * @return int|string|null\n+ */\n+ public static function getMorphAlias(string $className)\n+ {\n+ foreach (static::$morphMap as $alias => $morphedClass) {\n+ if ($morphedClass === $className) {\n+ return $alias;\n+ }\n+ }\n+\n+ return null;",
"comment_created_at": "2024-06-17T03:40:13+00:00",
"comment_author": "calebdw",
"comment_body": "Would it not be easier to do the following?\r\n\r\n```suggestion\r\n return array_flip(static::$morphMap)[$className] ?? null;\r\n```",
"pr_file_module": null
},
{
"comment_id": "1642138211",
"repo_full_name": "laravel/framework",
"pr_number": 51809,
"pr_file": "src/Illuminate/Database/Eloquent/Relations/Relation.php",
"discussion_id": "1642109271",
"commented_code": "@@ -501,6 +501,23 @@ public static function getMorphedModel($alias)\n return static::$morphMap[$alias] ?? null;\n }\n \n+ /**\n+ * Get the alias associated with a custom polymorphic class.\n+ *\n+ * @param string $className\n+ * @return int|string|null\n+ */\n+ public static function getMorphAlias(string $className)\n+ {\n+ foreach (static::$morphMap as $alias => $morphedClass) {\n+ if ($morphedClass === $className) {\n+ return $alias;\n+ }\n+ }\n+\n+ return null;",
"comment_created_at": "2024-06-17T04:15:42+00:00",
"comment_author": "donnysim",
"comment_body": "Would it not be easier to use array_search? Flip would create unnecessary garbage and for heavy morph use that's not desirable.",
"pr_file_module": null
},
{
"comment_id": "1642887423",
"repo_full_name": "laravel/framework",
"pr_number": 51809,
"pr_file": "src/Illuminate/Database/Eloquent/Relations/Relation.php",
"discussion_id": "1642109271",
"commented_code": "@@ -501,6 +501,23 @@ public static function getMorphedModel($alias)\n return static::$morphMap[$alias] ?? null;\n }\n \n+ /**\n+ * Get the alias associated with a custom polymorphic class.\n+ *\n+ * @param string $className\n+ * @return int|string|null\n+ */\n+ public static function getMorphAlias(string $className)\n+ {\n+ foreach (static::$morphMap as $alias => $morphedClass) {\n+ if ($morphedClass === $className) {\n+ return $alias;\n+ }\n+ }\n+\n+ return null;",
"comment_created_at": "2024-06-17T14:12:24+00:00",
"comment_author": "calebdw",
"comment_body": "@taylorotwell, I was mistaken---benchmarking shows that using `array_search` is a good deal faster than having to flip the entire array and then still search through the keys:\r\n\r\n![image](https://github.com/laravel/framework/assets/4176520/58880242-29ac-4a69-b8ad-3280b9190553)\r\n\r\n",
"pr_file_module": null
}
]
}
]

217
_reviewers/framework-descriptive-configuration-keys.md Normal file
View File

@@ -0,0 +1,217 @@
---
title: Descriptive configuration keys
description: Configuration keys should clearly indicate their value type, units, or
expected format to prevent misunderstandings and errors. Include unit information
directly in key names when appropriate and ensure documentation accurately reflects
the default values and behavior.
repository: laravel/framework
label: Configurations
language: PHP
comments_count: 4
repository_stars: 33763
---
Configuration keys should clearly indicate their value type, units, or expected format to prevent misunderstandings and errors. Include unit information directly in key names when appropriate and ensure documentation accurately reflects the default values and behavior.
For example, instead of using a generic key with ambiguous units:
```php
// Unclear - are these hours or minutes?
'maintenance_bypass_cookie_lifetime' => (int) env('SESSION_MAINTENANCE_BYPASS_COOKIE_LIFETIME', 720),
```
Use a more descriptive key that includes the unit:
```php
'maintenance_bypass_cookie_lifetime_minutes' => (int) env('SESSION_MAINTENANCE_BYPASS_COOKIE_LIFETIME_MINUTES', 720),
```
This approach makes configuration self-documenting, reduces the need to consult documentation, and prevents errors from unit mismatches or incorrect assumptions about value types. When environment variables serve as configuration sources, maintain this same naming convention to ensure consistency across your application's configuration system.
[
{
"discussion_id": "2086556212",
"pr_number": 55724,
"pr_file": "src/Illuminate/Foundation/Http/MaintenanceModeBypassCookie.php",
"created_at": "2025-05-13T11:06:49+00:00",
"commented_code": "*/\n public static function create(string $key)\n {\n $expiresAt = Carbon::now()->addHours(12);\n $expiresAt = Carbon::now()->addHours(config('session.maintenance_bypass_cookie_lifetime'));",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2086556212",
"repo_full_name": "laravel/framework",
"pr_number": 55724,
"pr_file": "src/Illuminate/Foundation/Http/MaintenanceModeBypassCookie.php",
"discussion_id": "2086556212",
"commented_code": "@@ -15,7 +15,7 @@ class MaintenanceModeBypassCookie\n */\n public static function create(string $key)\n {\n- $expiresAt = Carbon::now()->addHours(12);\n+ $expiresAt = Carbon::now()->addHours(config('session.maintenance_bypass_cookie_lifetime'));",
"comment_created_at": "2025-05-13T11:06:49+00:00",
"comment_author": "shaedrich",
"comment_body": "According to your config doc block,\r\n> number of minutes\r\n\r\nthis is now minutes that are added\r\n```suggestion\r\n $expiresAt = Carbon::now()->addMinutes(config('session.maintenance_bypass_cookie_lifetime'));\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "2086558463",
"pr_number": 55724,
"pr_file": "config/session.php",
"created_at": "2025-05-13T11:08:14+00:00",
"commented_code": "'expire_on_close' => env('SESSION_EXPIRE_ON_CLOSE', false),\n\n /*\n |--------------------------------------------------------------------------\n | Maintenance Bypass Cookie Lifetime\n |--------------------------------------------------------------------------\n |\n | Here you may specify the number of minutes the maintenance mode\n | bypass cookie should remain valid. This cookie is issued when a\n | user accesses the application using the secret bypass URL. Once\n | set, it allows temporary access to the application while it is\n | in maintenance mode. The default duration is 12 hours.\n |\n */\n\n 'maintenance_bypass_cookie_lifetime' => (int) env('SESSION_MAINTENANCE_BYPASS_COOKIE_LIFETIME', 7720),",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2086558463",
"repo_full_name": "laravel/framework",
"pr_number": 55724,
"pr_file": "config/session.php",
"discussion_id": "2086558463",
"commented_code": "@@ -36,6 +36,21 @@\n \n 'expire_on_close' => env('SESSION_EXPIRE_ON_CLOSE', false),\n \n+ /*\n+ |--------------------------------------------------------------------------\n+ | Maintenance Bypass Cookie Lifetime\n+ |--------------------------------------------------------------------------\n+ |\n+ | Here you may specify the number of minutes the maintenance mode\n+ | bypass cookie should remain valid. This cookie is issued when a\n+ | user accesses the application using the secret bypass URL. Once\n+ | set, it allows temporary access to the application while it is\n+ | in maintenance mode. The default duration is 12 hours.\n+ |\n+ */\n+\n+ 'maintenance_bypass_cookie_lifetime' => (int) env('SESSION_MAINTENANCE_BYPASS_COOKIE_LIFETIME', 7720),",
"comment_created_at": "2025-05-13T11:08:14+00:00",
"comment_author": "shaedrich",
"comment_body": "Accoding to your PHPDoc block,\r\n> number of minutes\r\n\r\nbut 7720 minutes is not 12 hours\r\n```suggestion\r\n /*\r\n |--------------------------------------------------------------------------\r\n | Maintenance Bypass Cookie Lifetime\r\n |--------------------------------------------------------------------------\r\n |\r\n | Here you may specify the number of minutes the maintenance mode\r\n | bypass cookie should remain valid. This cookie is issued when a\r\n | user accesses the application using the secret bypass URL. Once\r\n | set, it allows temporary access to the application while it is\r\n | in maintenance mode. The default duration is 12 hours.\r\n |\r\n */\r\n\r\n 'maintenance_bypass_cookie_lifetime' => (int) env('SESSION_MAINTENANCE_BYPASS_COOKIE_LIFETIME', 720),\r\n```",
"pr_file_module": null
},
{
"comment_id": "2086559648",
"repo_full_name": "laravel/framework",
"pr_number": 55724,
"pr_file": "config/session.php",
"discussion_id": "2086558463",
"commented_code": "@@ -36,6 +36,21 @@\n \n 'expire_on_close' => env('SESSION_EXPIRE_ON_CLOSE', false),\n \n+ /*\n+ |--------------------------------------------------------------------------\n+ | Maintenance Bypass Cookie Lifetime\n+ |--------------------------------------------------------------------------\n+ |\n+ | Here you may specify the number of minutes the maintenance mode\n+ | bypass cookie should remain valid. This cookie is issued when a\n+ | user accesses the application using the secret bypass URL. Once\n+ | set, it allows temporary access to the application while it is\n+ | in maintenance mode. The default duration is 12 hours.\n+ |\n+ */\n+\n+ 'maintenance_bypass_cookie_lifetime' => (int) env('SESSION_MAINTENANCE_BYPASS_COOKIE_LIFETIME', 7720),",
"comment_created_at": "2025-05-13T11:09:02+00:00",
"comment_author": "shaedrich",
"comment_body": "Maybe, to avoid confusion when using, the unit should be included in the key:\r\n```suggestion\r\n /*\r\n |--------------------------------------------------------------------------\r\n | Maintenance Bypass Cookie Lifetime\r\n |--------------------------------------------------------------------------\r\n |\r\n | Here you may specify the number of minutes the maintenance mode\r\n | bypass cookie should remain valid. This cookie is issued when a\r\n | user accesses the application using the secret bypass URL. Once\r\n | set, it allows temporary access to the application while it is\r\n | in maintenance mode. The default duration is 12 hours.\r\n |\r\n */\r\n\r\n 'maintenance_bypass_cookie_lifetime_minutes' => (int) env('SESSION_MAINTENANCE_BYPASS_COOKIE_LIFETIME_MINUTES', 720),\r\n```",
"pr_file_module": null
},
{
"comment_id": "2086578428",
"repo_full_name": "laravel/framework",
"pr_number": 55724,
"pr_file": "config/session.php",
"discussion_id": "2086558463",
"commented_code": "@@ -36,6 +36,21 @@\n \n 'expire_on_close' => env('SESSION_EXPIRE_ON_CLOSE', false),\n \n+ /*\n+ |--------------------------------------------------------------------------\n+ | Maintenance Bypass Cookie Lifetime\n+ |--------------------------------------------------------------------------\n+ |\n+ | Here you may specify the number of minutes the maintenance mode\n+ | bypass cookie should remain valid. This cookie is issued when a\n+ | user accesses the application using the secret bypass URL. Once\n+ | set, it allows temporary access to the application while it is\n+ | in maintenance mode. The default duration is 12 hours.\n+ |\n+ */\n+\n+ 'maintenance_bypass_cookie_lifetime' => (int) env('SESSION_MAINTENANCE_BYPASS_COOKIE_LIFETIME', 7720),",
"comment_created_at": "2025-05-13T11:20:06+00:00",
"comment_author": "sajjadhossainshohag",
"comment_body": "@shaedrich This was a mistake, thanks.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1868980349",
"pr_number": 53749,
"pr_file": "config/mail.php",
"created_at": "2024-12-04T08:46:30+00:00",
"commented_code": "'mailers' => [\n\n 'smtp' => [\n 'scheme' => env('MAIL_MAILER', 'smtp'),",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1868980349",
"repo_full_name": "laravel/framework",
"pr_number": 53749,
"pr_file": "config/mail.php",
"discussion_id": "1868980349",
"commented_code": "@@ -38,6 +38,7 @@\n 'mailers' => [\n \n 'smtp' => [\n+ 'scheme' => env('MAIL_MAILER', 'smtp'),",
"comment_created_at": "2024-12-04T08:46:30+00:00",
"comment_author": "crynobone",
"comment_body": "This is incorrect, possible values of `MAIL_MAILER` are \"smtp\", \"sendmail\", \"mailgun\", \"ses\", \"ses-v2\", \"postmark\", \"resend\", \"log\", \"array\", \"failover\" or \"roundrobin\"",
"pr_file_module": null
},
{
"comment_id": "1869165206",
"repo_full_name": "laravel/framework",
"pr_number": 53749,
"pr_file": "config/mail.php",
"discussion_id": "1868980349",
"commented_code": "@@ -38,6 +38,7 @@\n 'mailers' => [\n \n 'smtp' => [\n+ 'scheme' => env('MAIL_MAILER', 'smtp'),",
"comment_created_at": "2024-12-04T10:29:31+00:00",
"comment_author": "danielrona",
"comment_body": "Yes, you are right this should be of course a new variable, my mistake I added our hotfix instead of the generic config option.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1869186104",
"pr_number": 53749,
"pr_file": "config/mail.php",
"created_at": "2024-12-04T10:38:39+00:00",
"commented_code": "'mailers' => [\n\n 'smtp' => [\n 'scheme' => env('MAIL_SCHEME', 'smtp'),",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1869186104",
"repo_full_name": "laravel/framework",
"pr_number": 53749,
"pr_file": "config/mail.php",
"discussion_id": "1869186104",
"commented_code": "@@ -38,6 +38,7 @@\n 'mailers' => [\n \n 'smtp' => [\n+ 'scheme' => env('MAIL_SCHEME', 'smtp'),",
"comment_created_at": "2024-12-04T10:38:39+00:00",
"comment_author": "crynobone",
"comment_body": "This will make `scheme` default to `smtp` instead of relying on `port` value as indicated in #53585\r\n\r\nWith this changes everyone using the following configuration:\r\n\r\n```ini\r\nMAIL_ENCRYPTION=tls\r\nMAIL_PORT=465\r\n```\r\n\r\nWill resolved to `stmp` instead correctly using `smtps` and create more gotcha than what we correctly have with `MAIL_ENCRYPTION`.",
"pr_file_module": null
},
{
"comment_id": "1869237308",
"repo_full_name": "laravel/framework",
"pr_number": 53749,
"pr_file": "config/mail.php",
"discussion_id": "1869186104",
"commented_code": "@@ -38,6 +38,7 @@\n 'mailers' => [\n \n 'smtp' => [\n+ 'scheme' => env('MAIL_SCHEME', 'smtp'),",
"comment_created_at": "2024-12-04T11:01:21+00:00",
"comment_author": "danielrona",
"comment_body": "Right now everyone who has configured ```MAIL_ENCRYPTION=null``` has a broken configuration.\r\n\r\nI also do not understand why we would be relying on a port to define a scheme that makes no sense.\r\n\r\nJust because a port can be smtp or smtps doesn't mean it needs to be, \r\nI can have port 25 be smpts or I can have port 587 be smtp this is totally up to the setup of the provider.\r\n\r\nYes the defaults would be smtp for 25 and smtps for 587 not to mention that 465 is a legacy default port for smtps nowadays it would/should be 587. \r\n\r\nSo right now this would still end up causing issues for most people and having a dedicated configuration parameter would resolve this.\r\n\r\nYou can setup a fresh laravel installation and it will fail due to not having the proper config entries.\r\n\r\n```env\r\nMAIL_MAILER=smtp\r\nMAIL_HOST=127.0.0.1\r\nMAIL_PORT=9925\r\nMAIL_USERNAME=null\r\nMAIL_PASSWORD=null\r\nMAIL_ENCRYPTION=null\r\nMAIL_FROM_ADDRESS=\"hello@example.com\"\r\nMAIL_FROM_NAME=\"${APP_NAME}\"\r\n``` \r\nTestMail is a empty view Mailabable\r\n\r\n```php\r\nMail::to('test@localhost')->send(new TestMail());\r\n```\r\nwill fail with \r\n`The \"\" scheme is not supported; supported schemes for mailer \"smtp\" are: \"smtp\", \"smtps\".`",
"pr_file_module": null
},
{
"comment_id": "1869272592",
"repo_full_name": "laravel/framework",
"pr_number": 53749,
"pr_file": "config/mail.php",
"discussion_id": "1869186104",
"commented_code": "@@ -38,6 +38,7 @@\n 'mailers' => [\n \n 'smtp' => [\n+ 'scheme' => env('MAIL_SCHEME', 'smtp'),",
"comment_created_at": "2024-12-04T11:19:16+00:00",
"comment_author": "crynobone",
"comment_body": "> I also do not understand why we would be relying on a port to define a scheme that makes no sense.\r\n\r\nThis is a requirement by Symfony Mailer. `scheme` here is only used to comply with `Dsn` instance but the result is then passed to `EsmtpTransport` differently. \r\n\r\nhttps://github.com/symfony/mailer/blob/e4d358702fb66e4c8a2af08e90e7271a62de39cc/Transport/Smtp/EsmtpTransport.php#L56-L68\r\n\r\n> The \"\" scheme is not supported; supported schemes for mailer \"smtp\" are: \"smtp\", \"smtps\".\r\n\r\nThe fixed in https://github.com/laravel/framework/pull/53585 has already fixed this and before the release can be done you should downgrade to 7.1 as suggested.\r\n\r\nWe will look into removing `MAIL_ENCRYPTION` and streamlining the configuration for next major version if possible.\r\n",
"pr_file_module": null
},
{
"comment_id": "1869424105",
"repo_full_name": "laravel/framework",
"pr_number": 53749,
"pr_file": "config/mail.php",
"discussion_id": "1869186104",
"commented_code": "@@ -38,6 +38,7 @@\n 'mailers' => [\n \n 'smtp' => [\n+ 'scheme' => env('MAIL_SCHEME', 'smtp'),",
"comment_created_at": "2024-12-04T12:42:14+00:00",
"comment_author": "danielrona",
"comment_body": "while #53585 might fix this for everyone using port 465 , this doesn't make much sense and it's still a breaking change because everyone with a port != 465 will have not ```smtps``` set on their configuration files while actually having ```MAIL_ENCRYPTION=tls```",
"pr_file_module": null
},
{
"comment_id": "1869441169",
"repo_full_name": "laravel/framework",
"pr_number": 53749,
"pr_file": "config/mail.php",
"discussion_id": "1869186104",
"commented_code": "@@ -38,6 +38,7 @@\n 'mailers' => [\n \n 'smtp' => [\n+ 'scheme' => env('MAIL_SCHEME', 'smtp'),",
"comment_created_at": "2024-12-04T12:53:58+00:00",
"comment_author": "crynobone",
"comment_body": "https://github.com/symfony/mailer/blob/7.2/CHANGELOG.md#440\n\n`encryption` configuration was deprecated and removed in earlier `symfony/mailer` release. This probably being left out from `swiftmailer` era",
"pr_file_module": null
},
{
"comment_id": "1869587190",
"repo_full_name": "laravel/framework",
"pr_number": 53749,
"pr_file": "config/mail.php",
"discussion_id": "1869186104",
"commented_code": "@@ -38,6 +38,7 @@\n 'mailers' => [\n \n 'smtp' => [\n+ 'scheme' => env('MAIL_SCHEME', 'smtp'),",
"comment_created_at": "2024-12-04T14:02:47+00:00",
"comment_author": "danielrona",
"comment_body": "I adjusted my pull dropped the config changes and this would solve the issue, until ```MAIL_ENCRYPTION``` is removed and the configuration file streamlined with the next major version.",
"pr_file_module": null
}
]
}
]

259
_reviewers/framework-design-flexible-apis.md Normal file
View File

@@ -0,0 +1,259 @@
---
title: Design flexible APIs
description: 'When designing APIs, prioritize flexibility and developer experience
by:
1. **Accept broader parameter types** - Use `callable` instead of `Closure` to support
various invocation patterns:'
repository: laravel/framework
label: API
language: PHP
comments_count: 5
repository_stars: 33763
---
When designing APIs, prioritize flexibility and developer experience by:
1. **Accept broader parameter types** - Use `callable` instead of `Closure` to support various invocation patterns:
```php
// Instead of this (restrictive):
public function throw(?Closure $callback = null)
// Do this (flexible):
public function throw(?callable $callback = null)
```
2. **Support fluent method chaining** - Allow methods to return the instance for chainable calls:
```php
// Example from date formatting:
public function format(string $format): static
{
$this->format = $format;
return $this;
}
// Usage:
$date->format('Y-m-d')->after('2023-01-01');
```
3. **Accept multiple parameter formats** - Make your API handle different input types intelligently:
```php
// Example with status code handling:
if (is_numeric($callback) || is_string($callback) || is_array($callback)) {
$callback = static::response($callback);
}
```
4. **Use enums for constrained options** - Instead of arbitrary integers, use typed enums:
```php
// Instead of:
public static function query($array, $encodingType = PHP_QUERY_RFC3986)
// Do this:
public static function query($array, HttpQueryEncoding $encodingType = HttpQueryEncoding::Rfc3986)
```
Flexible APIs improve developer experience by being intuitive, reducing errors, and accommodating different coding styles while maintaining robustness and clarity.
[
{
"discussion_id": "909053006",
"pr_number": 42976,
"pr_file": "src/Illuminate/Foundation/Console/Kernel.php",
"created_at": "2022-06-28T22:30:59+00:00",
"commented_code": "* Register a Closure based command with the application.\n *\n * @param string $signature\n * @param \\Closure $callback\n * @param \\Closure|callable $callback\n * @return \\Illuminate\\Foundation\\Console\\ClosureCommand\n */\n public function command($signature, Closure $callback)\n public function command($signature, $callback)\n {\n $command = new ClosureCommand($signature, $callback);\n $command = $callback instanceof Closure\n ? new ClosureCommand($signature, $callback)\n : new CallableCommand($signature, $callback);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "909053006",
"repo_full_name": "laravel/framework",
"pr_number": 42976,
"pr_file": "src/Illuminate/Foundation/Console/Kernel.php",
"discussion_id": "909053006",
"commented_code": "@@ -185,12 +185,14 @@ protected function commands()\n * Register a Closure based command with the application.\n *\n * @param string $signature\n- * @param \\Closure $callback\n+ * @param \\Closure|callable $callback\n * @return \\Illuminate\\Foundation\\Console\\ClosureCommand\n */\n- public function command($signature, Closure $callback)\n+ public function command($signature, $callback)\n {\n- $command = new ClosureCommand($signature, $callback);\n+ $command = $callback instanceof Closure\n+ ? new ClosureCommand($signature, $callback)\n+ : new CallableCommand($signature, $callback);",
"comment_created_at": "2022-06-28T22:30:59+00:00",
"comment_author": "ziadoz",
"comment_body": "If you wrapped the callable in a closure here, would you be able to get rid of the `CallableCommand` class?\r\n\r\n```suggestion\r\n if (! $callback instanceof Closure && is_callable($callback)) {\r\n $callback = Closure::fromCallable($callback);\r\n }\r\n\r\n $command = new ClosureCommand($signature, $callback)\r\n```",
"pr_file_module": null
},
{
"comment_id": "920243479",
"repo_full_name": "laravel/framework",
"pr_number": 42976,
"pr_file": "src/Illuminate/Foundation/Console/Kernel.php",
"discussion_id": "909053006",
"commented_code": "@@ -185,12 +185,14 @@ protected function commands()\n * Register a Closure based command with the application.\n *\n * @param string $signature\n- * @param \\Closure $callback\n+ * @param \\Closure|callable $callback\n * @return \\Illuminate\\Foundation\\Console\\ClosureCommand\n */\n- public function command($signature, Closure $callback)\n+ public function command($signature, $callback)\n {\n- $command = new ClosureCommand($signature, $callback);\n+ $command = $callback instanceof Closure\n+ ? new ClosureCommand($signature, $callback)\n+ : new CallableCommand($signature, $callback);",
"comment_created_at": "2022-07-13T15:50:36+00:00",
"comment_author": "chu121su12",
"comment_body": "I tried this logic and the code here\r\n\r\nhttps://github.com/laravel/framework/blob/1f77f3d586debda17be6a886b172e7336d65bb60/src/Illuminate/Foundation/Console/CallableCommand.php#L34-L77\r\n\r\nis quite difficult to be spliced around the section in the `Console::command` suggested.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1715170843",
"pr_number": 52466,
"pr_file": "src/Illuminate/Http/Client/Response.php",
"created_at": "2024-08-13T11:57:14+00:00",
"commented_code": "/**\n * Throw an exception if a server or client error occurred.\n *\n * @param \\Closure|null $callback\n * @return $this\n *\n * @throws \\Illuminate\\Http\\Client\\RequestException\n */\n public function throw()\n public function throw(?Closure $callback = null)",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1715170843",
"repo_full_name": "laravel/framework",
"pr_number": 52466,
"pr_file": "src/Illuminate/Http/Client/Response.php",
"discussion_id": "1715170843",
"commented_code": "@@ -277,17 +278,16 @@ public function toException()\n /**\n * Throw an exception if a server or client error occurred.\n *\n+ * @param \\Closure|null $callback\n * @return $this\n *\n * @throws \\Illuminate\\Http\\Client\\RequestException\n */\n- public function throw()\n+ public function throw(?Closure $callback = null)",
"comment_created_at": "2024-08-13T11:57:14+00:00",
"comment_author": "rodrigopedra",
"comment_body": "I'd use `?callable` instead of `?Closure` to allow for invokable objects, and other callable (such as denoted by the array syntax).\r\n\r\n```php\r\npublic function throw(?callable $callback = null)\r\n```\r\n\r\nP.S.: don't forget to update the docblock",
"pr_file_module": null
},
{
"comment_id": "1715219915",
"repo_full_name": "laravel/framework",
"pr_number": 52466,
"pr_file": "src/Illuminate/Http/Client/Response.php",
"discussion_id": "1715170843",
"commented_code": "@@ -277,17 +278,16 @@ public function toException()\n /**\n * Throw an exception if a server or client error occurred.\n *\n+ * @param \\Closure|null $callback\n * @return $this\n *\n * @throws \\Illuminate\\Http\\Client\\RequestException\n */\n- public function throw()\n+ public function throw(?Closure $callback = null)",
"comment_created_at": "2024-08-13T12:36:10+00:00",
"comment_author": "alirezadp10",
"comment_body": "Thanks for your help.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1997723700",
"pr_number": 55033,
"pr_file": "src/Illuminate/Validation/Rules/Date.php",
"created_at": "2025-03-16T21:31:20+00:00",
"commented_code": "*/\n public function format(string $format): static\n {\n array_shift($this->constraints);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1997723700",
"repo_full_name": "laravel/framework",
"pr_number": 55033,
"pr_file": "src/Illuminate/Validation/Rules/Date.php",
"discussion_id": "1997723700",
"commented_code": "@@ -22,6 +22,8 @@ class Date implements Stringable\n */\n public function format(string $format): static\n {\n+ array_shift($this->constraints);",
"comment_created_at": "2025-03-16T21:31:20+00:00",
"comment_author": "AndrewMast",
"comment_body": "I propose we add a `$format` property (`protected ?string $format = null;`) and make the `format` method simply do:\r\n\r\n```php\r\n/**\r\n * Ensure the date has the given format.\r\n */\r\npublic function format(string $format): static\r\n{\r\n $this->format = $format;\r\n\r\n return $this;\r\n}\r\n```\r\n\r\nThen, in the `__toString` method, we can simply do:\r\n```php\r\n/**\r\n * Convert the rule to a validation string.\r\n */\r\npublic function __toString(): string\r\n{\r\n return implode('|', [\r\n $this->format === null ? 'date' : 'date_format:'.$this->format,\r\n ...$this->constraints,\r\n ]);\r\n}\r\n```\r\n\r\nAnd in `formatDate()`:\r\n```php\r\n/**\r\n * Format the date for the validation rule.\r\n */\r\nprotected function formatDate(DateTimeInterface|string $date): string\r\n{\r\n return $date instanceof DateTimeInterface\r\n ? $date->format($this->format ?? 'Y-m-d')\r\n : $date;\r\n}\r\n```\r\n\r\nWe would also need to make the `$constraints` property start out empty:\r\n```php\r\nprotected ?string $format = null;\r\n\r\nprotected array $constraints = [];\r\n```\r\n\r\nOne downside to this approach is this would require `format` to be called before `after()` or `before()`, but the only way I can think of to solve this would be to include the DateTime instances in the `$constraints` array and only format them in the `__toString()` method, but that would require a rather large rewrite of this class and wouldn't seem very straight forward.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1860845192",
"pr_number": 53663,
"pr_file": "src/Illuminate/Http/Client/Factory.php",
"created_at": "2024-11-27T15:16:42+00:00",
"commented_code": "return;\n }\n\n return $callback instanceof Closure || $callback instanceof ResponseSequence\n ? $callback($request, $options)\n : $callback;\n if (is_int($callback) && $callback >= 100 && $callback < 600) {\n return static::response(status: $callback);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1860845192",
"repo_full_name": "laravel/framework",
"pr_number": 53663,
"pr_file": "src/Illuminate/Http/Client/Factory.php",
"discussion_id": "1860845192",
"commented_code": "@@ -267,9 +267,19 @@ public function stubUrl($url, $callback)\n return;\n }\n \n- return $callback instanceof Closure || $callback instanceof ResponseSequence\n- ? $callback($request, $options)\n- : $callback;\n+ if (is_int($callback) && $callback >= 100 && $callback < 600) {\n+ return static::response(status: $callback);",
"comment_created_at": "2024-11-27T15:16:42+00:00",
"comment_author": "andrey-helldar",
"comment_body": "Wow. I took another look at the code and was very surprised!\r\n\r\n@jasonmccreary, okay, let's say `200` is the status code and `'204'` is the response body.\r\n\r\nBut what is the difference between, for example, `500` and `600`? Here `500` is the **status code**, and `600` is the **response body** \ud83d\ude00\r\n\r\n@taylorotwell, you'll be interested too (I wrote my thoughts above, and here's an addendum).\r\n\r\n```php\r\nHttp::fake([\r\n 'google.com' => 'Hello World', // no problem\r\n 'github.com' => ['foo' => 'bar'], // no problem\r\n\r\n 'bar.laravel.com' => '204', // it's a response body with status code 200\r\n\r\n 'foo.laravel.com' => 99, // it's a response body with status code 200\r\n 'foo.laravel.com' => 100, // it's a status code 100\r\n 'foo.laravel.com' => 204, // it's a status code 204\r\n 'foo.laravel.com' => 204.99, // it's a response body with status code 200\r\n 'foo.laravel.com' => 599, // it's a status code 599\r\n 'foo.laravel.com' => 600, // it's a response body with status code 200\r\n]);\r\n```",
"pr_file_module": null
},
{
"comment_id": "1860904761",
"repo_full_name": "laravel/framework",
"pr_number": 53663,
"pr_file": "src/Illuminate/Http/Client/Factory.php",
"discussion_id": "1860845192",
"commented_code": "@@ -267,9 +267,19 @@ public function stubUrl($url, $callback)\n return;\n }\n \n- return $callback instanceof Closure || $callback instanceof ResponseSequence\n- ? $callback($request, $options)\n- : $callback;\n+ if (is_int($callback) && $callback >= 100 && $callback < 600) {\n+ return static::response(status: $callback);",
"comment_created_at": "2024-11-27T15:52:20+00:00",
"comment_author": "andrey-helldar",
"comment_body": "My suggestion:\r\n\r\n```php\r\npublic function stubUrl($url, $callback)\r\n{\r\n return $this->fake(function ($request, $options) use ($url, $callback) {\r\n if (! Str::is(Str::start($url, '*'), $request->url())) {\r\n return;\r\n }\r\n\r\n if ($callback instanceof Closure || $callback instanceof ResponseSequence) {\r\n return $callback($request, $options);\r\n }\r\n\r\n if (is_numeric($callback) || is_string($callback) || is_array($callback)) { // here\r\n return static::response($callback);\r\n }\r\n\r\n return $callback;\r\n });\r\n}\r\n```",
"pr_file_module": null
},
{
"comment_id": "1860983198",
"repo_full_name": "laravel/framework",
"pr_number": 53663,
"pr_file": "src/Illuminate/Http/Client/Factory.php",
"discussion_id": "1860845192",
"commented_code": "@@ -267,9 +267,19 @@ public function stubUrl($url, $callback)\n return;\n }\n \n- return $callback instanceof Closure || $callback instanceof ResponseSequence\n- ? $callback($request, $options)\n- : $callback;\n+ if (is_int($callback) && $callback >= 100 && $callback < 600) {\n+ return static::response(status: $callback);",
"comment_created_at": "2024-11-27T16:46:20+00:00",
"comment_author": "shaedrich",
"comment_body": "What about just wrapping the callback? This would be following the original train of thought of this PR:\r\n```diff\r\npublic function stubUrl($url, $callback)\r\n{\r\n return $this->fake(function ($request, $options) use ($url, $callback) {\r\n if (! Str::is(Str::start($url, '*'), $request->url())) {\r\n return;\r\n }\r\n\r\n+ if (is_numeric($callback) || is_string($callback) || is_array($callback) {\r\n+ $callback = static::response($callback);\r\n+ }\r\n\r\n if ($callback instanceof Closure || $callback instanceof ResponseSequence) {\r\n return $callback($request, $options);\r\n }\r\n-\r\n- if (is_numeric($callback) || is_string($callback) || is_array($callback)) { // here\r\n- return static::response($callback);\r\n- }\r\n\r\n return $callback;\r\n });\r\n}\r\n```",
"pr_file_module": null
},
{
"comment_id": "1861197986",
"repo_full_name": "laravel/framework",
"pr_number": 53663,
"pr_file": "src/Illuminate/Http/Client/Factory.php",
"discussion_id": "1860845192",
"commented_code": "@@ -267,9 +267,19 @@ public function stubUrl($url, $callback)\n return;\n }\n \n- return $callback instanceof Closure || $callback instanceof ResponseSequence\n- ? $callback($request, $options)\n- : $callback;\n+ if (is_int($callback) && $callback >= 100 && $callback < 600) {\n+ return static::response(status: $callback);",
"comment_created_at": "2024-11-27T20:09:55+00:00",
"comment_author": "andrey-helldar",
"comment_body": "Yeah, you can do that too :)",
"pr_file_module": null
},
{
"comment_id": "1861200387",
"repo_full_name": "laravel/framework",
"pr_number": 53663,
"pr_file": "src/Illuminate/Http/Client/Factory.php",
"discussion_id": "1860845192",
"commented_code": "@@ -267,9 +267,19 @@ public function stubUrl($url, $callback)\n return;\n }\n \n- return $callback instanceof Closure || $callback instanceof ResponseSequence\n- ? $callback($request, $options)\n- : $callback;\n+ if (is_int($callback) && $callback >= 100 && $callback < 600) {\n+ return static::response(status: $callback);",
"comment_created_at": "2024-11-27T20:12:51+00:00",
"comment_author": "shaedrich",
"comment_body": "Oh, I misread `ResponseSequence` as `Response` \ud83e\udd26\ud83c\udffb\u200d\u2642\ufe0f Your suggestion makes more sense then \ud83d\ude05 ",
"pr_file_module": null
}
]
},
{
"discussion_id": "1941509492",
"pr_number": 54464,
"pr_file": "src/Illuminate/Collections/Arr.php",
"created_at": "2025-02-04T16:28:36+00:00",
"commented_code": "* Convert the array into a query string.\n *\n * @param array $array\n * @param int-mask-of<PHP_QUERY_*> $encodingType (optional) Query encoding type.\n * @return string\n */\n public static function query($array)\n public static function query($array, $encodingType = PHP_QUERY_RFC3986)\n {\n return http_build_query($array, '', '&', PHP_QUERY_RFC3986);\n return http_build_query($array, '', '&', $encodingType);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1941509492",
"repo_full_name": "laravel/framework",
"pr_number": 54464,
"pr_file": "src/Illuminate/Collections/Arr.php",
"discussion_id": "1941509492",
"commented_code": "@@ -702,11 +702,12 @@ public static function pull(&$array, $key, $default = null)\n * Convert the array into a query string.\n *\n * @param array $array\n+ * @param int-mask-of<PHP_QUERY_*> $encodingType (optional) Query encoding type.\n * @return string\n */\n- public static function query($array)\n+ public static function query($array, $encodingType = PHP_QUERY_RFC3986)\n {\n- return http_build_query($array, '', '&', PHP_QUERY_RFC3986);\n+ return http_build_query($array, '', '&', $encodingType);",
"comment_created_at": "2025-02-04T16:28:36+00:00",
"comment_author": "shaedrich",
"comment_body": "> I've added all the changes except your enum idea because I merged one in early by accident and I can't merge that in now due to it being classed as outdated. Could you suggest that change again and I'll pull that in\r\n\r\nRe-adding the enum suggestion from https://github.com/laravel/framework/pull/54464#discussion_r1941450711 as requested per https://github.com/laravel/framework/pull/54464#issuecomment-2634468189\r\n```suggestion\r\n * @param int-mask-of<PHP_QUERY_*>|HttpQueryEncoding $encodingType (optional) Query encoding type.\r\n * @return string\r\n */\r\n public static function query($array, $encodingType = HttpQueryEncoding::Rfc3986)\r\n {\r\n return http_build_query($array, '', '&', enum_value($encodingType))\r\n```",
"pr_file_module": null
},
{
"comment_id": "1941516190",
"repo_full_name": "laravel/framework",
"pr_number": 54464,
"pr_file": "src/Illuminate/Collections/Arr.php",
"discussion_id": "1941509492",
"commented_code": "@@ -702,11 +702,12 @@ public static function pull(&$array, $key, $default = null)\n * Convert the array into a query string.\n *\n * @param array $array\n+ * @param int-mask-of<PHP_QUERY_*> $encodingType (optional) Query encoding type.\n * @return string\n */\n- public static function query($array)\n+ public static function query($array, $encodingType = PHP_QUERY_RFC3986)\n {\n- return http_build_query($array, '', '&', PHP_QUERY_RFC3986);\n+ return http_build_query($array, '', '&', $encodingType);",
"comment_created_at": "2025-02-04T16:32:20+00:00",
"comment_author": "shaedrich",
"comment_body": "Please keep in mind, that you have to manually\r\n* import `enum_value` via `use function Illuminate\\Support\\enum_value;`\r\n* create said enum as\r\n ```php\r\n <?php\r\n\r\n namespace Illuminate\\Http\\Request\\Enums;\r\n\r\n enum HttpQueryEncoding: int\r\n {\r\n case Rfc3986 = PHP_QUERY_RFC3986;\r\n case Rfc1738 = PHP_QUERY_RFC1738;\r\n }\r\n ```",
"pr_file_module": null
}
]
}
]

119
_reviewers/framework-disable-coverage-in-workflows.md Normal file
View File

@@ -0,0 +1,119 @@
---
title: Disable coverage in workflows
description: Keep code coverage tools disabled in CI/CD workflows unless they're specifically
needed for generating coverage reports. Enabling coverage tools like xdebug in GitHub
Actions can significantly slow down pipeline execution without providing value when
reports aren't being collected or analyzed.
repository: laravel/framework
label: Testing
language: Yaml
comments_count: 4
repository_stars: 33763
---
Keep code coverage tools disabled in CI/CD workflows unless they're specifically needed for generating coverage reports. Enabling coverage tools like xdebug in GitHub Actions can significantly slow down pipeline execution without providing value when reports aren't being collected or analyzed.
When configuring PHP environments in workflows, explicitly set `coverage: none` and avoid including coverage extensions like xdebug unless they serve a specific purpose:
```yaml
uses: shivammathur/setup-php@v2
with:
php-version: 8.2
extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr
tools: composer:v2
coverage: none # Keep coverage disabled for better performance
```
Reserve coverage tools for dedicated testing jobs that specifically generate and process coverage reports. This helps maintain faster CI pipelines while still allowing coverage analysis when needed.
[
{
"discussion_id": "1856408580",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"created_at": "2024-11-25T11:05:37+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n coverage: none\n coverage: xdebug",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856408580",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"discussion_id": "1856408580",
"commented_code": "@@ -147,9 +147,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n- coverage: none\n+ coverage: xdebug",
"comment_created_at": "2024-11-25T11:05:37+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n coverage: none\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856409108",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"created_at": "2024-11-25T11:06:02+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n coverage: none\n coverage: xdebug",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856409108",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"discussion_id": "1856409108",
"commented_code": "@@ -107,9 +107,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n- coverage: none\n+ coverage: xdebug",
"comment_created_at": "2024-11-25T11:06:02+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n coverage: none\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856409578",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"created_at": "2024-11-25T11:06:21+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n coverage: none\n coverage: xdebug",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856409578",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"discussion_id": "1856409578",
"commented_code": "@@ -59,9 +59,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n- coverage: none\n+ coverage: xdebug",
"comment_created_at": "2024-11-25T11:06:21+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n coverage: none\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856413883",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"created_at": "2024-11-25T11:09:22+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n coverage: none\n coverage: xdebug",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856413883",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"discussion_id": "1856413883",
"commented_code": "@@ -35,9 +35,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n- coverage: none\n+ coverage: xdebug",
"comment_created_at": "2024-11-25T11:09:22+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n coverage: none\r\n```",
"pr_file_module": null
}
]
}
]

167
_reviewers/framework-escape-column-names-properly.md Normal file
View File

@@ -0,0 +1,167 @@
---
title: Escape column names properly
description: 'Always use the appropriate wrapping/escaping mechanism when generating
SQL to prevent syntax errors and SQL injection vulnerabilities, especially when
handling:'
repository: laravel/framework
label: Database
language: PHP
comments_count: 5
repository_stars: 33763
---
Always use the appropriate wrapping/escaping mechanism when generating SQL to prevent syntax errors and SQL injection vulnerabilities, especially when handling:
1. **Functional expressions in columns**: When dealing with columns that contain SQL functions or expressions, use a detection method before applying escaping:
```php
protected static function isFunctionalExpression(string $column): bool
{
return preg_match('/\(.+\)/', $column);
}
// Usage when building queries
$columns = collect($command->columns)
->map(fn (string $column) => self::isFunctionalExpression($column) ? $column : $this->wrap($column))
->implode(', ');
```
2. **Column names that are SQL keywords**: Database column names might conflict with SQL reserved keywords (like 'create', 'table', 'order'). Always properly wrap these identifiers:
```php
// DON'T rely on plain string interpolation
$query = "SELECT $column FROM users";
// DO use the query builder or wrap method
$query = $this->builder->select($this->wrap($column))->from('users');
```
3. **Cross-database compatibility**: Different database systems use different identifier quoting (MySQL uses backticks, PostgreSQL uses double quotes). Always use the database grammar's `wrap()` method instead of hardcoding quote characters.
This practice prevents SQL errors when working with reserved words as column names, functional expressions in indexes, and ensures consistent behavior across different database systems.
[
{
"discussion_id": "1724014845",
"pr_number": 52535,
"pr_file": "src/Illuminate/Database/Query/Grammars/MySqlGrammar.php",
"created_at": "2024-08-20T21:50:45+00:00",
"commented_code": "return 'json_extract('.$field.$path.')';\n }\n\n /**\n * Apply custom ordering to a query based on a priority array.\n *\n * @param $column\n * @param array $priority\n * @param $direction\n * @return string\n */\n public function orderByPriority($column, array $priority, $direction = 'asc')\n {\n $placeholders = implode(',', array_fill(0, count($priority), '?'));\n\n return \"FIELD($column, $placeholders) $direction\";",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1724014845",
"repo_full_name": "laravel/framework",
"pr_number": 52535,
"pr_file": "src/Illuminate/Database/Query/Grammars/MySqlGrammar.php",
"discussion_id": "1724014845",
"commented_code": "@@ -524,4 +524,19 @@ protected function wrapJsonBooleanSelector($value)\n \n return 'json_extract('.$field.$path.')';\n }\n+\n+ /**\n+ * Apply custom ordering to a query based on a priority array.\n+ *\n+ * @param $column\n+ * @param array $priority\n+ * @param $direction\n+ * @return string\n+ */\n+ public function orderByPriority($column, array $priority, $direction = 'asc')\n+ {\n+ $placeholders = implode(',', array_fill(0, count($priority), '?'));\n+\n+ return \"FIELD($column, $placeholders) $direction\";",
"comment_created_at": "2024-08-20T21:50:45+00:00",
"comment_author": "staudenmeir",
"comment_body": "The column name needs to be wrapped with `$this->wrap($column)`. This also affects the other grammars.",
"pr_file_module": null
},
{
"comment_id": "1724361191",
"repo_full_name": "laravel/framework",
"pr_number": 52535,
"pr_file": "src/Illuminate/Database/Query/Grammars/MySqlGrammar.php",
"discussion_id": "1724014845",
"commented_code": "@@ -524,4 +524,19 @@ protected function wrapJsonBooleanSelector($value)\n \n return 'json_extract('.$field.$path.')';\n }\n+\n+ /**\n+ * Apply custom ordering to a query based on a priority array.\n+ *\n+ * @param $column\n+ * @param array $priority\n+ * @param $direction\n+ * @return string\n+ */\n+ public function orderByPriority($column, array $priority, $direction = 'asc')\n+ {\n+ $placeholders = implode(',', array_fill(0, count($priority), '?'));\n+\n+ return \"FIELD($column, $placeholders) $direction\";",
"comment_created_at": "2024-08-21T04:21:26+00:00",
"comment_author": "Mushood",
"comment_body": "I have applied the wrap function on all grammars and updated the tests accordingly.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1898688984",
"pr_number": 54025,
"pr_file": "src/Illuminate/Database/Schema/Grammars/MySqlGrammar.php",
"created_at": "2024-12-27T19:45:49+00:00",
"commented_code": "*/\n protected function compileKey(Blueprint $blueprint, Fluent $command, $type)\n {\n return sprintf('alter table %s add %s %s%s(%s)',\n $columns = collect($command->columns)->map(function ($column) {\n // Check if the column contains a functional expression\n // and return as-is, otherwise wrap\n return preg_match('/\\(.+\\)/', $column) ? $column : $this->wrap($column);\n })->implode(', ');\n\n return sprintf(\n 'alter table %s add %s %s%s(%s)',\n $this->wrapTable($blueprint),\n $type,\n $this->wrap($command->index),\n $command->algorithm ? ' using '.$command->algorithm : '',\n $this->columnize($command->columns)\n $command->algorithm ? ' using ' . $command->algorithm : '',\n $columns\n );\n }",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1898688984",
"repo_full_name": "laravel/framework",
"pr_number": 54025,
"pr_file": "src/Illuminate/Database/Schema/Grammars/MySqlGrammar.php",
"discussion_id": "1898688984",
"commented_code": "@@ -492,12 +492,19 @@ public function compileSpatialIndex(Blueprint $blueprint, Fluent $command)\n */\n protected function compileKey(Blueprint $blueprint, Fluent $command, $type)\n {\n- return sprintf('alter table %s add %s %s%s(%s)',\n+ $columns = collect($command->columns)->map(function ($column) {\n+ // Check if the column contains a functional expression\n+ // and return as-is, otherwise wrap\n+ return preg_match('/\\(.+\\)/', $column) ? $column : $this->wrap($column);\n+ })->implode(', ');\n+\n+ return sprintf(\n+ 'alter table %s add %s %s%s(%s)',\n $this->wrapTable($blueprint),\n $type,\n $this->wrap($command->index),\n- $command->algorithm ? ' using '.$command->algorithm : '',\n- $this->columnize($command->columns)\n+ $command->algorithm ? ' using ' . $command->algorithm : '',\n+ $columns\n );\n }\n ",
"comment_created_at": "2024-12-27T19:45:49+00:00",
"comment_author": "shaedrich",
"comment_body": "One possible solution for https://github.com/laravel/framework/pull/54025#discussion_r1898460480\r\n```suggestion\r\n\r\n protected static function isFunctionalExpression(string $column): bool\r\n {\r\n return preg_match('/\\(.+\\)/', $column);\r\n }\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1898689872",
"pr_number": 54025,
"pr_file": "src/Illuminate/Database/Schema/Grammars/MySqlGrammar.php",
"created_at": "2024-12-27T19:48:17+00:00",
"commented_code": "*/\n protected function compileKey(Blueprint $blueprint, Fluent $command, $type)\n {\n return sprintf('alter table %s add %s %s%s(%s)',\n $columns = collect($command->columns)->map(function ($column) {\n // Check if the column contains a functional expression\n // and return as-is, otherwise wrap\n return preg_match('/\\(.+\\)/', $column) ? $column : $this->wrap($column);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1898689872",
"repo_full_name": "laravel/framework",
"pr_number": 54025,
"pr_file": "src/Illuminate/Database/Schema/Grammars/MySqlGrammar.php",
"discussion_id": "1898689872",
"commented_code": "@@ -492,12 +492,19 @@ public function compileSpatialIndex(Blueprint $blueprint, Fluent $command)\n */\n protected function compileKey(Blueprint $blueprint, Fluent $command, $type)\n {\n- return sprintf('alter table %s add %s %s%s(%s)',\n+ $columns = collect($command->columns)->map(function ($column) {\n+ // Check if the column contains a functional expression\n+ // and return as-is, otherwise wrap\n+ return preg_match('/\\(.+\\)/', $column) ? $column : $this->wrap($column);",
"comment_created_at": "2024-12-27T19:48:17+00:00",
"comment_author": "shaedrich",
"comment_body": "https://github.com/laravel/framework/pull/54025#discussion_r1898688984 can be applied here, then:\r\n```suggestion\r\n return self::isFunctionalExpression($column) ? $column : $this->wrap($column);\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1898691810",
"pr_number": 54025,
"pr_file": "src/Illuminate/Database/Schema/Grammars/MySqlGrammar.php",
"created_at": "2024-12-27T19:54:11+00:00",
"commented_code": "*/\n protected function compileKey(Blueprint $blueprint, Fluent $command, $type)\n {\n return sprintf('alter table %s add %s %s%s(%s)',\n $columns = collect($command->columns)->map(function (string $column) {\n return self::isFunctionalExpression($column) ? $column : $this->wrap($column);\n })->implode(', ');",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1898691810",
"repo_full_name": "laravel/framework",
"pr_number": 54025,
"pr_file": "src/Illuminate/Database/Schema/Grammars/MySqlGrammar.php",
"discussion_id": "1898691810",
"commented_code": "@@ -492,15 +492,23 @@ public function compileSpatialIndex(Blueprint $blueprint, Fluent $command)\n */\n protected function compileKey(Blueprint $blueprint, Fluent $command, $type)\n {\n- return sprintf('alter table %s add %s %s%s(%s)',\n+ $columns = collect($command->columns)->map(function (string $column) {\n+ return self::isFunctionalExpression($column) ? $column : $this->wrap($column);\n+ })->implode(', ');",
"comment_created_at": "2024-12-27T19:54:11+00:00",
"comment_author": "shaedrich",
"comment_body": "https://github.com/laravel/framework/pull/54025#discussion_r1898675719 got outdated\u2014not sure if you still want to change that:\r\n```suggestion\r\n $columns = collect($command->columns)\r\n ->map(fn (string $column) => self::isFunctionalExpression($column) ? $column : $this->wrap($column))\r\n ->implode(', ');\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1682404703",
"pr_number": 52177,
"pr_file": "tests/Integration/Database/DatabaseSchemaBlueprintTest.php",
"created_at": "2024-07-18T08:11:43+00:00",
"commented_code": "$this->assertEquals($expected, $queries);\n }\n\n public function testAddColumnNamedCreateWorks()\n {\n DB::connection()->getSchemaBuilder()->create('users', function ($table) {\n $table->string('name')->nullable();\n });\n\n $blueprintMySql = new Blueprint('users', function ($table) {\n $table->string('create')->nullable(false);\n });\n\n $queries = $blueprintMySql->toSql(DB::connection(), new MySqlGrammar);\n\n $expected = [\n 'alter table `users` add `create` varchar(255) not null',\n ];\n\n $this->assertEquals($expected, $queries);\n\n $blueprintPostgres = new Blueprint('users', function ($table) {\n $table->string('create')->nullable(false);\n });\n\n $queries = $blueprintPostgres->toSql(DB::connection(), new PostgresGrammar);\n\n $expected = [\n 'alter table \"users\" add column \"create\" varchar(255) not null',\n ];\n\n $this->assertEquals($expected, $queries);\n\n $blueprintSQLite = new Blueprint('users', function ($table) {\n $table->string('create')->nullable(false);\n });\n\n $queries = $blueprintSQLite->toSql(DB::connection(), new SQLiteGrammar);\n\n $expected = [\n 'alter table \"users\" add column \"create\" varchar not null',\n ];\n\n $this->assertEquals($expected, $queries);\n\n $blueprintSqlServer = new Blueprint('users', function ($table) {\n $table->string('create')->nullable(false);\n });\n\n $queries = $blueprintSqlServer->toSql(DB::connection(), new SqlServerGrammar);\n\n $expected = [\n 'alter table \"users\" add \"create\" nvarchar(255) not null',\n ];\n\n $this->assertEquals($expected, $queries);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1682404703",
"repo_full_name": "laravel/framework",
"pr_number": 52177,
"pr_file": "tests/Integration/Database/DatabaseSchemaBlueprintTest.php",
"discussion_id": "1682404703",
"commented_code": "@@ -507,6 +507,61 @@ public function testAddUniqueIndexWithNameWorks()\n $this->assertEquals($expected, $queries);\n }\n \n+ public function testAddColumnNamedCreateWorks()\n+ {\n+ DB::connection()->getSchemaBuilder()->create('users', function ($table) {\n+ $table->string('name')->nullable();\n+ });\n+\n+ $blueprintMySql = new Blueprint('users', function ($table) {\n+ $table->string('create')->nullable(false);\n+ });\n+\n+ $queries = $blueprintMySql->toSql(DB::connection(), new MySqlGrammar);\n+\n+ $expected = [\n+ 'alter table `users` add `create` varchar(255) not null',\n+ ];\n+\n+ $this->assertEquals($expected, $queries);\n+\n+ $blueprintPostgres = new Blueprint('users', function ($table) {\n+ $table->string('create')->nullable(false);\n+ });\n+\n+ $queries = $blueprintPostgres->toSql(DB::connection(), new PostgresGrammar);\n+\n+ $expected = [\n+ 'alter table \"users\" add column \"create\" varchar(255) not null',\n+ ];\n+\n+ $this->assertEquals($expected, $queries);\n+\n+ $blueprintSQLite = new Blueprint('users', function ($table) {\n+ $table->string('create')->nullable(false);\n+ });\n+\n+ $queries = $blueprintSQLite->toSql(DB::connection(), new SQLiteGrammar);\n+\n+ $expected = [\n+ 'alter table \"users\" add column \"create\" varchar not null',\n+ ];\n+\n+ $this->assertEquals($expected, $queries);\n+\n+ $blueprintSqlServer = new Blueprint('users', function ($table) {\n+ $table->string('create')->nullable(false);\n+ });\n+\n+ $queries = $blueprintSqlServer->toSql(DB::connection(), new SqlServerGrammar);\n+\n+ $expected = [\n+ 'alter table \"users\" add \"create\" nvarchar(255) not null',\n+ ];\n+\n+ $this->assertEquals($expected, $queries);",
"comment_created_at": "2024-07-18T08:11:43+00:00",
"comment_author": "hafezdivandari",
"comment_body": "```suggestion\r\n Schema::create('users', function (Blueprint $table) {\r\n $table->string('name');\r\n });\r\n\r\n Schema::table('users', function (Blueprint $table) {\r\n $table->string('create');\r\n });\r\n\r\n $this->assertTrue(Schema::hasColumn('users', 'create'));\r\n```",
"pr_file_module": null
}
]
}
]

454
_reviewers/framework-explicit-null-handling.md Normal file
View File

@@ -0,0 +1,454 @@
---
title: Explicit null handling
description: 'Use explicit identity comparisons for null checks and leverage modern
PHP null-handling features to create more reliable, readable code.
## Key practices:'
repository: laravel/framework
label: Null Handling
language: PHP
comments_count: 9
repository_stars: 33763
---
Use explicit identity comparisons for null checks and leverage modern PHP null-handling features to create more reliable, readable code.
## Key practices:
### 1. Use identity operators for null checks instead of functions
```php
// Avoid
if (is_null($value)) { ... }
if (empty($path)) { ... }
// Prefer
if ($value === null) { ... }
if ($path === '') { ... }
if ($array === []) { ... }
```
Identity comparisons (`===`, `!==`) are more precise than functions like `is_null()` or `empty()`. They clearly communicate your intent and avoid unexpected behavior with falsy values like `'0'` or `0`.
### 2. Leverage null coalescing operators
```php
// Avoid
$name = (string) (is_null($this->argument('name'))
? $choice
: $this->argument('name'));
// Prefer
$name = (string) ($this->argument('name') ?? $choice);
```
The null coalescing operator (`??`) simplifies conditional logic and makes your code more concise and readable.
### 3. Add proper null annotations in docblocks and types
```php
// Avoid
/** @param string $string The input string to sanitize. */
public static function sanitize($string)
{
if ($string === null) {
return null;
}
}
// Prefer
/** @param string|null $string The input string to sanitize. */
/** @return string|null The sanitized string. */
public static function sanitize(?string $string)
{
if ($string === null) {
return null;
}
}
```
Accurately document nullable parameters and return types for better static analysis and IDE support.
### 4. Check for null before method calls
```php
// Avoid direct method calls on possibly null values
$using($this->app);
// Prefer
if ($using !== null) {
$this->app->call($using);
}
```
Always check if variables are null before calling methods on them to prevent null reference exceptions.
### 5. Be careful with null in array and object relationships
```php
// Check if a relation exists before accessing properties
if (isset($relation)) {
$attributes[$key] = $relation;
}
```
Remember that `isset()` returns false for null values, which may not be what you expect when working with relationships or arrays that might legitimately contain null values.
[
{
"discussion_id": "2072707142",
"pr_number": 55645,
"pr_file": "src/Illuminate/Container/Container.php",
"created_at": "2025-05-04T20:43:00+00:00",
"commented_code": "array_pop($this->buildStack);\n\n if (!empty($reflector->getAttributes(Lazy::class))) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2072707142",
"repo_full_name": "laravel/framework",
"pr_number": 55645,
"pr_file": "src/Illuminate/Container/Container.php",
"discussion_id": "2072707142",
"commented_code": "@@ -1058,8 +1059,16 @@ public function build($concrete)\n \n array_pop($this->buildStack);\n \n+ if (!empty($reflector->getAttributes(Lazy::class))) {",
"comment_created_at": "2025-05-04T20:43:00+00:00",
"comment_author": "shaedrich",
"comment_body": "You could just directly check this\r\n```suggestion\r\n if ($reflector->getAttributes(Lazy::class) !== []) {\r\n```",
"pr_file_module": null
},
{
"comment_id": "2073825274",
"repo_full_name": "laravel/framework",
"pr_number": 55645,
"pr_file": "src/Illuminate/Container/Container.php",
"discussion_id": "2072707142",
"commented_code": "@@ -1058,8 +1059,16 @@ public function build($concrete)\n \n array_pop($this->buildStack);\n \n+ if (!empty($reflector->getAttributes(Lazy::class))) {",
"comment_created_at": "2025-05-05T17:04:20+00:00",
"comment_author": "olekjs",
"comment_body": "Does this change bring anything beyond syntax? I\u2019m just asking out of curiosity.",
"pr_file_module": null
},
{
"comment_id": "2073872455",
"repo_full_name": "laravel/framework",
"pr_number": 55645,
"pr_file": "src/Illuminate/Container/Container.php",
"discussion_id": "2072707142",
"commented_code": "@@ -1058,8 +1059,16 @@ public function build($concrete)\n \n array_pop($this->buildStack);\n \n+ if (!empty($reflector->getAttributes(Lazy::class))) {",
"comment_created_at": "2025-05-05T17:32:05+00:00",
"comment_author": "shaedrich",
"comment_body": "Not much. Technically, a function call is more expensive than a comparison, but this should mostly be optimized away in practice. One _slight_ advantage would be a clear communication that we are working with an array here and don't have to check _any_ structure's emptiness.",
"pr_file_module": null
},
{
"comment_id": "2078948392",
"repo_full_name": "laravel/framework",
"pr_number": 55645,
"pr_file": "src/Illuminate/Container/Container.php",
"discussion_id": "2072707142",
"commented_code": "@@ -1058,8 +1059,16 @@ public function build($concrete)\n \n array_pop($this->buildStack);\n \n+ if (!empty($reflector->getAttributes(Lazy::class))) {",
"comment_created_at": "2025-05-08T06:03:12+00:00",
"comment_author": "macropay-solutions",
"comment_body": "@olekjs empty function is a trap for future bugs.\r\nWe have a rule in place to never use empty function because of that. \r\n\r\nempty('0') is true for example.",
"pr_file_module": null
}
]
},
{
"discussion_id": "2111059728",
"pr_number": 55877,
"pr_file": "src/Illuminate/Foundation/Console/ConfigPublishCommand.php",
"created_at": "2025-05-28T06:45:12+00:00",
"commented_code": "return;\n }\n\n $name = (string) (is_null($this->argument('name')) ? select(\n label: 'Which configuration file would you like to publish?',\n options: (new Collection($config))->map(function (string $path) {\n return basename($path, '.php');\n }),\n ) : $this->argument('name'));\n $choices = array_map(\n fn (string $path) => basename($path, '.php'),\n $config,\n );\n\n $choice = windows_os()\n ? select(\n 'Which configuration file would you like to publish?',\n $choices,\n scroll: 15,\n )\n : search(\n label: 'Which configuration file would you like to publish?',\n placeholder: 'Search...',\n options: fn ($search) => array_values(array_filter(\n $choices,\n fn ($choice) => str_contains(strtolower($choice), strtolower($search))\n )),\n scroll: 15,\n );\n\n $name = (string) (is_null($this->argument('name'))\n ? $choice\n : $this->argument('name'));",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2111059728",
"repo_full_name": "laravel/framework",
"pr_number": 55877,
"pr_file": "src/Illuminate/Foundation/Console/ConfigPublishCommand.php",
"discussion_id": "2111059728",
"commented_code": "@@ -46,12 +47,30 @@ public function handle()\n return;\n }\n \n- $name = (string) (is_null($this->argument('name')) ? select(\n- label: 'Which configuration file would you like to publish?',\n- options: (new Collection($config))->map(function (string $path) {\n- return basename($path, '.php');\n- }),\n- ) : $this->argument('name'));\n+ $choices = array_map(\n+ fn (string $path) => basename($path, '.php'),\n+ $config,\n+ );\n+\n+ $choice = windows_os()\n+ ? select(\n+ 'Which configuration file would you like to publish?',\n+ $choices,\n+ scroll: 15,\n+ )\n+ : search(\n+ label: 'Which configuration file would you like to publish?',\n+ placeholder: 'Search...',\n+ options: fn ($search) => array_values(array_filter(\n+ $choices,\n+ fn ($choice) => str_contains(strtolower($choice), strtolower($search))\n+ )),\n+ scroll: 15,\n+ );\n+\n+ $name = (string) (is_null($this->argument('name'))\n+ ? $choice\n+ : $this->argument('name'));",
"comment_created_at": "2025-05-28T06:45:12+00:00",
"comment_author": "shaedrich",
"comment_body": "Wouldn't this suffice?\r\n```suggestion\r\n $name = (string) ($this->argument('name') ?? $choice);\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "2102660306",
"pr_number": 55810,
"pr_file": "src/Illuminate/Filesystem/FilesystemAdapter.php",
"created_at": "2025-05-22T14:05:37+00:00",
"commented_code": "*/\n protected function concatPathToUrl($url, $path)\n {\n if (empty($url) || empty($path)) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2102660306",
"repo_full_name": "laravel/framework",
"pr_number": 55810,
"pr_file": "src/Illuminate/Filesystem/FilesystemAdapter.php",
"discussion_id": "2102660306",
"commented_code": "@@ -847,6 +847,10 @@ public function temporaryUploadUrl($path, $expiration, array $options = [])\n */\n protected function concatPathToUrl($url, $path)\n {\n+ if (empty($url) || empty($path)) {",
"comment_created_at": "2025-05-22T14:05:37+00:00",
"comment_author": "GrahamCampbell",
"comment_body": "Empty is the wrong function here. It does not check if the variable us the empty string, it checks if it looks like an enmpty-ish value, inc if it's 0. One should compare against `''` instead.",
"pr_file_module": null
}
]
},
{
"discussion_id": "2090368477",
"pr_number": 55740,
"pr_file": "src/Illuminate/Foundation/Configuration/ApplicationBuilder.php",
"created_at": "2025-05-15T06:23:18+00:00",
"commented_code": "string $apiPrefix = 'api',\n ?callable $then = null)\n {\n if (is_null($using) && (is_string($web) || is_array($web) || is_string($api) || is_array($api) || is_string($pages) || is_string($health)) || is_callable($then)) {\n $using = $this->buildRoutingCallback($web, $api, $pages, $health, $apiPrefix, $then);\n if (is_null($using) && (is_string($web) || is_array($web) || is_string($api) || is_array($api) || is_string($pages)) || is_callable($then)) {\n $using = $this->buildRoutingCallback($web, $api, $pages, $apiPrefix, $then);\n }\n\n if (is_string($health)) {\n PreventRequestsDuringMaintenance::except($health);\n }\n if (is_string($health)) {\n $using = function () use ($health, $using) {\n $this->buildHealthCheckRoute($health);\n call_user_func($using, $this->app);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2090368477",
"repo_full_name": "laravel/framework",
"pr_number": 55740,
"pr_file": "src/Illuminate/Foundation/Configuration/ApplicationBuilder.php",
"discussion_id": "2090368477",
"commented_code": "@@ -159,12 +159,17 @@ public function withRouting(?Closure $using = null,\n string $apiPrefix = 'api',\n ?callable $then = null)\n {\n- if (is_null($using) && (is_string($web) || is_array($web) || is_string($api) || is_array($api) || is_string($pages) || is_string($health)) || is_callable($then)) {\n- $using = $this->buildRoutingCallback($web, $api, $pages, $health, $apiPrefix, $then);\n+ if (is_null($using) && (is_string($web) || is_array($web) || is_string($api) || is_array($api) || is_string($pages)) || is_callable($then)) {\n+ $using = $this->buildRoutingCallback($web, $api, $pages, $apiPrefix, $then);\n+ }\n \n- if (is_string($health)) {\n- PreventRequestsDuringMaintenance::except($health);\n- }\n+ if (is_string($health)) {\n+ $using = function () use ($health, $using) {\n+ $this->buildHealthCheckRoute($health);\n+ call_user_func($using, $this->app);",
"comment_created_at": "2025-05-15T06:23:18+00:00",
"comment_author": "rodrigopedra",
"comment_body": "This line should be:\r\n\r\n```php\r\n$this->app->call($using);\r\n```\r\n\r\nTo preserve the current behavior, as in:\r\n\r\nhttps://github.com/laravel/framework/blob/df12a087731b37708cfbba72172a4c381363fed2/src/Illuminate/Foundation/Support/Providers/RouteServiceProvider.php#L162",
"pr_file_module": null
},
{
"comment_id": "2090370825",
"repo_full_name": "laravel/framework",
"pr_number": 55740,
"pr_file": "src/Illuminate/Foundation/Configuration/ApplicationBuilder.php",
"discussion_id": "2090368477",
"commented_code": "@@ -159,12 +159,17 @@ public function withRouting(?Closure $using = null,\n string $apiPrefix = 'api',\n ?callable $then = null)\n {\n- if (is_null($using) && (is_string($web) || is_array($web) || is_string($api) || is_array($api) || is_string($pages) || is_string($health)) || is_callable($then)) {\n- $using = $this->buildRoutingCallback($web, $api, $pages, $health, $apiPrefix, $then);\n+ if (is_null($using) && (is_string($web) || is_array($web) || is_string($api) || is_array($api) || is_string($pages)) || is_callable($then)) {\n+ $using = $this->buildRoutingCallback($web, $api, $pages, $apiPrefix, $then);\n+ }\n \n- if (is_string($health)) {\n- PreventRequestsDuringMaintenance::except($health);\n- }\n+ if (is_string($health)) {\n+ $using = function () use ($health, $using) {\n+ $this->buildHealthCheckRoute($health);\n+ call_user_func($using, $this->app);",
"comment_created_at": "2025-05-15T06:25:02+00:00",
"comment_author": "rodrigopedra",
"comment_body": "Also, `$using` in this point, can be `null`. A check for that should be added.\r\n\r\nAn app can be built without specifying no routes, by just providing the `$commands` and/or the `$channels` parameters.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1953114524",
"pr_number": 54582,
"pr_file": "src/Illuminate/View/Compilers/BladeCompiler.php",
"created_at": "2025-02-12T17:32:47+00:00",
"commented_code": "if (is_null($alias)) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1953114524",
"repo_full_name": "laravel/framework",
"pr_number": 54582,
"pr_file": "src/Illuminate/View/Compilers/BladeCompiler.php",
"discussion_id": "1953114524",
"commented_code": "@@ -762,14 +768,14 @@ public function component($class, $alias = null, $prefix = '')\n \n if (is_null($alias)) {",
"comment_created_at": "2025-02-12T17:32:47+00:00",
"comment_author": "shaedrich",
"comment_body": "You can avoid the function call like this:\r\n```suggestion\r\n if ($alias === null) {\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1923809576",
"pr_number": 54285,
"pr_file": "src/Illuminate/Support/Str.php",
"created_at": "2025-01-21T14:16:17+00:00",
"commented_code": "static::$camelCache = [];\n static::$studlyCache = [];\n }\n\n /**\n * Sanitize the given string.\n *\n *\n * See: https://symfony.com/doc/current/html_sanitizer.html\n *\n * @param string $string The input string to sanitize.\n * @param HtmlSanitizerConfig|null $config Custom configuration to use for sanitizing.\n * @return string The sanitized string.\n */\n public static function sanitize($string, ?HtmlSanitizerConfig $config = null)\n {\n if (is_null($string)) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1923809576",
"repo_full_name": "laravel/framework",
"pr_number": 54285,
"pr_file": "src/Illuminate/Support/Str.php",
"discussion_id": "1923809576",
"commented_code": "@@ -2014,4 +2016,28 @@ public static function flushCache()\n static::$camelCache = [];\n static::$studlyCache = [];\n }\n+\n+ /**\n+ * Sanitize the given string.\n+ *\n+ *\n+ * See: https://symfony.com/doc/current/html_sanitizer.html\n+ *\n+ * @param string $string The input string to sanitize.\n+ * @param HtmlSanitizerConfig|null $config Custom configuration to use for sanitizing.\n+ * @return string The sanitized string.\n+ */\n+ public static function sanitize($string, ?HtmlSanitizerConfig $config = null)\n+ {\n+ if (is_null($string)) {",
"comment_created_at": "2025-01-21T14:16:17+00:00",
"comment_author": "shaedrich",
"comment_body": "According to your `is_null()` call, your string can be `null`. This should be reflected in the PHPDoc comment type as well:\r\n```suggestion\r\n * @param string|null $string The input string to sanitize.\r\n * @param HtmlSanitizerConfig|null $config Custom configuration to use for sanitizing.\r\n * @return string The sanitized string.\r\n */\r\n public static function sanitize($string, ?HtmlSanitizerConfig $config = null)\r\n {\r\n if ($string === null) {\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1923810842",
"pr_number": 54285,
"pr_file": "src/Illuminate/Support/Str.php",
"created_at": "2025-01-21T14:17:05+00:00",
"commented_code": "static::$camelCache = [];\n static::$studlyCache = [];\n }\n\n /**\n * Sanitize the given string.\n *\n *\n * See: https://symfony.com/doc/current/html_sanitizer.html\n *\n * @param string $string The input string to sanitize.\n * @param HtmlSanitizerConfig|null $config Custom configuration to use for sanitizing.\n * @return string The sanitized string.\n */\n public static function sanitize($string, ?HtmlSanitizerConfig $config = null)\n {\n if (is_null($string)) {\n return null;",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1923810842",
"repo_full_name": "laravel/framework",
"pr_number": 54285,
"pr_file": "src/Illuminate/Support/Str.php",
"discussion_id": "1923810842",
"commented_code": "@@ -2014,4 +2016,28 @@ public static function flushCache()\n static::$camelCache = [];\n static::$studlyCache = [];\n }\n+\n+ /**\n+ * Sanitize the given string.\n+ *\n+ *\n+ * See: https://symfony.com/doc/current/html_sanitizer.html\n+ *\n+ * @param string $string The input string to sanitize.\n+ * @param HtmlSanitizerConfig|null $config Custom configuration to use for sanitizing.\n+ * @return string The sanitized string.\n+ */\n+ public static function sanitize($string, ?HtmlSanitizerConfig $config = null)\n+ {\n+ if (is_null($string)) {\n+ return null;",
"comment_created_at": "2025-01-21T14:17:05+00:00",
"comment_author": "shaedrich",
"comment_body": "According to your `is_null()` call, the method can return `null`. This should be reflected in the PHPDoc comment type as well:\r\n```suggestion\r\n * @return string|null The sanitized string.\r\n */\r\n public static function sanitize($string, ?HtmlSanitizerConfig $config = null)\r\n {\r\n if (is_null($string)) {\r\n return null;\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1884704097",
"pr_number": 53872,
"pr_file": "src/Illuminate/Http/Response.php",
"created_at": "2024-12-14T01:28:04+00:00",
"commented_code": "return $this;\n }\n\n /**\n * Gets the current response content.\n */\n #[\\Override]\n public function getContent(): string|false\n {\n return transform(parent::getContent(), fn ($content) => $content, '');",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1884704097",
"repo_full_name": "laravel/framework",
"pr_number": 53872,
"pr_file": "src/Illuminate/Http/Response.php",
"discussion_id": "1884704097",
"commented_code": "@@ -75,6 +75,15 @@ public function setContent(mixed $content): static\n return $this;\n }\n \n+ /**\n+ * Gets the current response content.\n+ */\n+ #[\\Override]\n+ public function getContent(): string|false\n+ {\n+ return transform(parent::getContent(), fn ($content) => $content, '');",
"comment_created_at": "2024-12-14T01:28:04+00:00",
"comment_author": "rodrigopedra",
"comment_body": "Why not just?\r\n\r\n```php\r\nreturn parent::getContent() ?? '';\r\n```\r\n",
"pr_file_module": null
}
]
},
{
"discussion_id": "1689591772",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"created_at": "2024-07-24T11:15:45+00:00",
"commented_code": "// If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n if (isset($relation) || is_null($value)) {\n if (isset($relation)) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1689591772",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T11:15:45+00:00",
"comment_author": "pxpm",
"comment_body": "Doesn't this change the behavior? `null` values should enter the condition. Removing the `|| is_null()` they will not enter anymore as the `isset()` without the `is_null()` check would exclude them. \r\n\r\n```php\r\n$relation, $value = null;\r\n\r\nif (isset($relation)) {\r\n// we don't reach this code\r\n}\r\n\r\nif (isset($relation) || is_null($value)) {\r\n// this code is executed\r\n}\r\n```\r\nIf this is some kind of bug fixing it shouldn't be part of this PR IMO. ",
"pr_file_module": null
},
{
"comment_id": "1689784138",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T13:16:01+00:00",
"comment_author": "calebdw",
"comment_body": "This yielded the following error:\r\n\r\n```\r\n ------ ------------------------------------------------------------------------------------------------------------------ \r\n Line Illuminate/Database/Eloquent/Concerns/HasAttributes.php (in context of class Illuminate\\Database\\Eloquent\\Model) \r\n ------ ------------------------------------------------------------------------------------------------------------------ \r\n 398 Variable $relation might not be defined. \r\n \ud83e\udeaa variable.undefined \r\n```\r\n\r\nThe `is_null($value)` is already checked above:\r\n\r\nhttps://github.com/laravel/framework/blob/46297e441252d495713fb30223ffcfb325a4f120/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L380-L385\r\n\r\nAnd you'll notice that `$value` is not referenced inside the `if` body and therefore should not be referenced inside the condition. The only thing that matters is checking that the `$relation` isset before assigning it.",
"pr_file_module": null
},
{
"comment_id": "1689815593",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T13:31:40+00:00",
"comment_author": "pxpm",
"comment_body": "Hey @calebdw I think we are talking different things here. \r\n\r\nThe first check for `is_null($value)` just sets the `$relation` value to `null`. The check you removed sets `$attributes[$key] = $relation;` when `$value = null **OR** isset($relation)`. \r\n\r\nThe fact that `$value` is not used inside the if, does not mean you can remove it from the condition as that would turn up not setting `$attributes[$key] = null` like was previously done. ",
"pr_file_module": null
},
{
"comment_id": "1689824977",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T13:36:20+00:00",
"comment_author": "calebdw",
"comment_body": "If `$value === null`, then `$relation` isset.",
"pr_file_module": null
},
{
"comment_id": "1689830616",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T13:39:03+00:00",
"comment_author": "calebdw",
"comment_body": "Hmm, apparently there's some idiosyncrasies with [isset](https://www.php.net/manual/en/function.isset.php):\r\n\r\n> Determine if a variable is considered set, this means if a variable is declared and is different than [null](https://www.php.net/manual/en/reserved.constants.php#constant.null). ",
"pr_file_module": null
},
{
"comment_id": "1689855018",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T13:51:01+00:00",
"comment_author": "pxpm",
"comment_body": "Indeed @calebdw that was the issue with your reasoning, as you were considering `$relation` set when it's `null`. \ud83d\udc4d ",
"pr_file_module": null
},
{
"comment_id": "1689893235",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T14:09:10+00:00",
"comment_author": "calebdw",
"comment_body": "It turns out there's not a straightforward way in PHP to check if a variable isset / declared (null values being acceptable). Even using `isset($relation) || is_null($relation)` fails because `is_null` returns true when the variable does not exist :roll_eyes: .\r\n\r\nThanks for pointing this out! I also added a test since this is rather tricky",
"pr_file_module": null
},
{
"comment_id": "1689940559",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T14:37:04+00:00",
"comment_author": "pxpm",
"comment_body": "Well, theoretically you can do a `get_defined_vars()` and a `array_key_exists()` to check if a variable is actually defined disregarding it's value: \r\n\r\n```php\r\n$test = null;\r\n\r\nisset($test); // return false\r\narray_key_exists('test', get_defined_vars()); // return true\r\n```\r\n\r\nNote that `get_defined_vars()` gets the variables depending on the scope it's called. https://www.php.net/manual/en/function.get-defined-vars.php\r\n\r\nCheers",
"pr_file_module": null
},
{
"comment_id": "1689947777",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T14:41:33+00:00",
"comment_author": "calebdw",
"comment_body": "Yes, I considered that, however, phpstan doesn't recognize that the `$relation` is defined inside the `if` so you'd still need an `@phpstan-ignore` call",
"pr_file_module": null
},
{
"comment_id": "1689967349",
"repo_full_name": "laravel/framework",
"pr_number": 51956,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1689591772",
"commented_code": "@@ -394,7 +394,7 @@ public function relationsToArray()\n // If the relation value has been set, we will set it on this attributes\n // list for returning. If it was not arrayable or null, we'll not set\n // the value on the array because it is some type of invalid value.\n- if (isset($relation) || is_null($value)) {\n+ if (isset($relation)) {",
"comment_created_at": "2024-07-24T14:53:35+00:00",
"comment_author": "pxpm",
"comment_body": "You can probably use `$relation ?? null` to avoid the phpstan error ?",
"pr_file_module": null
}
]
}
]

339
_reviewers/framework-keep-ci-configurations-minimal.md Normal file
View File

@@ -0,0 +1,339 @@
---
title: Keep CI configurations minimal
description: 'When configuring CI workflows, include only the extensions, tools, and
settings that are necessary for the specific tests being run. Avoid adding unnecessary
PHP extensions (such as `xml` or `xdebug`) to workflow configurations unless they''re
explicitly required. Similarly, keep coverage disabled (`coverage: none`) in CI
pipelines by default to improve...'
repository: laravel/framework
label: CI/CD
language: Yaml
comments_count: 14
repository_stars: 33763
---
When configuring CI workflows, include only the extensions, tools, and settings that are necessary for the specific tests being run. Avoid adding unnecessary PHP extensions (such as `xml` or `xdebug`) to workflow configurations unless they're explicitly required. Similarly, keep coverage disabled (`coverage: none`) in CI pipelines by default to improve performance.
Example:
```yaml
- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: 8.3
extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr
coverage: none
```
This approach keeps CI workflows efficient, reduces execution time, and avoids potential issues from unnecessary dependencies.
[
{
"discussion_id": "1856404321",
"pr_number": 53648,
"pr_file": ".github/workflows/databases-nightly.yml",
"created_at": "2024-11-25T11:03:56+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856404321",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/databases-nightly.yml",
"discussion_id": "1856404321",
"commented_code": "@@ -77,9 +77,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"comment_created_at": "2024-11-25T11:03:56+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856406988",
"pr_number": 53648,
"pr_file": ".github/workflows/databases-nightly.yml",
"created_at": "2024-11-25T11:04:34+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856406988",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/databases-nightly.yml",
"discussion_id": "1856406988",
"commented_code": "@@ -31,9 +31,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"comment_created_at": "2024-11-25T11:04:34+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856407588",
"pr_number": 53648,
"pr_file": ".github/workflows/tests.yml",
"created_at": "2024-11-25T11:04:57+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: ${{ matrix.php }}\n extensions: dom, curl, libxml, mbstring, zip, pdo, sqlite, pdo_sqlite, gd, pdo_mysql, fileinfo, ftp, redis, memcached, gmp, intl, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pdo, sqlite, pdo_sqlite, gd, pdo_mysql, fileinfo, ftp, redis, memcached, gmp, intl, xml, xdebug, :php-psr",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856407588",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/tests.yml",
"discussion_id": "1856407588",
"commented_code": "@@ -133,9 +133,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: ${{ matrix.php }}\n- extensions: dom, curl, libxml, mbstring, zip, pdo, sqlite, pdo_sqlite, gd, pdo_mysql, fileinfo, ftp, redis, memcached, gmp, intl, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pdo, sqlite, pdo_sqlite, gd, pdo_mysql, fileinfo, ftp, redis, memcached, gmp, intl, xml, xdebug, :php-psr",
"comment_created_at": "2024-11-25T11:04:57+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n extensions: dom, curl, libxml, mbstring, zip, pdo, sqlite, pdo_sqlite, gd, pdo_mysql, fileinfo, ftp, redis, memcached, gmp, intl, :php-psr\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856408180",
"pr_number": 53648,
"pr_file": ".github/workflows/tests.yml",
"created_at": "2024-11-25T11:05:20+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: ${{ matrix.php }}\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd, redis, igbinary, msgpack, memcached, gmp, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd, redis, igbinary, msgpack, memcached, gmp, xml, xdebug, :php-psr\n ini-values: error_reporting=E_ALL\n tools: composer:v2\n coverage: none\n coverage: xdebug",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856408180",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/tests.yml",
"discussion_id": "1856408180",
"commented_code": "@@ -53,10 +53,10 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: ${{ matrix.php }}\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd, redis, igbinary, msgpack, memcached, gmp, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd, redis, igbinary, msgpack, memcached, gmp, xml, xdebug, :php-psr\n ini-values: error_reporting=E_ALL\n tools: composer:v2\n- coverage: none\n+ coverage: xdebug",
"comment_created_at": "2024-11-25T11:05:20+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n coverage: none\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856408369",
"pr_number": 53648,
"pr_file": ".github/workflows/tests.yml",
"created_at": "2024-11-25T11:05:28+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: ${{ matrix.php }}\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd, redis, igbinary, msgpack, memcached, gmp, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd, redis, igbinary, msgpack, memcached, gmp, xml, xdebug, :php-psr",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856408369",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/tests.yml",
"discussion_id": "1856408369",
"commented_code": "@@ -53,10 +53,10 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: ${{ matrix.php }}\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd, redis, igbinary, msgpack, memcached, gmp, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd, redis, igbinary, msgpack, memcached, gmp, xml, xdebug, :php-psr",
"comment_created_at": "2024-11-25T11:05:28+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, sqlite, pdo_sqlite, gd, redis, igbinary, msgpack, memcached, gmp, :php-psr\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856408715",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"created_at": "2024-11-25T11:05:44+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856408715",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"discussion_id": "1856408715",
"commented_code": "@@ -147,9 +147,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"comment_created_at": "2024-11-25T11:05:44+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856409284",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"created_at": "2024-11-25T11:06:11+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856409284",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"discussion_id": "1856409284",
"commented_code": "@@ -107,9 +107,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"comment_created_at": "2024-11-25T11:06:11+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856409723",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"created_at": "2024-11-25T11:06:28+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856409723",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"discussion_id": "1856409723",
"commented_code": "@@ -59,9 +59,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"comment_created_at": "2024-11-25T11:06:28+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856409908",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"created_at": "2024-11-25T11:06:37+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n coverage: none\n coverage: xdebug",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856409908",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"discussion_id": "1856409908",
"commented_code": "@@ -24,9 +24,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n- coverage: none\n+ coverage: xdebug",
"comment_created_at": "2024-11-25T11:06:37+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n coverage: none\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856410088",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"created_at": "2024-11-25T11:06:45+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856410088",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/queues.yml",
"discussion_id": "1856410088",
"commented_code": "@@ -24,9 +24,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.2\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr",
"comment_created_at": "2024-11-25T11:06:45+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856411136",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"created_at": "2024-11-25T11:07:18+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n extensions: dom, curl, libxml, mbstring, zip, pcntl, sqlsrv, pdo, pdo_sqlsrv, odbc, pdo_odbc, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, sqlsrv, pdo, pdo_sqlsrv, odbc, pdo_odbc, xml, xdebug, :php-psr\n tools: composer:v2\n coverage: none\n coverage: xdebug",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856411136",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"discussion_id": "1856411136",
"commented_code": "@@ -319,9 +319,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, sqlsrv, pdo, pdo_sqlsrv, odbc, pdo_odbc, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, sqlsrv, pdo, pdo_sqlsrv, odbc, pdo_odbc, xml, xdebug, :php-psr\n tools: composer:v2\n- coverage: none\n+ coverage: xdebug",
"comment_created_at": "2024-11-25T11:07:18+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n coverage: none\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856412237",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"created_at": "2024-11-25T11:08:08+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_pgsql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_pgsql, xml, xdebug, :php-psr",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856412237",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"discussion_id": "1856412237",
"commented_code": "@@ -224,9 +224,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_pgsql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_pgsql, xml, xdebug, :php-psr",
"comment_created_at": "2024-11-25T11:08:08+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_pgsql, :php-psr\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856412446",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"created_at": "2024-11-25T11:08:17+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_pgsql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_pgsql, xml, xdebug, :php-psr\n tools: composer:v2\n coverage: none\n coverage: xdebug",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856412446",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"discussion_id": "1856412446",
"commented_code": "@@ -175,9 +175,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_pgsql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_pgsql, xml, xdebug, :php-psr\n tools: composer:v2\n- coverage: none\n+ coverage: xdebug",
"comment_created_at": "2024-11-25T11:08:17+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n coverage: none\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1856413251",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"created_at": "2024-11-25T11:08:51+00:00",
"commented_code": "uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n coverage: none\n coverage: xdebug",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1856413251",
"repo_full_name": "laravel/framework",
"pr_number": 53648,
"pr_file": ".github/workflows/databases.yml",
"discussion_id": "1856413251",
"commented_code": "@@ -128,9 +128,9 @@ jobs:\n uses: shivammathur/setup-php@v2\n with:\n php-version: 8.3\n- extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, :php-psr\n+ extensions: dom, curl, libxml, mbstring, zip, pcntl, pdo, pdo_mysql, xml, xdebug, :php-psr\n tools: composer:v2\n- coverage: none\n+ coverage: xdebug",
"comment_created_at": "2024-11-25T11:08:51+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n coverage: none\r\n```",
"pr_file_module": null
}
]
}
]

265
_reviewers/framework-manage-dependencies-wisely.md Normal file
View File

@@ -0,0 +1,265 @@
---
title: Manage dependencies wisely
description: 'When configuring dependencies in composer.json files, follow these guidelines
to ensure maintainable and reliable configurations:
1. **Only include direct dependencies**: Add dependencies only when they are directly
used by your package. If a dependency is accessed through another package, don''t
include it directly.'
repository: laravel/framework
label: Configurations
language: Json
comments_count: 5
repository_stars: 33763
---
When configuring dependencies in composer.json files, follow these guidelines to ensure maintainable and reliable configurations:
1. **Only include direct dependencies**: Add dependencies only when they are directly used by your package. If a dependency is accessed through another package, don't include it directly.
```json
// AVOID - including unused direct dependencies
{
"require": {
"illuminate/contracts": "^12.0",
"psr/simple-cache": "^1.0|^2.0|^3.0" // Already included via illuminate/contracts
}
}
// BETTER - only include what you directly use
{
"require": {
"illuminate/contracts": "^12.0"
}
}
```
2. **Use appropriate dependency categories**: Place non-essential extensions and packages in the "suggest" section rather than "require" when they're only needed for optional features.
```json
// BETTER - use suggest for optional dependencies
{
"require": {
"php": "^8.2"
},
"suggest": {
"ext-zlib": "For compression features",
"spatie/fork": "For alternative concurrency driver"
}
}
```
3. **Evaluate maintainer activity**: Before adding third-party dependencies, consider the package's maintenance status. For critical functionality, prefer actively maintained packages or consider implementing the functionality internally if maintenance is questionable.
4. **Use proper version constraints**: Format version constraints consistently and appropriately (use pipe | without spaces) and ensure they're compatible with all other dependencies.
```json
// AVOID
"laravel/serializable-closure": "^2.0@dev",
"laravel/prompts": "^0.1.20 || ^0.2 || ^0.3"
// BETTER
"laravel/serializable-closure": "^1.3|^2.0",
"laravel/prompts": "^0.1.20|^0.2|^0.3"
```
Following these practices helps maintain reliable, clearly defined configuration files that minimize surprises during updates and installations.
[
{
"discussion_id": "2019945476",
"pr_number": 55203,
"pr_file": "src/Illuminate/Concurrency/composer.json",
"created_at": "2025-03-29T19:04:13+00:00",
"commented_code": "],\n \"require\": {\n \"php\": \"^8.2\",\n \"illuminate/console\": \"^12.0\",\n \"illuminate/contracts\": \"^12.0\",\n \"illuminate/process\": \"^12.0\",",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2019945476",
"repo_full_name": "laravel/framework",
"pr_number": 55203,
"pr_file": "src/Illuminate/Concurrency/composer.json",
"discussion_id": "2019945476",
"commented_code": "@@ -15,9 +15,7 @@\n ],\n \"require\": {\n \"php\": \"^8.2\",\n- \"illuminate/console\": \"^12.0\",\n \"illuminate/contracts\": \"^12.0\",\n- \"illuminate/process\": \"^12.0\",",
"comment_created_at": "2025-03-29T19:04:13+00:00",
"comment_author": "AndrewMast",
"comment_body": "Why are `illuminate/console` and `illuminate/process` removed? I see that `console` is added to the `suggest` field, but what about `process`? Does the rest of the subpackage not need these? If not, it might be best to address the changes in a separate PR.\r\n\r\nI do like the idea of adding `spatie/fork` and the other driver options to `suggest`, though. ",
"pr_file_module": null
},
{
"comment_id": "2019948808",
"repo_full_name": "laravel/framework",
"pr_number": 55203,
"pr_file": "src/Illuminate/Concurrency/composer.json",
"discussion_id": "2019945476",
"commented_code": "@@ -15,9 +15,7 @@\n ],\n \"require\": {\n \"php\": \"^8.2\",\n- \"illuminate/console\": \"^12.0\",\n \"illuminate/contracts\": \"^12.0\",\n- \"illuminate/process\": \"^12.0\",",
"comment_created_at": "2025-03-29T19:09:46+00:00",
"comment_author": "Amirhf1",
"comment_body": "You are absolutely right. It was my mistake to remove the dependencies for `illuminate/console` and `illuminate/process`. I\u2019ve corrected that and added both dependencies back to the composer.json file. I\u2019ve still kept the suggestions for illuminate/redis and spatie/fork, as they are needed for optional drivers.\r\nThank you for your thorough review.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1934190230",
"pr_number": 54375,
"pr_file": "composer.json",
"created_at": "2025-01-29T16:18:29+00:00",
"commented_code": "\"symfony/uid\": \"^7.0.3\",\n \"symfony/var-dumper\": \"^7.0.3\",\n \"tijsverkoyen/css-to-inline-styles\": \"^2.2.5\",\n \"visus/cuid2\": \"^5.0.0\",",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1934190230",
"repo_full_name": "laravel/framework",
"pr_number": 54375,
"pr_file": "composer.json",
"discussion_id": "1934190230",
"commented_code": "@@ -57,6 +58,7 @@\n \"symfony/uid\": \"^7.0.3\",\n \"symfony/var-dumper\": \"^7.0.3\",\n \"tijsverkoyen/css-to-inline-styles\": \"^2.2.5\",\n+ \"visus/cuid2\": \"^5.0.0\",",
"comment_created_at": "2025-01-29T16:18:29+00:00",
"comment_author": "stevebauman",
"comment_body": "This maintainer isn't very active. I'd be very hesitant to base the framework on this package's support.",
"pr_file_module": null
},
{
"comment_id": "1976438706",
"repo_full_name": "laravel/framework",
"pr_number": 54375,
"pr_file": "composer.json",
"discussion_id": "1934190230",
"commented_code": "@@ -57,6 +58,7 @@\n \"symfony/uid\": \"^7.0.3\",\n \"symfony/var-dumper\": \"^7.0.3\",\n \"tijsverkoyen/css-to-inline-styles\": \"^2.2.5\",\n+ \"visus/cuid2\": \"^5.0.0\",",
"comment_created_at": "2025-03-01T15:34:45+00:00",
"comment_author": "xaoseric",
"comment_body": "Something like cuid2 ideally won't need a lot of updates, but if that is a blocker then I can bring over the code for it as an illuminate/cuid2 package so we can update it without relying on that package.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1667427139",
"pr_number": 52039,
"pr_file": "composer.json",
"created_at": "2024-07-06T19:56:55+00:00",
"commented_code": "\"ext-openssl\": \"*\",\n \"ext-session\": \"*\",\n \"ext-tokenizer\": \"*\",\n \"ext-zlib\": \"*\",",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1667427139",
"repo_full_name": "laravel/framework",
"pr_number": 52039,
"pr_file": "composer.json",
"discussion_id": "1667427139",
"commented_code": "@@ -23,6 +23,7 @@\n \"ext-openssl\": \"*\",\n \"ext-session\": \"*\",\n \"ext-tokenizer\": \"*\",\n+ \"ext-zlib\": \"*\",",
"comment_created_at": "2024-07-06T19:56:55+00:00",
"comment_author": "rojtjo",
"comment_body": "Adding an additional extension in a patch release is a breaking change. This should either only be in the `suggest` section (with an `extension_loaded` check in the code) or be targeted at 12.x instead.",
"pr_file_module": null
},
{
"comment_id": "1668174517",
"repo_full_name": "laravel/framework",
"pr_number": 52039,
"pr_file": "composer.json",
"discussion_id": "1667427139",
"commented_code": "@@ -23,6 +23,7 @@\n \"ext-openssl\": \"*\",\n \"ext-session\": \"*\",\n \"ext-tokenizer\": \"*\",\n+ \"ext-zlib\": \"*\",",
"comment_created_at": "2024-07-08T08:07:27+00:00",
"comment_author": "driesvints",
"comment_body": "Just for reference, this isn't a breaking change. Composer will warn you about the missing extension. Adding new dependencies on patch releases is allowed.",
"pr_file_module": null
},
{
"comment_id": "1668209544",
"repo_full_name": "laravel/framework",
"pr_number": 52039,
"pr_file": "composer.json",
"discussion_id": "1667427139",
"commented_code": "@@ -23,6 +23,7 @@\n \"ext-openssl\": \"*\",\n \"ext-session\": \"*\",\n \"ext-tokenizer\": \"*\",\n+ \"ext-zlib\": \"*\",",
"comment_created_at": "2024-07-08T08:29:38+00:00",
"comment_author": "rodrigopedra",
"comment_body": "@driesvints thanks for clarifying that. I understand the argument for composer dependencies, but does it also applies for extensions?\r\n\r\nThat Composer warns about a missing extension, it does, indeed, all good about that.\r\n\r\nWhat I mean is: if someone is running a Laravel 11 app on a server which they are not allowed to freely install extensions, such as shared hosting, then they wouldn't be able to update their app due to the inability to install a newly required extension.\r\n\r\nDoesn't it count as a breaking change? As a requirement change?\r\n\r\nI believe a `suggests` entry would be a better fit in that case.\r\n\r\nIf that case is still allowed, as a requirement, I believe docs should also be updated to reflect the newly required extension on the \"Server Requirements\" section:\r\n\r\nhttps://laravel.com/docs/11.x/deployment#server-requirements",
"pr_file_module": null
},
{
"comment_id": "1668251380",
"repo_full_name": "laravel/framework",
"pr_number": 52039,
"pr_file": "composer.json",
"discussion_id": "1667427139",
"commented_code": "@@ -23,6 +23,7 @@\n \"ext-openssl\": \"*\",\n \"ext-session\": \"*\",\n \"ext-tokenizer\": \"*\",\n+ \"ext-zlib\": \"*\",",
"comment_created_at": "2024-07-08T08:52:47+00:00",
"comment_author": "driesvints",
"comment_body": "If the extension isn't needed for a core essential of Laravel then it can indeed be added as a suggest. I believe this can be done here since the feature is indeed isolated.\r\n\r\nSemVer doesn't look at things like shared hosting and the ability to not install extensions. It's solely focused on what is a breaking change and adding a dependency on a patch release isn't one. A good dependency manager, like Composer, will warn you about the new requirement and prevent you from moving forward in a deploy/install process until you've fulfilled the requirement. And thus nothing will break.",
"pr_file_module": null
},
{
"comment_id": "1668706256",
"repo_full_name": "laravel/framework",
"pr_number": 52039,
"pr_file": "composer.json",
"discussion_id": "1667427139",
"commented_code": "@@ -23,6 +23,7 @@\n \"ext-openssl\": \"*\",\n \"ext-session\": \"*\",\n \"ext-tokenizer\": \"*\",\n+ \"ext-zlib\": \"*\",",
"comment_created_at": "2024-07-08T14:06:01+00:00",
"comment_author": "driesvints",
"comment_body": "@rmunate move this to suggest please. Also for the `composer.json` of the support package.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1847590909",
"pr_number": 53552,
"pr_file": "composer.json",
"created_at": "2024-11-19T03:33:16+00:00",
"commented_code": "\"guzzlehttp/guzzle\": \"^7.8\",\n \"guzzlehttp/uri-template\": \"^1.0\",\n \"laravel/prompts\": \"^0.1.18|^0.2.0|^0.3.0\",\n \"laravel/serializable-closure\": \"^1.3\",\n \"laravel/serializable-closure\": \"^2.0@dev\",",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1847590909",
"repo_full_name": "laravel/framework",
"pr_number": 53552,
"pr_file": "composer.json",
"discussion_id": "1847590909",
"commented_code": "@@ -32,7 +32,7 @@\n \"guzzlehttp/guzzle\": \"^7.8\",\n \"guzzlehttp/uri-template\": \"^1.0\",\n \"laravel/prompts\": \"^0.1.18|^0.2.0|^0.3.0\",\n- \"laravel/serializable-closure\": \"^1.3\",\n+ \"laravel/serializable-closure\": \"^2.0@dev\",",
"comment_created_at": "2024-11-19T03:33:16+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n \"laravel/serializable-closure\": \"^1.3|^2.0\",\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1661941378",
"pr_number": 51983,
"pr_file": "src/Illuminate/Cache/composer.json",
"created_at": "2024-07-02T07:00:36+00:00",
"commented_code": "\"illuminate/collections\": \"^11.0\",\n \"illuminate/contracts\": \"^11.0\",\n \"illuminate/macroable\": \"^11.0\",\n \"illuminate/support\": \"^11.0\"\n \"illuminate/support\": \"^11.0\",\n \"psr/simple-cache\": \"^1.0|^2.0|^3.0\"",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1661941378",
"repo_full_name": "laravel/framework",
"pr_number": 51983,
"pr_file": "src/Illuminate/Cache/composer.json",
"discussion_id": "1661941378",
"commented_code": "@@ -18,7 +18,8 @@\n \"illuminate/collections\": \"^11.0\",\n \"illuminate/contracts\": \"^11.0\",\n \"illuminate/macroable\": \"^11.0\",\n- \"illuminate/support\": \"^11.0\"\n+ \"illuminate/support\": \"^11.0\",\n+ \"psr/simple-cache\": \"^1.0|^2.0|^3.0\"",
"comment_created_at": "2024-07-02T07:00:36+00:00",
"comment_author": "driesvints",
"comment_body": "This isn't correct. This package doesn't use this dependency directly but through a Symfony component. Please revert this.",
"pr_file_module": null
},
{
"comment_id": "1661979504",
"repo_full_name": "laravel/framework",
"pr_number": 51983,
"pr_file": "src/Illuminate/Cache/composer.json",
"discussion_id": "1661941378",
"commented_code": "@@ -18,7 +18,8 @@\n \"illuminate/collections\": \"^11.0\",\n \"illuminate/contracts\": \"^11.0\",\n \"illuminate/macroable\": \"^11.0\",\n- \"illuminate/support\": \"^11.0\"\n+ \"illuminate/support\": \"^11.0\",\n+ \"psr/simple-cache\": \"^1.0|^2.0|^3.0\"",
"comment_created_at": "2024-07-02T07:27:40+00:00",
"comment_author": "seriquynh",
"comment_body": "Oh, my bad. `illuminate\\cache` doesn't use directly `psr/simple-cache`, it uses through `illuminate/contracts`. I restored this change and updated PR's description. Thank you!\r\nhttps://github.com/laravel/framework/blob/11.x/src/Illuminate/Contracts/Cache/Repository.php\r\n\r\n```php\r\n<?php\r\n\r\nnamespace Illuminate\\Contracts\\Cache;\r\n\r\nuse Closure;\r\nuse Psr\\SimpleCache\\CacheInterface;\r\n\r\ninterface Repository extends CacheInterface\r\n{\r\n //\r\n}\r\n```",
"pr_file_module": null
}
]
}
]

152
_reviewers/framework-mark-sensitive-parameters.md Normal file
View File

@@ -0,0 +1,152 @@
---
title: Mark sensitive parameters
description: Always use the `#[\SensitiveParameter]` attribute for parameters containing
sensitive information such as passwords, tokens, API keys, and personal identifiable
information. This prevents accidental exposure of sensitive data in logs, stack
traces, and debugging information, which could otherwise lead to security breaches.
repository: laravel/framework
label: Security
language: PHP
comments_count: 2
repository_stars: 33763
---
Always use the `#[\SensitiveParameter]` attribute for parameters containing sensitive information such as passwords, tokens, API keys, and personal identifiable information. This prevents accidental exposure of sensitive data in logs, stack traces, and debugging information, which could otherwise lead to security breaches.
```php
// Before - security risk
public function __construct(
public ?string $username = null,
public ?string $pass = null,
) {}
// After - secure
public function __construct(
public ?string $username = null,
#[\SensitiveParameter]
public ?string $pass = null,
) {}
// Also use in method parameters
public function validateCredentials(
$username,
#[\SensitiveParameter] $password
) {
// Password won't appear in logs or stack traces
}
```
For legacy PHP versions without attribute support, consider using alternative patterns such as:
1. Accepting sensitive data through non-logged channels
2. Sanitizing log output manually before writing
3. Using specialized secure input handlers
Additionally, ensure proper error handling for security-critical functions. Avoid using error suppression operators (@) in favor of try-catch blocks that capture errors without exposing implementation details.
[
{
"discussion_id": "1878116494",
"pr_number": 53821,
"pr_file": "src/Illuminate/Hashing/ArgonHasher.php",
"created_at": "2024-12-10T13:40:39+00:00",
"commented_code": "*/\n public function make(#[\\SensitiveParameter] $value, array $options = [])\n {\n $hash = @password_hash($value, $this->algorithm(), [\n 'memory_cost' => $this->memory($options),\n 'time_cost' => $this->time($options),\n 'threads' => $this->threads($options),\n ]);\n\n if (! is_string($hash)) {\n try {\n $hash = password_hash($value, $this->algorithm(), [\n 'memory_cost' => $this->memory($options),\n 'time_cost' => $this->time($options),\n 'threads' => $this->threads($options),\n ]);\n } catch (Error) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1878116494",
"repo_full_name": "laravel/framework",
"pr_number": 53821,
"pr_file": "src/Illuminate/Hashing/ArgonHasher.php",
"discussion_id": "1878116494",
"commented_code": "@@ -60,13 +61,13 @@ public function __construct(array $options = [])\n */\n public function make(#[\\SensitiveParameter] $value, array $options = [])\n {\n- $hash = @password_hash($value, $this->algorithm(), [\n- 'memory_cost' => $this->memory($options),\n- 'time_cost' => $this->time($options),\n- 'threads' => $this->threads($options),\n- ]);\n-\n- if (! is_string($hash)) {\n+ try {\n+ $hash = password_hash($value, $this->algorithm(), [\n+ 'memory_cost' => $this->memory($options),\n+ 'time_cost' => $this->time($options),\n+ 'threads' => $this->threads($options),\n+ ]);\n+ } catch (Error) {",
"comment_created_at": "2024-12-10T13:40:39+00:00",
"comment_author": "cosmastech",
"comment_body": "Might it be helpful to add this caught Error as the `previous` parameter of the RuntimeException?",
"pr_file_module": null
},
{
"comment_id": "1878332094",
"repo_full_name": "laravel/framework",
"pr_number": 53821,
"pr_file": "src/Illuminate/Hashing/ArgonHasher.php",
"discussion_id": "1878116494",
"commented_code": "@@ -60,13 +61,13 @@ public function __construct(array $options = [])\n */\n public function make(#[\\SensitiveParameter] $value, array $options = [])\n {\n- $hash = @password_hash($value, $this->algorithm(), [\n- 'memory_cost' => $this->memory($options),\n- 'time_cost' => $this->time($options),\n- 'threads' => $this->threads($options),\n- ]);\n-\n- if (! is_string($hash)) {\n+ try {\n+ $hash = password_hash($value, $this->algorithm(), [\n+ 'memory_cost' => $this->memory($options),\n+ 'time_cost' => $this->time($options),\n+ 'threads' => $this->threads($options),\n+ ]);\n+ } catch (Error) {",
"comment_created_at": "2024-12-10T15:38:01+00:00",
"comment_author": "browner12",
"comment_body": "yah, I was considering that. is there any security implication though, because it could expose info like which algo is being used?",
"pr_file_module": null
},
{
"comment_id": "1878386477",
"repo_full_name": "laravel/framework",
"pr_number": 53821,
"pr_file": "src/Illuminate/Hashing/ArgonHasher.php",
"discussion_id": "1878116494",
"commented_code": "@@ -60,13 +61,13 @@ public function __construct(array $options = [])\n */\n public function make(#[\\SensitiveParameter] $value, array $options = [])\n {\n- $hash = @password_hash($value, $this->algorithm(), [\n- 'memory_cost' => $this->memory($options),\n- 'time_cost' => $this->time($options),\n- 'threads' => $this->threads($options),\n- ]);\n-\n- if (! is_string($hash)) {\n+ try {\n+ $hash = password_hash($value, $this->algorithm(), [\n+ 'memory_cost' => $this->memory($options),\n+ 'time_cost' => $this->time($options),\n+ 'threads' => $this->threads($options),\n+ ]);\n+ } catch (Error) {",
"comment_created_at": "2024-12-10T16:09:01+00:00",
"comment_author": "cosmastech",
"comment_body": "Isn't it already showing the hashing algo in the exception message?\n\nIn theory, these exceptions should not be passed along to the end-user when the app is set to production mode.\n\n(I don't have a strong opinion on it either way, just a thought as I have fallen into the practice of forwarding exceptions when throwing a new one)",
"pr_file_module": null
},
{
"comment_id": "1878387944",
"repo_full_name": "laravel/framework",
"pr_number": 53821,
"pr_file": "src/Illuminate/Hashing/ArgonHasher.php",
"discussion_id": "1878116494",
"commented_code": "@@ -60,13 +61,13 @@ public function __construct(array $options = [])\n */\n public function make(#[\\SensitiveParameter] $value, array $options = [])\n {\n- $hash = @password_hash($value, $this->algorithm(), [\n- 'memory_cost' => $this->memory($options),\n- 'time_cost' => $this->time($options),\n- 'threads' => $this->threads($options),\n- ]);\n-\n- if (! is_string($hash)) {\n+ try {\n+ $hash = password_hash($value, $this->algorithm(), [\n+ 'memory_cost' => $this->memory($options),\n+ 'time_cost' => $this->time($options),\n+ 'threads' => $this->threads($options),\n+ ]);\n+ } catch (Error) {",
"comment_created_at": "2024-12-10T16:09:58+00:00",
"comment_author": "cosmastech",
"comment_body": "It also catches a generic Error object, which could be caused by any number of things. At least this way you can see what specifically happened.",
"pr_file_module": null
},
{
"comment_id": "1878417875",
"repo_full_name": "laravel/framework",
"pr_number": 53821,
"pr_file": "src/Illuminate/Hashing/ArgonHasher.php",
"discussion_id": "1878116494",
"commented_code": "@@ -60,13 +61,13 @@ public function __construct(array $options = [])\n */\n public function make(#[\\SensitiveParameter] $value, array $options = [])\n {\n- $hash = @password_hash($value, $this->algorithm(), [\n- 'memory_cost' => $this->memory($options),\n- 'time_cost' => $this->time($options),\n- 'threads' => $this->threads($options),\n- ]);\n-\n- if (! is_string($hash)) {\n+ try {\n+ $hash = password_hash($value, $this->algorithm(), [\n+ 'memory_cost' => $this->memory($options),\n+ 'time_cost' => $this->time($options),\n+ 'threads' => $this->threads($options),\n+ ]);\n+ } catch (Error) {",
"comment_created_at": "2024-12-10T16:24:50+00:00",
"comment_author": "browner12",
"comment_body": "it could also expose other info, like cost factor, etc. \r\n\r\nyou're right, it should never show to users in production.\r\n\r\nI'd say give a PR a shot, and see what other people think.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1837330959",
"pr_number": 53370,
"pr_file": "src/Illuminate/Support/Url.php",
"created_at": "2024-11-12T00:21:55+00:00",
"commented_code": "<?php\n\nnamespace Illuminate\\Support;\n\nuse Illuminate\\Contracts\\Support\\Arrayable;\nuse Stringable;\n\nclass Url implements Arrayable, Stringable\n{\n /**\n * Constructor.\n */\n public function __construct(\n public ?string $scheme = null,\n public ?string $host = null,\n public ?int $port = null,\n public ?string $user = null,\n public ?string $pass = null,",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1837330959",
"repo_full_name": "laravel/framework",
"pr_number": 53370,
"pr_file": "src/Illuminate/Support/Url.php",
"discussion_id": "1837330959",
"commented_code": "@@ -0,0 +1,113 @@\n+<?php\n+\n+namespace Illuminate\\Support;\n+\n+use Illuminate\\Contracts\\Support\\Arrayable;\n+use Stringable;\n+\n+class Url implements Arrayable, Stringable\n+{\n+ /**\n+ * Constructor.\n+ */\n+ public function __construct(\n+ public ?string $scheme = null,\n+ public ?string $host = null,\n+ public ?int $port = null,\n+ public ?string $user = null,\n+ public ?string $pass = null,",
"comment_created_at": "2024-11-12T00:21:55+00:00",
"comment_author": "timacdonald",
"comment_body": "Should we mark this as sensitive so it does not end up in logs / stacktraces?\r\n\r\n```suggestion\r\n #[\\SensitiveParameter]\r\n public ?string $pass = null,\r\n```",
"pr_file_module": null
},
{
"comment_id": "1837332870",
"repo_full_name": "laravel/framework",
"pr_number": 53370,
"pr_file": "src/Illuminate/Support/Url.php",
"discussion_id": "1837330959",
"commented_code": "@@ -0,0 +1,113 @@\n+<?php\n+\n+namespace Illuminate\\Support;\n+\n+use Illuminate\\Contracts\\Support\\Arrayable;\n+use Stringable;\n+\n+class Url implements Arrayable, Stringable\n+{\n+ /**\n+ * Constructor.\n+ */\n+ public function __construct(\n+ public ?string $scheme = null,\n+ public ?string $host = null,\n+ public ?int $port = null,\n+ public ?string $user = null,\n+ public ?string $pass = null,",
"comment_created_at": "2024-11-12T00:25:35+00:00",
"comment_author": "stevebauman",
"comment_body": "Yup I agree!",
"pr_file_module": null
}
]
}
]

256
_reviewers/framework-name-indicates-clear-purpose.md Normal file
View File

@@ -0,0 +1,256 @@
---
title: Name indicates clear purpose
description: Names should clearly indicate their purpose, type, and behavior. This
applies to methods, variables, and parameters. Choose names that are self-documenting
and unambiguous.
repository: laravel/framework
label: Naming Conventions
language: PHP
comments_count: 6
repository_stars: 33763
---
Names should clearly indicate their purpose, type, and behavior. This applies to methods, variables, and parameters. Choose names that are self-documenting and unambiguous.
Key guidelines:
- Use prefixes for boolean methods (is, has, can)
- Use accurate type indicators (integer vs number)
- Choose domain-specific terms (digits vs length for numbers)
- Avoid abbreviated or ambiguous names
Example:
```php
// Unclear naming
public function pan($value) { }
public function num($len = 6) { }
// Clear naming
public function formatPanNumber($value) { }
public function generateRandomInteger(int $digits = 6) { }
// Boolean method naming
public function arrayable($value) { } // Unclear purpose
public function isArrayable($value) { } // Clear purpose
```
This standard helps prevent confusion, makes code self-documenting, and maintains consistency across the codebase.
[
{
"discussion_id": "2174034334",
"pr_number": 56169,
"pr_file": "tests/Database/DatabaseEloquentBuilderFindOrFailWithEnumTest.php",
"created_at": "2025-06-30T00:51:53+00:00",
"commented_code": "<?php\n\nnamespace Illuminate\\Tests\\Database;\n\nuse Illuminate\\Database\\Eloquent\\Model;\nuse Illuminate\\Database\\Eloquent\\ModelNotFoundException;\nuse Illuminate\\Database\\Schema\\Blueprint;\nuse Illuminate\\Support\\Facades\\Schema;\nuse Orchestra\\Testbench\\TestCase;\nuse PHPUnit\\Framework\\Attributes\\Test;\n\nclass DatabaseEloquentBuilderFindOrFailWithEnumTest extends TestCase\n{\n protected function setUp(): void\n {\n parent::setUp();\n\n $this->createSchema();\n }\n\n protected function tearDown(): void\n {\n Schema::drop('test_models');\n\n parent::tearDown();\n }\n\n protected function getEnvironmentSetUp($app)\n {\n $app['config']->set('database.default', 'testing');\n }\n\n #[Test]\n public function it_finds_existing_model_when_using_enum_id()\n {\n EloquentBuilderFindOrFailWithEnumTestModel::create(['id' => 1, 'name' => 'one']);\n\n $model = EloquentBuilderFindOrFailWithEnumTestModel::findOrFail(EloquentBuilderFindOrFailWithEnumTestBackedEnum::One);\n\n $this->assertInstanceOf(EloquentBuilderFindOrFailWithEnumTestModel::class, $model);\n $this->assertTrue($model->exists);\n $this->assertEquals(1, $model->id);\n }\n\n #[Test]\n public function it_throws_exception_when_enum_id_does_not_exist()",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2174034334",
"repo_full_name": "laravel/framework",
"pr_number": 56169,
"pr_file": "tests/Database/DatabaseEloquentBuilderFindOrFailWithEnumTest.php",
"discussion_id": "2174034334",
"commented_code": "@@ -0,0 +1,74 @@\n+<?php\n+\n+namespace Illuminate\\Tests\\Database;\n+\n+use Illuminate\\Database\\Eloquent\\Model;\n+use Illuminate\\Database\\Eloquent\\ModelNotFoundException;\n+use Illuminate\\Database\\Schema\\Blueprint;\n+use Illuminate\\Support\\Facades\\Schema;\n+use Orchestra\\Testbench\\TestCase;\n+use PHPUnit\\Framework\\Attributes\\Test;\n+\n+class DatabaseEloquentBuilderFindOrFailWithEnumTest extends TestCase\n+{\n+ protected function setUp(): void\n+ {\n+ parent::setUp();\n+\n+ $this->createSchema();\n+ }\n+\n+ protected function tearDown(): void\n+ {\n+ Schema::drop('test_models');\n+\n+ parent::tearDown();\n+ }\n+\n+ protected function getEnvironmentSetUp($app)\n+ {\n+ $app['config']->set('database.default', 'testing');\n+ }\n+\n+ #[Test]\n+ public function it_finds_existing_model_when_using_enum_id()\n+ {\n+ EloquentBuilderFindOrFailWithEnumTestModel::create(['id' => 1, 'name' => 'one']);\n+\n+ $model = EloquentBuilderFindOrFailWithEnumTestModel::findOrFail(EloquentBuilderFindOrFailWithEnumTestBackedEnum::One);\n+\n+ $this->assertInstanceOf(EloquentBuilderFindOrFailWithEnumTestModel::class, $model);\n+ $this->assertTrue($model->exists);\n+ $this->assertEquals(1, $model->id);\n+ }\n+\n+ #[Test]\n+ public function it_throws_exception_when_enum_id_does_not_exist()",
"comment_created_at": "2025-06-30T00:51:53+00:00",
"comment_author": "crynobone",
"comment_body": "We don't use `Test` attribute in the framework, prefix the method name with `test` and use studly case.",
"pr_file_module": null
}
]
},
{
"discussion_id": "2084631773",
"pr_number": 55715,
"pr_file": "src/Illuminate/Collections/Arr.php",
"created_at": "2025-05-12T13:05:05+00:00",
"commented_code": "return is_array($value) || $value instanceof ArrayAccess;\n }\n\n /**\n * Determine whether the given value is arrayable.\n *\n * @param mixed $value\n * @return bool\n */\n public static function arrayable($value)",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2084631773",
"repo_full_name": "laravel/framework",
"pr_number": 55715,
"pr_file": "src/Illuminate/Collections/Arr.php",
"discussion_id": "2084631773",
"commented_code": "@@ -23,6 +28,21 @@ public static function accessible($value)\n return is_array($value) || $value instanceof ArrayAccess;\n }\n \n+ /**\n+ * Determine whether the given value is arrayable.\n+ *\n+ * @param mixed $value\n+ * @return bool\n+ */\n+ public static function arrayable($value)",
"comment_created_at": "2025-05-12T13:05:05+00:00",
"comment_author": "shaedrich",
"comment_body": "Wouldn't it make sense to use a prefix here to indicate the return value? Because, I would expect the method to convert it from array-like values to `Arrayable` instances\r\n```suggestion\r\n public static function isArrayable($value)\r\n```",
"pr_file_module": null
},
{
"comment_id": "2084658378",
"repo_full_name": "laravel/framework",
"pr_number": 55715,
"pr_file": "src/Illuminate/Collections/Arr.php",
"discussion_id": "2084631773",
"commented_code": "@@ -23,6 +28,21 @@ public static function accessible($value)\n return is_array($value) || $value instanceof ArrayAccess;\n }\n \n+ /**\n+ * Determine whether the given value is arrayable.\n+ *\n+ * @param mixed $value\n+ * @return bool\n+ */\n+ public static function arrayable($value)",
"comment_created_at": "2025-05-12T13:18:12+00:00",
"comment_author": "daniser",
"comment_body": "I followed naming convention used in `Arr::accessible`.",
"pr_file_module": null
},
{
"comment_id": "2084679790",
"repo_full_name": "laravel/framework",
"pr_number": 55715,
"pr_file": "src/Illuminate/Collections/Arr.php",
"discussion_id": "2084631773",
"commented_code": "@@ -23,6 +28,21 @@ public static function accessible($value)\n return is_array($value) || $value instanceof ArrayAccess;\n }\n \n+ /**\n+ * Determine whether the given value is arrayable.\n+ *\n+ * @param mixed $value\n+ * @return bool\n+ */\n+ public static function arrayable($value)",
"comment_created_at": "2025-05-12T13:28:41+00:00",
"comment_author": "shaedrich",
"comment_body": "Ah, thanks for the explanation \ud83d\udc4d\ud83c\udffb ",
"pr_file_module": null
}
]
},
{
"discussion_id": "1898695749",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"created_at": "2024-12-27T20:06:02+00:00",
"commented_code": "return static::$currency;\n }\n\n /**\n * Generate a random number of the given length.",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1898695749",
"repo_full_name": "laravel/framework",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "1898695749",
"commented_code": "@@ -367,6 +367,30 @@ public static function defaultCurrency()\n return static::$currency;\n }\n \n+ /**\n+ * Generate a random number of the given length.",
"comment_created_at": "2024-12-27T20:06:02+00:00",
"comment_author": "shaedrich",
"comment_body": "\"number\" might be misleading here:\r\n```suggestion\r\n * Generate a random integer of the given length.\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1898696764",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"created_at": "2024-12-27T20:08:52+00:00",
"commented_code": "return static::$currency;\n }\n\n /**\n * Generate a random number of the given length.\n *\n * @param int $length\n * @return int\n */\n public static function random(int $length = 6): int\n {\n $maxLength = strlen((string) PHP_INT_MAX);\n\n if ($length < 1 || $length > $maxLength) {\n return 0;\n }\n\n $min = 10 ** ($length - 1);\n $max = (10 ** $length) - 1;\n\n if ($length == $maxLength) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1898696764",
"repo_full_name": "laravel/framework",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "1898696764",
"commented_code": "@@ -367,6 +367,30 @@ public static function defaultCurrency()\n return static::$currency;\n }\n \n+ /**\n+ * Generate a random number of the given length.\n+ *\n+ * @param int $length\n+ * @return int\n+ */\n+ public static function random(int $length = 6): int\n+ {\n+ $maxLength = strlen((string) PHP_INT_MAX);\n+\n+ if ($length < 1 || $length > $maxLength) {\n+ return 0;\n+ }\n+\n+ $min = 10 ** ($length - 1);\n+ $max = (10 ** $length) - 1;\n+\n+ if ($length == $maxLength) {",
"comment_created_at": "2024-12-27T20:08:52+00:00",
"comment_author": "shaedrich",
"comment_body": "Even though, you are using `strlen()` here, I'd say, \"digits\" is the more correct term when dealing with numbers:\r\n```suggestion\r\n * Generate a random number with the given amount of digits.\r\n *\r\n * @param int $digits\r\n * @return int\r\n */\r\n public static function random(int $digits = 6): int\r\n {\r\n $maxDigits = strlen((string) PHP_INT_MAX);\r\n\r\n if ($digits < 1 || $digits > $maxDigits) {\r\n return 0;\r\n }\r\n\r\n $min = 10 ** ($digits - 1);\r\n $max = (10 ** $digits) - 1;\r\n\r\n if ($digits == $maxDigits) {\r\n```",
"pr_file_module": null
},
{
"comment_id": "1900456982",
"repo_full_name": "laravel/framework",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "1898696764",
"commented_code": "@@ -367,6 +367,30 @@ public static function defaultCurrency()\n return static::$currency;\n }\n \n+ /**\n+ * Generate a random number of the given length.\n+ *\n+ * @param int $length\n+ * @return int\n+ */\n+ public static function random(int $length = 6): int\n+ {\n+ $maxLength = strlen((string) PHP_INT_MAX);\n+\n+ if ($length < 1 || $length > $maxLength) {\n+ return 0;\n+ }\n+\n+ $min = 10 ** ($length - 1);\n+ $max = (10 ** $length) - 1;\n+\n+ if ($length == $maxLength) {",
"comment_created_at": "2025-01-01T20:30:55+00:00",
"comment_author": "dilovanmatini",
"comment_body": "Yes, it is more readable.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1470746887",
"pr_number": 49900,
"pr_file": "src/Illuminate/Translation/Translator.php",
"created_at": "2024-01-30T08:14:28+00:00",
"commented_code": "// the translator was instantiated. Then, we can load the lines and return.\n $locales = $fallback ? $this->localeArray($locale) : [$locale];\n\n foreach ($locales as $locale) {\n foreach ($locales as $_locale) {\n if (! is_null($line = $this->getLine(\n $namespace, $group, $locale, $item, $replace\n $namespace, $group, $_locale, $item, $replace",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1470746887",
"repo_full_name": "laravel/framework",
"pr_number": 49900,
"pr_file": "src/Illuminate/Translation/Translator.php",
"discussion_id": "1470746887",
"commented_code": "@@ -160,9 +160,9 @@ public function get($key, array $replace = [], $locale = null, $fallback = true)\n // the translator was instantiated. Then, we can load the lines and return.\n $locales = $fallback ? $this->localeArray($locale) : [$locale];\n \n- foreach ($locales as $locale) {\n+ foreach ($locales as $_locale) {\n if (! is_null($line = $this->getLine(\n- $namespace, $group, $locale, $item, $replace\n+ $namespace, $group, $_locale, $item, $replace",
"comment_created_at": "2024-01-30T08:14:28+00:00",
"comment_author": "driesvints",
"comment_body": "Use `$languageLineLocale` instead here. `$_` isn't a syntax we use anywhere.",
"pr_file_module": null
},
{
"comment_id": "1470754575",
"repo_full_name": "laravel/framework",
"pr_number": 49900,
"pr_file": "src/Illuminate/Translation/Translator.php",
"discussion_id": "1470746887",
"commented_code": "@@ -160,9 +160,9 @@ public function get($key, array $replace = [], $locale = null, $fallback = true)\n // the translator was instantiated. Then, we can load the lines and return.\n $locales = $fallback ? $this->localeArray($locale) : [$locale];\n \n- foreach ($locales as $locale) {\n+ foreach ($locales as $_locale) {\n if (! is_null($line = $this->getLine(\n- $namespace, $group, $locale, $item, $replace\n+ $namespace, $group, $_locale, $item, $replace",
"comment_created_at": "2024-01-30T08:21:24+00:00",
"comment_author": "VicGUTT",
"comment_body": "Sure. I was wondering about it myself.\r\n\r\nThanks for the feedback; done in: [549de01](https://github.com/laravel/framework/pull/49900/commits/549de01ea9f7fa17b272d9baa04be0fe186b9c26)",
"pr_file_module": null
}
]
},
{
"discussion_id": "1714804417",
"pr_number": 52461,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php",
"created_at": "2024-08-13T07:26:20+00:00",
"commented_code": "<?php\n\nnamespace Illuminate\\Database\\Eloquent\\Concerns;\n\nuse Illuminate\\Support\\Arr;\nuse Illuminate\\Support\\Onceable;\n\ntrait PreventsCircularRecursion\n{\n /**\n * The cache of objects processed to prevent infinite recursion.\n *\n * @var \\WeakMap<static, array<string, mixed>>\n */\n protected static $recursionCache;\n\n /**\n * Get the current recursion cache being used by the model.\n *\n * @return \\WeakMap\n */\n protected static function getRecursionCache()\n {\n return static::$recursionCache ??= new \\WeakMap();\n }\n\n /**\n * Get the current stack of methods being called recursively.\n *\n * @param object $object\n * @return array\n */\n protected static function getRecursiveCallStack($object): array\n {\n return static::getRecursionCache()->offsetExists($object)\n ? static::getRecursionCache()->offsetGet($object)\n : [];\n }\n\n /**\n * Prevent a method from being called multiple times on the same object within the same call stack.\n *\n * @param callable $callback\n * @param mixed $default\n * @return mixed\n */\n protected function once($callback, $default = null)",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1714804417",
"repo_full_name": "laravel/framework",
"pr_number": 52461,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php",
"discussion_id": "1714804417",
"commented_code": "@@ -0,0 +1,74 @@\n+<?php\n+\n+namespace Illuminate\\Database\\Eloquent\\Concerns;\n+\n+use Illuminate\\Support\\Arr;\n+use Illuminate\\Support\\Onceable;\n+\n+trait PreventsCircularRecursion\n+{\n+ /**\n+ * The cache of objects processed to prevent infinite recursion.\n+ *\n+ * @var \\WeakMap<static, array<string, mixed>>\n+ */\n+ protected static $recursionCache;\n+\n+ /**\n+ * Get the current recursion cache being used by the model.\n+ *\n+ * @return \\WeakMap\n+ */\n+ protected static function getRecursionCache()\n+ {\n+ return static::$recursionCache ??= new \\WeakMap();\n+ }\n+\n+ /**\n+ * Get the current stack of methods being called recursively.\n+ *\n+ * @param object $object\n+ * @return array\n+ */\n+ protected static function getRecursiveCallStack($object): array\n+ {\n+ return static::getRecursionCache()->offsetExists($object)\n+ ? static::getRecursionCache()->offsetGet($object)\n+ : [];\n+ }\n+\n+ /**\n+ * Prevent a method from being called multiple times on the same object within the same call stack.\n+ *\n+ * @param callable $callback\n+ * @param mixed $default\n+ * @return mixed\n+ */\n+ protected function once($callback, $default = null)",
"comment_created_at": "2024-08-13T07:26:20+00:00",
"comment_author": "Tofandel",
"comment_body": "I do think following laravel's pattern this should be renamed to `withoutInfiniteRecursion` or `withoutCircularRecursion`\r\n\r\nAs you've explained it has a very different behavior than the once helper and could be confusing from a readability standpoint",
"pr_file_module": null
},
{
"comment_id": "1714811116",
"repo_full_name": "laravel/framework",
"pr_number": 52461,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php",
"discussion_id": "1714804417",
"commented_code": "@@ -0,0 +1,74 @@\n+<?php\n+\n+namespace Illuminate\\Database\\Eloquent\\Concerns;\n+\n+use Illuminate\\Support\\Arr;\n+use Illuminate\\Support\\Onceable;\n+\n+trait PreventsCircularRecursion\n+{\n+ /**\n+ * The cache of objects processed to prevent infinite recursion.\n+ *\n+ * @var \\WeakMap<static, array<string, mixed>>\n+ */\n+ protected static $recursionCache;\n+\n+ /**\n+ * Get the current recursion cache being used by the model.\n+ *\n+ * @return \\WeakMap\n+ */\n+ protected static function getRecursionCache()\n+ {\n+ return static::$recursionCache ??= new \\WeakMap();\n+ }\n+\n+ /**\n+ * Get the current stack of methods being called recursively.\n+ *\n+ * @param object $object\n+ * @return array\n+ */\n+ protected static function getRecursiveCallStack($object): array\n+ {\n+ return static::getRecursionCache()->offsetExists($object)\n+ ? static::getRecursionCache()->offsetGet($object)\n+ : [];\n+ }\n+\n+ /**\n+ * Prevent a method from being called multiple times on the same object within the same call stack.\n+ *\n+ * @param callable $callback\n+ * @param mixed $default\n+ * @return mixed\n+ */\n+ protected function once($callback, $default = null)",
"comment_created_at": "2024-08-13T07:31:08+00:00",
"comment_author": "samlev",
"comment_body": "I had actuall named it `withoutRecursion()` originally then changed it to `once()`",
"pr_file_module": null
},
{
"comment_id": "1714829338",
"repo_full_name": "laravel/framework",
"pr_number": 52461,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php",
"discussion_id": "1714804417",
"commented_code": "@@ -0,0 +1,74 @@\n+<?php\n+\n+namespace Illuminate\\Database\\Eloquent\\Concerns;\n+\n+use Illuminate\\Support\\Arr;\n+use Illuminate\\Support\\Onceable;\n+\n+trait PreventsCircularRecursion\n+{\n+ /**\n+ * The cache of objects processed to prevent infinite recursion.\n+ *\n+ * @var \\WeakMap<static, array<string, mixed>>\n+ */\n+ protected static $recursionCache;\n+\n+ /**\n+ * Get the current recursion cache being used by the model.\n+ *\n+ * @return \\WeakMap\n+ */\n+ protected static function getRecursionCache()\n+ {\n+ return static::$recursionCache ??= new \\WeakMap();\n+ }\n+\n+ /**\n+ * Get the current stack of methods being called recursively.\n+ *\n+ * @param object $object\n+ * @return array\n+ */\n+ protected static function getRecursiveCallStack($object): array\n+ {\n+ return static::getRecursionCache()->offsetExists($object)\n+ ? static::getRecursionCache()->offsetGet($object)\n+ : [];\n+ }\n+\n+ /**\n+ * Prevent a method from being called multiple times on the same object within the same call stack.\n+ *\n+ * @param callable $callback\n+ * @param mixed $default\n+ * @return mixed\n+ */\n+ protected function once($callback, $default = null)",
"comment_created_at": "2024-08-13T07:44:44+00:00",
"comment_author": "Tofandel",
"comment_body": "That also works, as long as it makes the code more readable in `Model` so you can understand without having to take a look inside the function",
"pr_file_module": null
},
{
"comment_id": "1715001879",
"repo_full_name": "laravel/framework",
"pr_number": 52461,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php",
"discussion_id": "1714804417",
"commented_code": "@@ -0,0 +1,74 @@\n+<?php\n+\n+namespace Illuminate\\Database\\Eloquent\\Concerns;\n+\n+use Illuminate\\Support\\Arr;\n+use Illuminate\\Support\\Onceable;\n+\n+trait PreventsCircularRecursion\n+{\n+ /**\n+ * The cache of objects processed to prevent infinite recursion.\n+ *\n+ * @var \\WeakMap<static, array<string, mixed>>\n+ */\n+ protected static $recursionCache;\n+\n+ /**\n+ * Get the current recursion cache being used by the model.\n+ *\n+ * @return \\WeakMap\n+ */\n+ protected static function getRecursionCache()\n+ {\n+ return static::$recursionCache ??= new \\WeakMap();\n+ }\n+\n+ /**\n+ * Get the current stack of methods being called recursively.\n+ *\n+ * @param object $object\n+ * @return array\n+ */\n+ protected static function getRecursiveCallStack($object): array\n+ {\n+ return static::getRecursionCache()->offsetExists($object)\n+ ? static::getRecursionCache()->offsetGet($object)\n+ : [];\n+ }\n+\n+ /**\n+ * Prevent a method from being called multiple times on the same object within the same call stack.\n+ *\n+ * @param callable $callback\n+ * @param mixed $default\n+ * @return mixed\n+ */\n+ protected function once($callback, $default = null)",
"comment_created_at": "2024-08-13T09:44:11+00:00",
"comment_author": "samlev",
"comment_body": "I've updated this back to `withoutRecursion()`",
"pr_file_module": null
}
]
}
]

317
_reviewers/framework-optimize-for-code-readability.md Normal file
View File

@@ -0,0 +1,317 @@
---
title: Optimize for code readability
description: 'Prioritize code readability over clever solutions by:
1. Using early returns to reduce nesting
2. Leveraging modern PHP features when they improve clarity'
repository: laravel/framework
label: Code Style
language: PHP
comments_count: 10
repository_stars: 33763
---
Prioritize code readability over clever solutions by:
1. Using early returns to reduce nesting
2. Leveraging modern PHP features when they improve clarity
3. Maintaining consistent style patterns
4. Simplifying complex logic
Example - Before:
```php
protected function parseIds($value)
{
if (is_null($value)) {
return [];
}
if (is_string($value)) {
return array_map('trim', explode(',', $value));
}
check_type($value, 'array', $key, 'Environment');
return $value;
}
```
Example - After:
```php
protected function parseIds($value)
{
return match (true) {
$value === null => [],
is_string($value) => array_map('trim', explode(',', $value)),
default => check_type($value, 'array', $key, 'Environment'),
};
}
```
The improved version:
- Uses match expression for cleaner flow control
- Maintains consistent null comparison style
- Reduces nesting and cognitive load
- Leverages modern PHP features appropriately
Choose simpler constructs when they improve readability, but avoid sacrificing clarity for brevity. The goal is to write code that is easy to understand and maintain.
[
{
"discussion_id": "2101199329",
"pr_number": 55810,
"pr_file": "src/Illuminate/Filesystem/FilesystemAdapter.php",
"created_at": "2025-05-21T21:27:36+00:00",
"commented_code": "*/\n public function url($path)\n {\n if (isset($this->config['prefix'])) {\n if (isset($this->config['prefix']) && ! empty($this->config['prefix'])) {\n $path = $this->concatPathToUrl($this->config['prefix'], $path);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2101199329",
"repo_full_name": "laravel/framework",
"pr_number": 55810,
"pr_file": "src/Illuminate/Filesystem/FilesystemAdapter.php",
"discussion_id": "2101199329",
"commented_code": "@@ -725,7 +725,7 @@ public function writeStream($path, $resource, array $options = [])\n */\n public function url($path)\n {\n- if (isset($this->config['prefix'])) {\n+ if (isset($this->config['prefix']) && ! empty($this->config['prefix'])) {\n $path = $this->concatPathToUrl($this->config['prefix'], $path);",
"comment_created_at": "2025-05-21T21:27:36+00:00",
"comment_author": "shaedrich",
"comment_body": "Shouldn't this be handled _inside_ the `concatPathToUrl()` method?",
"pr_file_module": null
},
{
"comment_id": "2101227158",
"repo_full_name": "laravel/framework",
"pr_number": 55810,
"pr_file": "src/Illuminate/Filesystem/FilesystemAdapter.php",
"discussion_id": "2101199329",
"commented_code": "@@ -725,7 +725,7 @@ public function writeStream($path, $resource, array $options = [])\n */\n public function url($path)\n {\n- if (isset($this->config['prefix'])) {\n+ if (isset($this->config['prefix']) && ! empty($this->config['prefix'])) {\n $path = $this->concatPathToUrl($this->config['prefix'], $path);",
"comment_created_at": "2025-05-21T21:50:07+00:00",
"comment_author": "MrTuffaha",
"comment_body": "good point",
"pr_file_module": null
}
]
},
{
"discussion_id": "2101260253",
"pr_number": 55810,
"pr_file": "src/Illuminate/Filesystem/FilesystemAdapter.php",
"created_at": "2025-05-21T22:12:45+00:00",
"commented_code": "*/\n protected function concatPathToUrl($url, $path)\n {\n if (empty($url)) {\n return trim($path, '/');\n }\n\n if (empty($path)) {\n return trim($url, '/');\n }\n\n return rtrim($url, '/').'/'.ltrim($path, '/');",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2101260253",
"repo_full_name": "laravel/framework",
"pr_number": 55810,
"pr_file": "src/Illuminate/Filesystem/FilesystemAdapter.php",
"discussion_id": "2101260253",
"commented_code": "@@ -847,6 +847,14 @@ public function temporaryUploadUrl($path, $expiration, array $options = [])\n */\n protected function concatPathToUrl($url, $path)\n {\n+ if (empty($url)) {\n+ return trim($path, '/');\n+ }\n+\n+ if (empty($path)) {\n+ return trim($url, '/');\n+ }\n+\n return rtrim($url, '/').'/'.ltrim($path, '/');",
"comment_created_at": "2025-05-21T22:12:45+00:00",
"comment_author": "shaedrich",
"comment_body": "You could even shorten that, if you want:\r\n```suggestion\r\n return match (true) {\r\n empty($url) => trim($path, '/'),\r\n empty($path) => trim($url, '/'),\r\n default => rtrim($url, '/').'/'.ltrim($path, '/'),\r\n };\r\n```",
"pr_file_module": null
},
{
"comment_id": "2101264645",
"repo_full_name": "laravel/framework",
"pr_number": 55810,
"pr_file": "src/Illuminate/Filesystem/FilesystemAdapter.php",
"discussion_id": "2101260253",
"commented_code": "@@ -847,6 +847,14 @@ public function temporaryUploadUrl($path, $expiration, array $options = [])\n */\n protected function concatPathToUrl($url, $path)\n {\n+ if (empty($url)) {\n+ return trim($path, '/');\n+ }\n+\n+ if (empty($path)) {\n+ return trim($url, '/');\n+ }\n+\n return rtrim($url, '/').'/'.ltrim($path, '/');",
"comment_created_at": "2025-05-21T22:17:40+00:00",
"comment_author": "MrTuffaha",
"comment_body": "While i do like shorter code, i do belive that my code is easier to inderstand/more readable, at least at first glance.",
"pr_file_module": null
},
{
"comment_id": "2102187470",
"repo_full_name": "laravel/framework",
"pr_number": 55810,
"pr_file": "src/Illuminate/Filesystem/FilesystemAdapter.php",
"discussion_id": "2101260253",
"commented_code": "@@ -847,6 +847,14 @@ public function temporaryUploadUrl($path, $expiration, array $options = [])\n */\n protected function concatPathToUrl($url, $path)\n {\n+ if (empty($url)) {\n+ return trim($path, '/');\n+ }\n+\n+ if (empty($path)) {\n+ return trim($url, '/');\n+ }\n+\n return rtrim($url, '/').'/'.ltrim($path, '/');",
"comment_created_at": "2025-05-22T10:11:49+00:00",
"comment_author": "shaedrich",
"comment_body": "Yeah, no problem \ud83d\ude03 :+1:",
"pr_file_module": null
}
]
},
{
"discussion_id": "1975059131",
"pr_number": 54845,
"pr_file": "src/Illuminate/Validation/Validator.php",
"created_at": "2025-02-28T09:05:49+00:00",
"commented_code": "*/\n protected function validateUsingCustomRule($attribute, $value, $rule)\n {\n $attribute = $this->replacePlaceholderInString($attribute);\n $originalAttribute = $this->replacePlaceholderInString($attribute);\n\n $attribute = match (true) {\n $rule instanceof Rules\\File => $attribute,\n default => $originalAttribute,\n };",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1975059131",
"repo_full_name": "laravel/framework",
"pr_number": 54845,
"pr_file": "src/Illuminate/Validation/Validator.php",
"discussion_id": "1975059131",
"commented_code": "@@ -872,7 +872,12 @@ protected function hasNotFailedPreviousRuleIfPresenceRule($rule, $attribute)\n */\n protected function validateUsingCustomRule($attribute, $value, $rule)\n {\n- $attribute = $this->replacePlaceholderInString($attribute);\n+ $originalAttribute = $this->replacePlaceholderInString($attribute);\n+\n+ $attribute = match (true) {\n+ $rule instanceof Rules\\File => $attribute,\n+ default => $originalAttribute,\n+ };",
"comment_created_at": "2025-02-28T09:05:49+00:00",
"comment_author": "shaedrich",
"comment_body": "match statements with only two cases kinda defeat the purpose, I'd say\r\n```suggestion\r\n $originalAttribute = $this->replacePlaceholderInString($attribute);\r\n\r\n $attribute = $rule instanceof Rules\\File\r\n ? $attribute\r\n : $this->replacePlaceholderInString($attribute);\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1868256502",
"pr_number": 53748,
"pr_file": "src/Illuminate/Database/Console/PruneCommand.php",
"created_at": "2024-12-03T19:08:18+00:00",
"commented_code": "protected function getPath()\n {\n if (! empty($path = $this->option('path'))) {\n return (new Collection($path))->map(function ($path) {\n return base_path($path);\n })->all();\n return (new Collection($path))\n ->map(fn ($path) => base_path($path))",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1868256502",
"repo_full_name": "laravel/framework",
"pr_number": 53748,
"pr_file": "src/Illuminate/Database/Console/PruneCommand.php",
"discussion_id": "1868256502",
"commented_code": "@@ -157,9 +157,9 @@ protected function models()\n protected function getPath()\n {\n if (! empty($path = $this->option('path'))) {\n- return (new Collection($path))->map(function ($path) {\n- return base_path($path);\n- })->all();\n+ return (new Collection($path))\n+ ->map(fn ($path) => base_path($path))",
"comment_created_at": "2024-12-03T19:08:18+00:00",
"comment_author": "shaedrich",
"comment_body": "This can be just\r\n```suggestion\r\n ->map('base_path')\r\n```\r\nor\r\n```suggestion\r\n ->map(base_path(...))\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1868258127",
"pr_number": 53748,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"created_at": "2024-12-03T19:09:48+00:00",
"commented_code": "*/\n public function qualifyColumns($columns)\n {\n return (new BaseCollection($columns))->map(function ($column) {\n return $this->qualifyColumn($column);\n })->all();\n return (new BaseCollection($columns))\n ->map(fn ($column) => $this->qualifyColumn($column))",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1868258127",
"repo_full_name": "laravel/framework",
"pr_number": 53748,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"discussion_id": "1868258127",
"commented_code": "@@ -600,9 +600,9 @@ public function qualifyColumn($column)\n */\n public function qualifyColumns($columns)\n {\n- return (new BaseCollection($columns))->map(function ($column) {\n- return $this->qualifyColumn($column);\n- })->all();\n+ return (new BaseCollection($columns))\n+ ->map(fn ($column) => $this->qualifyColumn($column))",
"comment_created_at": "2024-12-03T19:09:48+00:00",
"comment_author": "shaedrich",
"comment_body": "This can just be\r\n```suggestion\r\n ->map([ $this, 'qualifyColumn' ])\r\n```\r\n\r\nor\r\n```suggestion\r\n ->map($this->qualifyColumn(...))\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1868261023",
"pr_number": 53748,
"pr_file": "src/Illuminate/Database/Query/Grammars/PostgresGrammar.php",
"created_at": "2024-12-03T19:12:24+00:00",
"commented_code": "{\n $quote = func_num_args() === 2 ? func_get_arg(1) : \"'\";\n\n return (new Collection($path))->map(function ($attribute) {\n return $this->parseJsonPathArrayKeys($attribute);\n })->collapse()->map(function ($attribute) use ($quote) {\n return filter_var($attribute, FILTER_VALIDATE_INT) !== false\n ? $attribute\n : $quote.$attribute.$quote;\n })->all();\n return (new Collection($path))\n ->map(fn ($attribute) => $this->parseJsonPathArrayKeys($attribute))",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1868261023",
"repo_full_name": "laravel/framework",
"pr_number": 53748,
"pr_file": "src/Illuminate/Database/Query/Grammars/PostgresGrammar.php",
"discussion_id": "1868261023",
"commented_code": "@@ -734,13 +736,15 @@ protected function wrapJsonPathAttributes($path)\n {\n $quote = func_num_args() === 2 ? func_get_arg(1) : \"'\";\n \n- return (new Collection($path))->map(function ($attribute) {\n- return $this->parseJsonPathArrayKeys($attribute);\n- })->collapse()->map(function ($attribute) use ($quote) {\n- return filter_var($attribute, FILTER_VALIDATE_INT) !== false\n- ? $attribute\n- : $quote.$attribute.$quote;\n- })->all();\n+ return (new Collection($path))\n+ ->map(fn ($attribute) => $this->parseJsonPathArrayKeys($attribute))",
"comment_created_at": "2024-12-03T19:12:24+00:00",
"comment_author": "shaedrich",
"comment_body": "This can be simplified to just\r\n```suggestion\r\n ->map([ $this, 'parseJsonPathArrayKeys' ])\r\n```\r\n\r\nor\r\n```suggestion\r\n ->map($this->parseJsonPathArrayKeys(...))\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1977480863",
"pr_number": 54874,
"pr_file": "src/Illuminate/Collections/helpers.php",
"created_at": "2025-03-03T12:59:21+00:00",
"commented_code": "}\n}\n\nif (! function_exists('deepCollect')) {\n /**\n * Recursively convert an array and its nested arrays into Laravel collections.\n *\n * This function takes an array (or any iterable) and transforms it into an instance\n * of Laravel's Collection class. It applies the transformation to all nested arrays,\n * ensuring that every level of the array structure is converted into collections.\n *\n * @param mixed $value The input value, which can be an array, an iterable, or any other data type.\n * @return mixed If the value is an array, a Collection instance is returned with all nested arrays\n * converted. If the value is not an array, it is returned as-is.\n */\n function deepCollect($value)\n {\n // If the value is an array, convert it into a Collection and apply deepCollect recursively\n if (is_array($value)) {\n return collect($value)->map(fn ($item) => deepCollect($item));",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1977480863",
"repo_full_name": "laravel/framework",
"pr_number": 54874,
"pr_file": "src/Illuminate/Collections/helpers.php",
"discussion_id": "1977480863",
"commented_code": "@@ -19,6 +19,30 @@ function collect($value = [])\n }\n }\n \n+if (! function_exists('deepCollect')) {\n+ /**\n+ * Recursively convert an array and its nested arrays into Laravel collections.\n+ *\n+ * This function takes an array (or any iterable) and transforms it into an instance\n+ * of Laravel's Collection class. It applies the transformation to all nested arrays,\n+ * ensuring that every level of the array structure is converted into collections.\n+ *\n+ * @param mixed $value The input value, which can be an array, an iterable, or any other data type.\n+ * @return mixed If the value is an array, a Collection instance is returned with all nested arrays\n+ * converted. If the value is not an array, it is returned as-is.\n+ */\n+ function deepCollect($value)\n+ {\n+ // If the value is an array, convert it into a Collection and apply deepCollect recursively\n+ if (is_array($value)) {\n+ return collect($value)->map(fn ($item) => deepCollect($item));",
"comment_created_at": "2025-03-03T12:59:21+00:00",
"comment_author": "shaedrich",
"comment_body": "You might be able to use first-class callable syntax here:\r\n```suggestion\r\n return collect($value)->map(deepCollect(...));\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1953107378",
"pr_number": 54582,
"pr_file": "src/Illuminate/Database/Eloquent/Filterable.php",
"created_at": "2025-02-12T17:28:02+00:00",
"commented_code": "<?php\n\nnamespace Illuminate\\Database\\Eloquent;\n\nuse Illuminate\\Database\\Eloquent\\Builder;\n\ntrait Filterable\n{\n\n protected $filterNamespace = 'App\\Filters\\\\';\n\n public function scopeFilter(Builder $builder, array $params): void\n {\n if (is_array($params) and !empty($params)) {\n\n foreach ($params as $class => $methodOrValue) {\n\n $className = $this->filterNamespace . ucfirst($class);\n\n if (class_exists($className)) {\n\n if (method_exists($className, $methodOrValue))\n $className::{$methodOrValue}($builder);\n\n if (method_exists($className, 'apply'))\n $className::apply($builder, $methodOrValue);\n }\n }\n }\n }",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1953107378",
"repo_full_name": "laravel/framework",
"pr_number": 54582,
"pr_file": "src/Illuminate/Database/Eloquent/Filterable.php",
"discussion_id": "1953107378",
"commented_code": "@@ -0,0 +1,36 @@\n+<?php\n+\n+namespace Illuminate\\Database\\Eloquent;\n+\n+use Illuminate\\Database\\Eloquent\\Builder;\n+\n+trait Filterable\n+{\n+\n+ protected $filterNamespace = 'App\\Filters\\\\';\n+\n+ public function scopeFilter(Builder $builder, array $params): void\n+ {\n+ if (is_array($params) and !empty($params)) {\n+\n+ foreach ($params as $class => $methodOrValue) {\n+\n+ $className = $this->filterNamespace . ucfirst($class);\n+\n+ if (class_exists($className)) {\n+\n+ if (method_exists($className, $methodOrValue))\n+ $className::{$methodOrValue}($builder);\n+\n+ if (method_exists($className, 'apply'))\n+ $className::apply($builder, $methodOrValue);\n+ }\n+ }\n+ }\n+ }",
"comment_created_at": "2025-02-12T17:28:02+00:00",
"comment_author": "shaedrich",
"comment_body": "You might want to use early returns here:\r\n```suggestion\r\n public function scopeFilter(Builder $builder, array $params): void\r\n {\r\n if (!is_array($params) || $params === []) {\r\n \treturn;\r\n }\r\n\r\n foreach ($params as $class => $methodOrValue) {\r\n $className = $this->filterNamespace . ucfirst($class);\r\n\r\n if (!class_exists($className)) {\r\n\t\t\t\tcontinue;\r\n }\r\n\r\n if (method_exists($className, $methodOrValue)) {\r\n $className::{$methodOrValue}($builder);\r\n }\r\n\r\n if (method_exists($className, 'apply')) {\r\n $className::apply($builder, $methodOrValue);\r\n }\r\n }\r\n }\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1953141984",
"pr_number": 54582,
"pr_file": "src/Illuminate/View/Compilers/Concerns/CompilesApplied.php",
"created_at": "2025-02-12T17:49:06+00:00",
"commented_code": "<?php\n\nnamespace Illuminate\\View\\Compilers\\Concerns;\n\nuse Illuminate\\Contracts\\View\\ViewCompilationException;\n\ntrait CompilesApplied\n{\n /**\n * Compile the conditional applied statement into valid PHP.\n */\n protected function compileApplied(string|null $expression): string\n {\n preg_match('/^\\(([\\'\"])([^\\'\"]+)\\1,\\1([^\\'\"]+)\\1,\\1([^\\'\"]+)\\1\\)$/', $expression ?? '', $matches);\n\n if (count($matches) === 0)\n throw new ViewCompilationException('Malformed @applied statement.');\n\n $expression = str_replace('(', '', $expression);\n $expression = str_replace(')', '', $expression);\n $result = explode(',', $expression);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1953141984",
"repo_full_name": "laravel/framework",
"pr_number": 54582,
"pr_file": "src/Illuminate/View/Compilers/Concerns/CompilesApplied.php",
"discussion_id": "1953141984",
"commented_code": "@@ -0,0 +1,30 @@\n+<?php\n+\n+namespace Illuminate\\View\\Compilers\\Concerns;\n+\n+use Illuminate\\Contracts\\View\\ViewCompilationException;\n+\n+trait CompilesApplied\n+{\n+ /**\n+ * Compile the conditional applied statement into valid PHP.\n+ */\n+ protected function compileApplied(string|null $expression): string\n+ {\n+ preg_match('/^\\(([\\'\"])([^\\'\"]+)\\1,\\1([^\\'\"]+)\\1,\\1([^\\'\"]+)\\1\\)$/', $expression ?? '', $matches);\n+\n+ if (count($matches) === 0)\n+ throw new ViewCompilationException('Malformed @applied statement.');\n+\n+ $expression = str_replace('(', '', $expression);\n+ $expression = str_replace(')', '', $expression);\n+ $result = explode(',', $expression);",
"comment_created_at": "2025-02-12T17:49:06+00:00",
"comment_author": "shaedrich",
"comment_body": "You can avoid intermediate variables:\r\n```suggestion\r\n $result = Str::of($expression)\r\n \t->replace('(', '')\r\n \t->replace(')', '')\r\n \t->explode(',');\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1935718228",
"pr_number": 54417,
"pr_file": "src/Illuminate/Validation/ValidationRuleParser.php",
"created_at": "2025-01-30T14:37:26+00:00",
"commented_code": "return Arr::wrap($this->prepareRule($rule, $attribute));\n }\n\n $rules = [];\n\n foreach ($rule as $value) {\n if ($value instanceof Date) {\n $rules = array_merge($rules, explode('|', (string) $value));\n } else {\n $rules[] = $this->prepareRule($value, $attribute);\n }\n }\n\n return $rules;\n return array_map(\n [$this, 'prepareRule'],",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1935718228",
"repo_full_name": "laravel/framework",
"pr_number": 54417,
"pr_file": "src/Illuminate/Validation/ValidationRuleParser.php",
"discussion_id": "1935718228",
"commented_code": "@@ -96,17 +95,11 @@ protected function explodeExplicitRule($rule, $attribute)\n return Arr::wrap($this->prepareRule($rule, $attribute));\n }\n \n- $rules = [];\n-\n- foreach ($rule as $value) {\n- if ($value instanceof Date) {\n- $rules = array_merge($rules, explode('|', (string) $value));\n- } else {\n- $rules[] = $this->prepareRule($value, $attribute);\n- }\n- }\n-\n- return $rules;\n+ return array_map(\n+ [$this, 'prepareRule'],",
"comment_created_at": "2025-01-30T14:37:26+00:00",
"comment_author": "shaedrich",
"comment_body": "You could use first-class callable syntax here:\r\n```suggestion\r\n $this->prepareRule(...),\r\n```",
"pr_file_module": null
}
]
}
]

246
_reviewers/framework-optimize-loop-operations.md Normal file
View File

@@ -0,0 +1,246 @@
---
title: Optimize loop operations
description: 'When writing loops, optimize for both readability and performance by
following these key principles:
1. **Exit early** when a decision can be made:
```php'
repository: laravel/framework
label: Algorithms
language: PHP
comments_count: 4
repository_stars: 33763
---
When writing loops, optimize for both readability and performance by following these key principles:
1. **Exit early** when a decision can be made:
```php
// Instead of this:
foreach ($keys as $key) {
if (! static::has($array, $key)) {
$result = false;
}
}
return $result;
// Do this:
foreach ($keys as $key) {
if (! static::has($array, $key)) {
return false;
}
}
return true;
```
2. **Move invariant operations outside loops**:
```php
// Instead of this:
foreach ($array as $key => $item) {
$groupKey = is_callable($groupBy) ? $groupBy($item, $key) : static::get($item, $groupBy);
// ...
}
// Do this:
$groupBy = is_callable($groupBy) ? $groupBy : fn ($item) => static::get($item, $groupBy);
foreach ($array as $key => $item) {
$groupKey = $groupBy($item, $key);
// ...
}
```
3. **Use O(1) operations** where possible instead of O(n):
```php
// Instead of this:
if (in_array(InteractsWithQueue::class, $uses) && in_array(Queueable::class, $uses)) {
// ...
}
// Do this:
if (isset($uses[InteractsWithQueue::class], $uses[Queueable::class])) {
// ...
}
```
4. **Add early termination conditions** for algorithms that can complete before processing all elements:
```php
// Add early exit:
foreach ($map as $roman => $value) {
while ($number >= $value) {
$result .= $roman;
$number -= $value;
}
if ($number === 0) {
return $result;
}
}
```
These optimizations help reduce computational complexity and unnecessary operations, resulting in more efficient and maintainable code.
[
{
"discussion_id": "2101429227",
"pr_number": 55815,
"pr_file": "src/Illuminate/Collections/Arr.php",
"created_at": "2025-05-22T01:05:00+00:00",
"commented_code": "return true;\n }\n\n /**\n * Determine if all keys exist in an array using \"dot\" notation.\n *\n * @param \\ArrayAccess|array $array\n * @param string|array $keys\n * @return bool\n */\n public static function hasAll($array, $keys)\n {\n $keys = (array) $keys;\n\n if (! $array || $keys === []) {\n return false;\n }\n\n $result = true;\n\n foreach ($keys as $key) {\n if (! static::has($array, $key)) {\n $result = false;\n }\n }\n\n return $result;",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2101429227",
"repo_full_name": "laravel/framework",
"pr_number": 55815,
"pr_file": "src/Illuminate/Collections/Arr.php",
"discussion_id": "2101429227",
"commented_code": "@@ -495,6 +495,32 @@ public static function has($array, $keys)\n return true;\n }\n \n+ /**\n+ * Determine if all keys exist in an array using \"dot\" notation.\n+ *\n+ * @param \\ArrayAccess|array $array\n+ * @param string|array $keys\n+ * @return bool\n+ */\n+ public static function hasAll($array, $keys)\n+ {\n+ $keys = (array) $keys;\n+\n+ if (! $array || $keys === []) {\n+ return false;\n+ }\n+\n+ $result = true;\n+\n+ foreach ($keys as $key) {\n+ if (! static::has($array, $key)) {\n+ $result = false;\n+ }\n+ }\n+\n+ return $result;",
"comment_created_at": "2025-05-22T01:05:00+00:00",
"comment_author": "Rizky92",
"comment_body": "Might be better to bail on first failure.\r\n```suggestion\r\n foreach ($keys as $key) {\r\n if (! static::has($array, $key)) {\r\n return false;\r\n }\r\n }\r\n\r\n return true;\r\n```",
"pr_file_module": null
},
{
"comment_id": "2101461078",
"repo_full_name": "laravel/framework",
"pr_number": 55815,
"pr_file": "src/Illuminate/Collections/Arr.php",
"discussion_id": "2101429227",
"commented_code": "@@ -495,6 +495,32 @@ public static function has($array, $keys)\n return true;\n }\n \n+ /**\n+ * Determine if all keys exist in an array using \"dot\" notation.\n+ *\n+ * @param \\ArrayAccess|array $array\n+ * @param string|array $keys\n+ * @return bool\n+ */\n+ public static function hasAll($array, $keys)\n+ {\n+ $keys = (array) $keys;\n+\n+ if (! $array || $keys === []) {\n+ return false;\n+ }\n+\n+ $result = true;\n+\n+ foreach ($keys as $key) {\n+ if (! static::has($array, $key)) {\n+ $result = false;\n+ }\n+ }\n+\n+ return $result;",
"comment_created_at": "2025-05-22T01:42:28+00:00",
"comment_author": "devajmeireles",
"comment_body": "Thanks!",
"pr_file_module": null
}
]
},
{
"discussion_id": "2051354482",
"pr_number": 55472,
"pr_file": "src/Illuminate/Collections/Arr.php",
"created_at": "2025-04-19T02:42:19+00:00",
"commented_code": "return is_array($value) ? $value : [$value];\n }\n\n /**\n * Group an associative array by a key or callback.\n *\n * @param array $array\n * @param string|int|callable $groupBy\n * @param bool $preserveKeys\n * @return array\n */\n public static function groupBy($array, $groupBy, $preserveKeys = false)\n {\n $result = [];\n\n foreach ($array as $key => $item) {\n $groupKey = is_callable($groupBy) ? $groupBy($item, $key) : static::get($item, $groupBy);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2051354482",
"repo_full_name": "laravel/framework",
"pr_number": 55472,
"pr_file": "src/Illuminate/Collections/Arr.php",
"discussion_id": "2051354482",
"commented_code": "@@ -1021,4 +1021,29 @@ public static function wrap($value)\n \n return is_array($value) ? $value : [$value];\n }\n+\n+ /**\n+ * Group an associative array by a key or callback.\n+ *\n+ * @param array $array\n+ * @param string|int|callable $groupBy\n+ * @param bool $preserveKeys\n+ * @return array\n+ */\n+ public static function groupBy($array, $groupBy, $preserveKeys = false)\n+ {\n+ $result = [];\n+\n+ foreach ($array as $key => $item) {\n+ $groupKey = is_callable($groupBy) ? $groupBy($item, $key) : static::get($item, $groupBy);",
"comment_created_at": "2025-04-19T02:42:19+00:00",
"comment_author": "rodrigopedra",
"comment_body": "I would ensure `$groupBy` is a callback out the loop, so we don't need to check if it is a callback on every item.\r\n\r\nSomething like this:\r\n\r\n```php\r\n$groupBy = is_callable($groupBy) ? $groupBy : fn ($item) => static::get($item, $groupBy);\r\n\r\nforeach ($array as $key => $item) {\r\n $groupKey = $groupBy($item, $key);\r\n\r\n // ...\r\n}\r\n```",
"pr_file_module": null
},
{
"comment_id": "2051403566",
"repo_full_name": "laravel/framework",
"pr_number": 55472,
"pr_file": "src/Illuminate/Collections/Arr.php",
"discussion_id": "2051354482",
"commented_code": "@@ -1021,4 +1021,29 @@ public static function wrap($value)\n \n return is_array($value) ? $value : [$value];\n }\n+\n+ /**\n+ * Group an associative array by a key or callback.\n+ *\n+ * @param array $array\n+ * @param string|int|callable $groupBy\n+ * @param bool $preserveKeys\n+ * @return array\n+ */\n+ public static function groupBy($array, $groupBy, $preserveKeys = false)\n+ {\n+ $result = [];\n+\n+ foreach ($array as $key => $item) {\n+ $groupKey = is_callable($groupBy) ? $groupBy($item, $key) : static::get($item, $groupBy);",
"comment_created_at": "2025-04-19T05:54:40+00:00",
"comment_author": "vishal2931",
"comment_body": "Thanks for the feedback, but that approach looks more readable than this one. \ud83e\udd14",
"pr_file_module": null
}
]
},
{
"discussion_id": "2096571598",
"pr_number": 55784,
"pr_file": "src/Illuminate/Support/Number.php",
"created_at": "2025-05-19T22:38:42+00:00",
"commented_code": "throw new RuntimeException('The \"intl\" PHP extension is required to use the ['.$method.'] method.');\n }\n }\n\n /**\n * Convert an integer into its Roman numeral representation.\n *\n * @param int $number The number to convert (must be between 1 and 3999)\n * @return string The Roman numeral representation\n *\n * @throws InvalidArgumentException If the number is not between 1 and 3999\n */\n public static function roman(int $number): string\n {\n if ($number <= 0 || $number > 3999) {\n throw new InvalidArgumentException('Number must be between 1 and 3999.');\n }\n\n $map = [\n 'M' => 1000,\n 'CM' => 900,\n 'D' => 500,\n 'CD' => 400,\n 'C' => 100,\n 'XC' => 90,\n 'L' => 50,\n 'XL' => 40,\n 'X' => 10,\n 'IX' => 9,\n 'V' => 5,\n 'IV' => 4,\n 'I' => 1,\n ];\n\n $result = '';\n\n foreach ($map as $roman => $value) {\n while ($number >= $value) {\n $result .= $roman;\n $number -= $value;\n }\n }",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2096571598",
"repo_full_name": "laravel/framework",
"pr_number": 55784,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "2096571598",
"commented_code": "@@ -427,4 +428,46 @@ protected static function ensureIntlExtensionIsInstalled()\n throw new RuntimeException('The \"intl\" PHP extension is required to use the ['.$method.'] method.');\n }\n }\n+\n+ /**\n+ * Convert an integer into its Roman numeral representation.\n+ *\n+ * @param int $number The number to convert (must be between 1 and 3999)\n+ * @return string The Roman numeral representation\n+ *\n+ * @throws InvalidArgumentException If the number is not between 1 and 3999\n+ */\n+ public static function roman(int $number): string\n+ {\n+ if ($number <= 0 || $number > 3999) {\n+ throw new InvalidArgumentException('Number must be between 1 and 3999.');\n+ }\n+\n+ $map = [\n+ 'M' => 1000,\n+ 'CM' => 900,\n+ 'D' => 500,\n+ 'CD' => 400,\n+ 'C' => 100,\n+ 'XC' => 90,\n+ 'L' => 50,\n+ 'XL' => 40,\n+ 'X' => 10,\n+ 'IX' => 9,\n+ 'V' => 5,\n+ 'IV' => 4,\n+ 'I' => 1,\n+ ];\n+\n+ $result = '';\n+\n+ foreach ($map as $roman => $value) {\n+ while ($number >= $value) {\n+ $result .= $roman;\n+ $number -= $value;\n+ }\n+ }",
"comment_created_at": "2025-05-19T22:38:42+00:00",
"comment_author": "shaedrich",
"comment_body": "You could prevent trailing iterations when calculating the result is done, speeding the process slightly more up:\r\n```suggestion\r\n foreach ($map as $roman => $value) {\r\n while ($number >= $value) {\r\n $result .= $roman;\r\n $number -= $value;\r\n }\r\n \r\n if ($number === 0) {\r\n \treturn $result;\r\n }\r\n }\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1850814073",
"pr_number": 53596,
"pr_file": "src/Illuminate/Bus/Dispatcher.php",
"created_at": "2024-11-20T18:41:45+00:00",
"commented_code": "{\n $uses = class_uses_recursive($command);\n\n if (in_array(InteractsWithQueue::class, $uses) &&\n in_array(Queueable::class, $uses) &&\n ! $command->job) {\n if (isset($uses[InteractsWithQueue::class], $uses[Queueable::class]) && ! $command->job) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1850814073",
"repo_full_name": "laravel/framework",
"pr_number": 53596,
"pr_file": "src/Illuminate/Bus/Dispatcher.php",
"discussion_id": "1850814073",
"commented_code": "@@ -109,9 +109,7 @@ public function dispatchNow($command, $handler = null)\n {\n $uses = class_uses_recursive($command);\n \n- if (in_array(InteractsWithQueue::class, $uses) &&\n- in_array(Queueable::class, $uses) &&\n- ! $command->job) {\n+ if (isset($uses[InteractsWithQueue::class], $uses[Queueable::class]) && ! $command->job) {",
"comment_created_at": "2024-11-20T18:41:45+00:00",
"comment_author": "GrahamCampbell",
"comment_body": "This is an incorrect refactor. isset looks at the keys. in_array looks at the values.",
"pr_file_module": null
},
{
"comment_id": "1850820663",
"repo_full_name": "laravel/framework",
"pr_number": 53596,
"pr_file": "src/Illuminate/Bus/Dispatcher.php",
"discussion_id": "1850814073",
"commented_code": "@@ -109,9 +109,7 @@ public function dispatchNow($command, $handler = null)\n {\n $uses = class_uses_recursive($command);\n \n- if (in_array(InteractsWithQueue::class, $uses) &&\n- in_array(Queueable::class, $uses) &&\n- ! $command->job) {\n+ if (isset($uses[InteractsWithQueue::class], $uses[Queueable::class]) && ! $command->job) {",
"comment_created_at": "2024-11-20T18:47:36+00:00",
"comment_author": "ralphjsmit",
"comment_body": "Isn't `class_uses_recursive()` outputting the exact same key as value?\r\n```php\r\n[\r\n SomeConcern::class => SomeConcern::class,\r\n]\r\n```",
"pr_file_module": null
},
{
"comment_id": "1850845485",
"repo_full_name": "laravel/framework",
"pr_number": 53596,
"pr_file": "src/Illuminate/Bus/Dispatcher.php",
"discussion_id": "1850814073",
"commented_code": "@@ -109,9 +109,7 @@ public function dispatchNow($command, $handler = null)\n {\n $uses = class_uses_recursive($command);\n \n- if (in_array(InteractsWithQueue::class, $uses) &&\n- in_array(Queueable::class, $uses) &&\n- ! $command->job) {\n+ if (isset($uses[InteractsWithQueue::class], $uses[Queueable::class]) && ! $command->job) {",
"comment_created_at": "2024-11-20T19:08:07+00:00",
"comment_author": "dshafik",
"comment_body": "@GrahamCampbell as @ralphjsmit points out, the result of `class_uses_recursive()` is a key-value pair with the trait name for _both_, therefore you can use `isset()` to check for the usage of a trait (`O(1)`) rather than traverse the array till you find the value using `in_array()`. (`O(n)`)",
"pr_file_module": null
},
{
"comment_id": "1851152975",
"repo_full_name": "laravel/framework",
"pr_number": 53596,
"pr_file": "src/Illuminate/Bus/Dispatcher.php",
"discussion_id": "1850814073",
"commented_code": "@@ -109,9 +109,7 @@ public function dispatchNow($command, $handler = null)\n {\n $uses = class_uses_recursive($command);\n \n- if (in_array(InteractsWithQueue::class, $uses) &&\n- in_array(Queueable::class, $uses) &&\n- ! $command->job) {\n+ if (isset($uses[InteractsWithQueue::class], $uses[Queueable::class]) && ! $command->job) {",
"comment_created_at": "2024-11-21T00:24:15+00:00",
"comment_author": "crynobone",
"comment_body": "This should be fine and we use `isset()` as well for https://github.com/laravel/framework/blob/eb625fa6aa083dbae74b4f2cc7dc6dafcce5ff07/src/Illuminate/Foundation/Testing/Concerns/InteractsWithTestCaseLifecycle.php#L196-L220",
"pr_file_module": null
},
{
"comment_id": "1851217495",
"repo_full_name": "laravel/framework",
"pr_number": 53596,
"pr_file": "src/Illuminate/Bus/Dispatcher.php",
"discussion_id": "1850814073",
"commented_code": "@@ -109,9 +109,7 @@ public function dispatchNow($command, $handler = null)\n {\n $uses = class_uses_recursive($command);\n \n- if (in_array(InteractsWithQueue::class, $uses) &&\n- in_array(Queueable::class, $uses) &&\n- ! $command->job) {\n+ if (isset($uses[InteractsWithQueue::class], $uses[Queueable::class]) && ! $command->job) {",
"comment_created_at": "2024-11-21T02:09:03+00:00",
"comment_author": "dshafik",
"comment_body": "@crynobone the `array_flip()` in that example should probably be removed too\u2026 (unrelated to this PR)",
"pr_file_module": null
}
]
}
]

164
_reviewers/framework-optimize-migration-code.md Normal file
View File

@@ -0,0 +1,164 @@
---
title: Optimize migration code
description: 'When writing database migration code, prioritize clarity and efficiency
to ensure migrations are reliable and maintainable across environments. Apply these
practices:'
repository: laravel/framework
label: Migrations
language: PHP
comments_count: 4
repository_stars: 33763
---
When writing database migration code, prioritize clarity and efficiency to ensure migrations are reliable and maintainable across environments. Apply these practices:
1. **Use early returns for better flow control**
Instead of nesting conditions or using complex branching, return early when a condition is met:
```php
// Instead of this:
if ($this->shouldSkipMigration($migration)) {
$this->write(Task::class, $name, fn () => MigrationResult::Skipped);
} else {
// other operations
}
// Prefer this:
if ($this->shouldSkipMigration($migration)) {
$this->write(Task::class, $name, fn () => MigrationResult::Skipped);
return;
}
// other operations
```
2. **Prefer array emptiness checks over count operations**
For better readability and potentially better performance:
```php
// Instead of this:
if (count($options['selected']) > 0) {
// ...
}
// Prefer this:
if ($options['selected'] !== []) {
// ...
}
```
3. **Use method reference syntax when appropriate**
Replace arrow functions with direct method references when the function is simply passing through arguments:
```php
// Instead of this:
->keyBy(fn($file) => $this->getMigrationName($file))
// Prefer this:
->keyBy($this->getMigrationName(...))
```
These practices help create more readable and maintainable migration code, reducing the chance of errors during database schema changes.
[
{
"discussion_id": "2008500679",
"pr_number": 55011,
"pr_file": "src/Illuminate/Database/Migrations/Migrator.php",
"created_at": "2025-03-22T00:25:12+00:00",
"commented_code": "return $this->pretendToRun($migration, 'up');\n }\n\n $this->write(Task::class, $name, fn () => $this->runMigration($migration, 'up'));\n if ($this->shouldSkipMigration($migration)) {\n $this->write(Task::class, $name, fn () => MigrationResult::Skipped);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2008500679",
"repo_full_name": "laravel/framework",
"pr_number": 55011,
"pr_file": "src/Illuminate/Database/Migrations/Migrator.php",
"discussion_id": "2008500679",
"commented_code": "@@ -241,12 +254,16 @@ protected function runUp($file, $batch, $pretend)\n return $this->pretendToRun($migration, 'up');\n }\n \n- $this->write(Task::class, $name, fn () => $this->runMigration($migration, 'up'));\n+ if ($this->shouldSkipMigration($migration)) {\n+ $this->write(Task::class, $name, fn () => MigrationResult::Skipped);",
"comment_created_at": "2025-03-22T00:25:12+00:00",
"comment_author": "inmanturbo",
"comment_body": "Shouldn't this return here instead of the else?\r\n\r\nThis will log the migration even though it was never run? This means that the migration won't get run later either, after the \"feature\" is \"enabled\" or `Migration::shouldRun()` otherwise returns `true`;\r\n\r\n```php\r\n\r\n if ($this->shouldSkipMigration($migration)) {\r\n $this->write(Task::class, $name, fn () => MigrationResult::Skipped);\r\n return;\r\n }\r\n```",
"pr_file_module": null
},
{
"comment_id": "2008510104",
"repo_full_name": "laravel/framework",
"pr_number": 55011,
"pr_file": "src/Illuminate/Database/Migrations/Migrator.php",
"discussion_id": "2008500679",
"commented_code": "@@ -241,12 +254,16 @@ protected function runUp($file, $batch, $pretend)\n return $this->pretendToRun($migration, 'up');\n }\n \n- $this->write(Task::class, $name, fn () => $this->runMigration($migration, 'up'));\n+ if ($this->shouldSkipMigration($migration)) {\n+ $this->write(Task::class, $name, fn () => MigrationResult::Skipped);",
"comment_created_at": "2025-03-22T00:32:25+00:00",
"comment_author": "inmanturbo",
"comment_body": "Ahh I see, I was confused, you wrapped the `Migrator::log()` call inside the else block. In that case I would suggest just returning early.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1921142352",
"pr_number": 54250,
"pr_file": "src/Illuminate/Database/Migrations/Migrator.php",
"created_at": "2025-01-18T20:00:20+00:00",
"commented_code": "// We want to pull in the last batch of migrations that ran on the previous\n // migration operation. We'll then reverse those migrations and run each\n // of them \"down\" to reverse the last migration \"operation\" which ran.\n $migrations = $this->getMigrationsForRollback($options);\n $migrations = isset($options['selected']) && count($options['selected']) > 0 ? $options['selected'] : $this->getMigrationsForRollback($options);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1921142352",
"repo_full_name": "laravel/framework",
"pr_number": 54250,
"pr_file": "src/Illuminate/Database/Migrations/Migrator.php",
"discussion_id": "1921142352",
"commented_code": "@@ -230,7 +231,7 @@ public function rollback($paths = [], array $options = [])\n // We want to pull in the last batch of migrations that ran on the previous\n // migration operation. We'll then reverse those migrations and run each\n // of them \"down\" to reverse the last migration \"operation\" which ran.\n- $migrations = $this->getMigrationsForRollback($options);\n+ $migrations = isset($options['selected']) && count($options['selected']) > 0 ? $options['selected'] : $this->getMigrationsForRollback($options);",
"comment_created_at": "2025-01-18T20:00:20+00:00",
"comment_author": "shaedrich",
"comment_body": "Function call can be avoided like this:\r\n```suggestion\r\n $migrations = isset($options['selected']) && $options['selected'] !== [] ? $options['selected'] : $this->getMigrationsForRollback($options);\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1921142441",
"pr_number": 54250,
"pr_file": "src/Illuminate/Database/Migrations/Migrator.php",
"created_at": "2025-01-18T20:01:08+00:00",
"commented_code": "{\n $this->events?->dispatch($event);\n }\n\n protected function getMigrationBatches($migrations)\n {\n $options = $this->resolveOptions($migrations);\n $selected = $options['selected'] ?? [];\n\n if (count($selected) > 0) {",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1921142441",
"repo_full_name": "laravel/framework",
"pr_number": 54250,
"pr_file": "src/Illuminate/Database/Migrations/Migrator.php",
"discussion_id": "1921142441",
"commented_code": "@@ -754,4 +758,32 @@ public function fireMigrationEvent($event)\n {\n $this->events?->dispatch($event);\n }\n+\n+ protected function getMigrationBatches($migrations)\n+ {\n+ $options = $this->resolveOptions($migrations);\n+ $selected = $options['selected'] ?? [];\n+\n+ if (count($selected) > 0) {",
"comment_created_at": "2025-01-18T20:01:08+00:00",
"comment_author": "shaedrich",
"comment_body": "Same here:\r\n```suggestion\r\n if ($selected !== []) {\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1921142809",
"pr_number": 54250,
"pr_file": "src/Illuminate/Database/Migrations/Migrator.php",
"created_at": "2025-01-18T20:03:19+00:00",
"commented_code": "public function getMigrationFiles($paths)\n {\n return (new Collection($paths))\n ->flatMap(fn ($path) => str_ends_with($path, '.php') ? [$path] : $this->files->glob($path.'/*_*.php'))\n ->flatMap(fn($path) => str_ends_with($path, '.php') ? [$path] : $this->files->glob($path . '/*_*.php'))\n ->filter()\n ->values()\n ->keyBy(fn ($file) => $this->getMigrationName($file))\n ->sortBy(fn ($file, $key) => $key)\n ->keyBy(fn($file) => $this->getMigrationName($file))",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1921142809",
"repo_full_name": "laravel/framework",
"pr_number": 54250,
"pr_file": "src/Illuminate/Database/Migrations/Migrator.php",
"discussion_id": "1921142809",
"commented_code": "@@ -537,11 +541,11 @@ protected function getMigrationClass(string $migrationName): string\n public function getMigrationFiles($paths)\n {\n return (new Collection($paths))\n- ->flatMap(fn ($path) => str_ends_with($path, '.php') ? [$path] : $this->files->glob($path.'/*_*.php'))\n+ ->flatMap(fn($path) => str_ends_with($path, '.php') ? [$path] : $this->files->glob($path . '/*_*.php'))\n ->filter()\n ->values()\n- ->keyBy(fn ($file) => $this->getMigrationName($file))\n- ->sortBy(fn ($file, $key) => $key)\n+ ->keyBy(fn($file) => $this->getMigrationName($file))",
"comment_created_at": "2025-01-18T20:03:19+00:00",
"comment_author": "shaedrich",
"comment_body": "You might be able to replace it with this:\r\n```suggestion\r\n ->keyBy($this->getMigrationName(...))\r\n```",
"pr_file_module": null
}
]
}
]

80
_reviewers/framework-precise-testing-dependency-versioning.md Normal file
View File

@@ -0,0 +1,80 @@
---
title: Precise testing dependency versioning
description: When specifying testing library dependencies, always use explicit minimum
patch versions rather than development branches or broad version constraints. This
practice ensures consistent and reproducible test environments across development,
CI systems, and production builds.
repository: laravel/framework
label: Testing
language: Json
comments_count: 2
repository_stars: 33763
---
When specifying testing library dependencies, always use explicit minimum patch versions rather than development branches or broad version constraints. This practice ensures consistent and reproducible test environments across development, CI systems, and production builds.
Example of good practice:
```json
"dependencies": {
"phpunit/phpunit": "^10.5.35|^11.3.6|^12.0.1",
"orchestra/testbench-core": "^9.9.3"
}
```
Example of practices to avoid:
```json
"dependencies": {
"phpunit/phpunit": "^10.5|^11.0", // Too broad, may include versions with issues
"orchestra/testbench-core": "9.x-dev" // Development branch, not stable
}
```
This standard helps prevent subtle test failures due to dependency changes and makes test behavior more predictable across all environments. Using precise version constraints is particularly important for testing frameworks as unexpected behavior in these dependencies can lead to false positives or negatives in your test results.
[
{
"discussion_id": "1938439791",
"pr_number": 54316,
"pr_file": "composer.json",
"created_at": "2025-02-02T09:02:38+00:00",
"commented_code": "\"league/flysystem-read-only\": \"^3.25.1\",\n \"league/flysystem-sftp-v3\": \"^3.25.1\",\n \"mockery/mockery\": \"^1.6.10\",\n \"orchestra/testbench-core\": \"^9.6\",\n \"orchestra/testbench-core\": \"9.x-dev\",",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1938439791",
"repo_full_name": "laravel/framework",
"pr_number": 54316,
"pr_file": "composer.json",
"discussion_id": "1938439791",
"commented_code": "@@ -111,11 +111,11 @@\n \"league/flysystem-read-only\": \"^3.25.1\",\n \"league/flysystem-sftp-v3\": \"^3.25.1\",\n \"mockery/mockery\": \"^1.6.10\",\n- \"orchestra/testbench-core\": \"^9.6\",\n+ \"orchestra/testbench-core\": \"9.x-dev\",",
"comment_created_at": "2025-02-02T09:02:38+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n \"orchestra/testbench-core\": \"^9.9.3\",\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1946329442",
"pr_number": 54316,
"pr_file": "src/Illuminate/Testing/composer.json",
"created_at": "2025-02-07T10:41:38+00:00",
"commented_code": "\"illuminate/database\": \"Required to assert databases (^11.0).\",\n \"illuminate/http\": \"Required to assert responses (^11.0).\",\n \"mockery/mockery\": \"Required to use mocking (^1.6).\",\n \"phpunit/phpunit\": \"Required to use assertions and run tests (^10.5|^11.0).\"\n \"phpunit/phpunit\": \"Required to use assertions and run tests (^10.5.35|^11.3.6|^12.0).\"",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1946329442",
"repo_full_name": "laravel/framework",
"pr_number": 54316,
"pr_file": "src/Illuminate/Testing/composer.json",
"discussion_id": "1946329442",
"commented_code": "@@ -37,7 +37,7 @@\n \"illuminate/database\": \"Required to assert databases (^11.0).\",\n \"illuminate/http\": \"Required to assert responses (^11.0).\",\n \"mockery/mockery\": \"Required to use mocking (^1.6).\",\n- \"phpunit/phpunit\": \"Required to use assertions and run tests (^10.5|^11.0).\"\n+ \"phpunit/phpunit\": \"Required to use assertions and run tests (^10.5.35|^11.3.6|^12.0).\"",
"comment_created_at": "2025-02-07T10:41:38+00:00",
"comment_author": "crynobone",
"comment_body": "```suggestion\r\n \"phpunit/phpunit\": \"Required to use assertions and run tests (^10.5.35|^11.3.6|^12.0.1).\"\r\n```",
"pr_file_module": null
}
]
}
]

261
_reviewers/framework-precise-type-annotations.md Normal file
View File

@@ -0,0 +1,261 @@
---
title: Precise type annotations
description: 'Always use the most specific and accurate type information possible
in PHPDoc comments to improve static analysis, IDE autocompletion, and code clarity.
Pay special attention to array types using appropriate syntax:'
repository: laravel/framework
label: Documentation
language: PHP
comments_count: 8
repository_stars: 33763
---
Always use the most specific and accurate type information possible in PHPDoc comments to improve static analysis, IDE autocompletion, and code clarity. Pay special attention to array types using appropriate syntax:
- For associative arrays with string keys: `array<string, mixed>` instead of just `array`
- For arrays of strings: `string[]` rather than generic `array`
- For complex generics: maintain template type parameters like `Collection<int, TPivotModel>`
- For method parameters accepting string or array of strings: `string|string[]` instead of `string|array`
Include full namespaces in type references (e.g., `\Illuminate\Support\Collection` instead of just `Collection`).
When documenting specialized types, use appropriate annotations:
```php
/**
* Get JSON casting flags for the specified attribute.
*
* @param string $key
* @return int-mask-of<JSON_*>
*/
protected function getJsonCastingFlags($key)
```
Precise type annotations help both developers and tools understand your code better, reducing potential errors and improving maintainability.
[
{
"discussion_id": "2144424524",
"pr_number": 56028,
"pr_file": "src/Illuminate/Cache/MemoizedStore.php",
"created_at": "2025-06-13T07:53:41+00:00",
"commented_code": "/**\n * Store multiple items in the cache for a given number of seconds.\n *\n * @param array $values",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2144424524",
"repo_full_name": "laravel/framework",
"pr_number": 56028,
"pr_file": "src/Illuminate/Cache/MemoizedStore.php",
"discussion_id": "2144424524",
"commented_code": "@@ -108,6 +108,7 @@ public function put($key, $value, $seconds)\n /**\n * Store multiple items in the cache for a given number of seconds.\n *\n+ * @param array $values",
"comment_created_at": "2025-06-13T07:53:41+00:00",
"comment_author": "shaedrich",
"comment_body": "Since `prefix()` expects a `string`, so this can be reflected here:\r\n```suggestion\r\n * @param array<string, mixed> $values\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "2102803004",
"pr_number": 55821,
"pr_file": "src/Illuminate/Support/Str.php",
"created_at": "2025-05-22T15:08:00+00:00",
"commented_code": "static::$camelCache = [];\n static::$studlyCache = [];\n }\n\n /**\n * Split a string by a given separator.\n *\n * @param string $separator\n * @param string $string\n * @param int $limit\n * @return array",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2102803004",
"repo_full_name": "laravel/framework",
"pr_number": 55821,
"pr_file": "src/Illuminate/Support/Str.php",
"discussion_id": "2102803004",
"commented_code": "@@ -2083,4 +2083,17 @@ public static function flushCache()\n static::$camelCache = [];\n static::$studlyCache = [];\n }\n+\n+ /**\n+ * Split a string by a given separator.\n+ *\n+ * @param string $separator\n+ * @param string $string\n+ * @param int $limit\n+ * @return array",
"comment_created_at": "2025-05-22T15:08:00+00:00",
"comment_author": "shaedrich",
"comment_body": "The `array` will either consist of strings or be empty, so we can narrow this further as well:\r\n```suggestion\r\n * @return string[]\r\n```",
"pr_file_module": null
},
{
"comment_id": "2102806447",
"repo_full_name": "laravel/framework",
"pr_number": 55821,
"pr_file": "src/Illuminate/Support/Str.php",
"discussion_id": "2102803004",
"commented_code": "@@ -2083,4 +2083,17 @@ public static function flushCache()\n static::$camelCache = [];\n static::$studlyCache = [];\n }\n+\n+ /**\n+ * Split a string by a given separator.\n+ *\n+ * @param string $separator\n+ * @param string $string\n+ * @param int $limit\n+ * @return array",
"comment_created_at": "2025-05-22T15:09:35+00:00",
"comment_author": "shaedrich",
"comment_body": "Ah, damn, sorry, hadn't submitted it and didn't see that it was closed when I hit the 'submit' button an hour later \ud83d\ude05",
"pr_file_module": null
}
]
},
{
"discussion_id": "2085688431",
"pr_number": 55721,
"pr_file": "src/Illuminate/Database/Eloquent/Relations/MorphToMany.php",
"created_at": "2025-05-12T23:36:35+00:00",
"commented_code": "}\n\n /**\n * Get the pivot models that are currently attached.\n * Get the pivot models that are currently attached, filtered by related model keys.\n *\n * @return \\Illuminate\\Support\\Collection<int, TPivotModel>\n * @param mixed $ids\n * @return \\Illuminate\\Support\\Collection",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2085688431",
"repo_full_name": "laravel/framework",
"pr_number": 55721,
"pr_file": "src/Illuminate/Database/Eloquent/Relations/MorphToMany.php",
"discussion_id": "2085688431",
"commented_code": "@@ -121,13 +121,14 @@ public function getRelationExistenceQuery(Builder $query, Builder $parentQuery,\n }\n \n /**\n- * Get the pivot models that are currently attached.\n+ * Get the pivot models that are currently attached, filtered by related model keys.\n *\n- * @return \\Illuminate\\Support\\Collection<int, TPivotModel>\n+ * @param mixed $ids\n+ * @return \\Illuminate\\Support\\Collection",
"comment_created_at": "2025-05-12T23:36:35+00:00",
"comment_author": "shaedrich",
"comment_body": "Shouldn't this stay\r\n```suggestion\r\n * @return \\Illuminate\\Support\\Collection<int, TPivotModel>\r\n```\r\n? If there is trouble with static analysis, the [PHPDoc comment in `InteractsWithPivotTable`](https://github.com/laravel/framework/blob/12.x/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php#L496) should be adjusted accordingly",
"pr_file_module": null
}
]
},
{
"discussion_id": "2077010982",
"pr_number": 55663,
"pr_file": "src/Illuminate/Database/Query/Builder.php",
"created_at": "2025-05-07T07:45:32+00:00",
"commented_code": "/**\n * Set the columns to be selected.\n *\n * @param array|mixed $columns\n * @param array<mixed>|mixed $columns",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2077010982",
"repo_full_name": "laravel/framework",
"pr_number": 55663,
"pr_file": "src/Illuminate/Database/Query/Builder.php",
"discussion_id": "2077010982",
"commented_code": "@@ -278,7 +278,7 @@ public function __construct(\n /**\n * Set the columns to be selected.\n *\n- * @param array|mixed $columns\n+ * @param array<mixed>|mixed $columns",
"comment_created_at": "2025-05-07T07:45:32+00:00",
"comment_author": "shaedrich",
"comment_body": "This is not primarily to be read by humans but by tools and for them `mixed` and your proposed alternative make a **huge** difference. If you want to make it more bite-sized, you can always resort to `@phpstan-type` or the like",
"pr_file_module": null
}
]
},
{
"discussion_id": "1993214446",
"pr_number": 54992,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"created_at": "2025-03-13T10:25:43+00:00",
"commented_code": "return $value;\n }\n\n /**\n * Get JSON casting flags for the specified attribute.\n *\n * @param string $key\n * @return int",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1993214446",
"repo_full_name": "laravel/framework",
"pr_number": 54992,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php",
"discussion_id": "1993214446",
"commented_code": "@@ -1324,15 +1326,33 @@ protected function castAttributeAsJson($key, $value)\n return $value;\n }\n \n+ /**\n+ * Get JSON casting flags for the specified attribute.\n+ *\n+ * @param string $key\n+ * @return int",
"comment_created_at": "2025-03-13T10:25:43+00:00",
"comment_author": "shaedrich",
"comment_body": "Therefore, you can narrow the return here as well:\r\n```suggestion\r\n * @return int-mask-of<JSON_*>\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1842638038",
"pr_number": 53427,
"pr_file": "src/Illuminate/Console/Scheduling/Schedule.php",
"created_at": "2024-11-14T17:33:19+00:00",
"commented_code": "use Illuminate\\Support\\Traits\\Macroable;\nuse RuntimeException;\n\n/**\n * @mixin ScheduleAttributes",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1842638038",
"repo_full_name": "laravel/framework",
"pr_number": 53427,
"pr_file": "src/Illuminate/Console/Scheduling/Schedule.php",
"discussion_id": "1842638038",
"commented_code": "@@ -17,9 +18,14 @@\n use Illuminate\\Support\\Traits\\Macroable;\n use RuntimeException;\n \n+/**\n+ * @mixin ScheduleAttributes",
"comment_created_at": "2024-11-14T17:33:19+00:00",
"comment_author": "stevebauman",
"comment_body": "This should be the full namespace:\r\n\r\n```suggestion\r\n * @mixin \\Illuminate\\Console\\Scheduling\\ScheduleAttributes\r\n```",
"pr_file_module": null
}
]
},
{
"discussion_id": "1910208001",
"pr_number": 54148,
"pr_file": "src/Illuminate/Routing/Controllers/HasMiddleware.php",
"created_at": "2025-01-10T10:57:00+00:00",
"commented_code": "/**\n * Get the middleware that should be assigned to the controller.\n *\n * @return \\Illuminate\\Routing\\Controllers\\Middleware[]\n * @return array<int,\\Illuminate\\Routing\\Controllers\\Middleware|\\Closure|string>",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1910208001",
"repo_full_name": "laravel/framework",
"pr_number": 54148,
"pr_file": "src/Illuminate/Routing/Controllers/HasMiddleware.php",
"discussion_id": "1910208001",
"commented_code": "@@ -7,7 +7,7 @@ interface HasMiddleware\n /**\n * Get the middleware that should be assigned to the controller.\n *\n- * @return \\Illuminate\\Routing\\Controllers\\Middleware[]\n+ * @return array<int,\\Illuminate\\Routing\\Controllers\\Middleware|\\Closure|string>",
"comment_created_at": "2025-01-10T10:57:00+00:00",
"comment_author": "shaedrich",
"comment_body": "Wouldn't this have sufficed?\r\n```suggestion\r\n * @return (\\Illuminate\\Routing\\Controllers\\Middleware|\\Closure|string)[]\r\n```",
"pr_file_module": null
},
{
"comment_id": "1910211747",
"repo_full_name": "laravel/framework",
"pr_number": 54148,
"pr_file": "src/Illuminate/Routing/Controllers/HasMiddleware.php",
"discussion_id": "1910208001",
"commented_code": "@@ -7,7 +7,7 @@ interface HasMiddleware\n /**\n * Get the middleware that should be assigned to the controller.\n *\n- * @return \\Illuminate\\Routing\\Controllers\\Middleware[]\n+ * @return array<int,\\Illuminate\\Routing\\Controllers\\Middleware|\\Closure|string>",
"comment_created_at": "2025-01-10T11:00:12+00:00",
"comment_author": "shaedrich",
"comment_body": "Also, is `string` the closest we can get? Have you tried `callable`?\r\n```suggestion\r\n * @return array<int,\\Illuminate\\Routing\\Controllers\\Middleware|callable>\r\n```",
"pr_file_module": null
},
{
"comment_id": "1910225592",
"repo_full_name": "laravel/framework",
"pr_number": 54148,
"pr_file": "src/Illuminate/Routing/Controllers/HasMiddleware.php",
"discussion_id": "1910208001",
"commented_code": "@@ -7,7 +7,7 @@ interface HasMiddleware\n /**\n * Get the middleware that should be assigned to the controller.\n *\n- * @return \\Illuminate\\Routing\\Controllers\\Middleware[]\n+ * @return array<int,\\Illuminate\\Routing\\Controllers\\Middleware|\\Closure|string>",
"comment_created_at": "2025-01-10T11:11:42+00:00",
"comment_author": "willpower232",
"comment_body": "using `(...)[]` seems to work just as well but I can't say I have seen that syntax before and tend to defer to the error output from phpstan which currently uses `array<...>`\r\n\r\nit does seem I can remove `\\Closure|string` and replace with `callable` so that is a nice improvement\r\n\r\nI have also seen `list<...>` is valid now so technically `@return list<\\Illuminate\\Routing\\Controllers\\Middleware|callable>` is an option in this case\r\n\r\nI'll wait for github to support polls and or others to vote on the best combination and way forward :sweat_smile: ",
"pr_file_module": null
},
{
"comment_id": "1910258359",
"repo_full_name": "laravel/framework",
"pr_number": 54148,
"pr_file": "src/Illuminate/Routing/Controllers/HasMiddleware.php",
"discussion_id": "1910208001",
"commented_code": "@@ -7,7 +7,7 @@ interface HasMiddleware\n /**\n * Get the middleware that should be assigned to the controller.\n *\n- * @return \\Illuminate\\Routing\\Controllers\\Middleware[]\n+ * @return array<int,\\Illuminate\\Routing\\Controllers\\Middleware|\\Closure|string>",
"comment_created_at": "2025-01-10T11:39:49+00:00",
"comment_author": "shaedrich",
"comment_body": "That would actually be great for such cases \ud83d\ude02\ud83d\udc4d\ud83c\udffb ",
"pr_file_module": null
}
]
},
{
"discussion_id": "1898688064",
"pr_number": 54025,
"pr_file": "src/Illuminate/Database/Schema/Blueprint.php",
"created_at": "2024-12-27T19:42:45+00:00",
"commented_code": "return $this->indexCommand('index', $columns, $name, $algorithm);\n }\n\n /**\n * Specify a functional index for the table.\n *\n * This method allows you to create a functional index using MySQL expressions\n * like `LOWER`, `UPPER`, or other supported functions directly on columns.\n *\n * @param string|array $columns The column(s) to include in the index.",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1898688064",
"repo_full_name": "laravel/framework",
"pr_number": 54025,
"pr_file": "src/Illuminate/Database/Schema/Blueprint.php",
"discussion_id": "1898688064",
"commented_code": "@@ -678,6 +678,37 @@ public function index($columns, $name = null, $algorithm = null)\n return $this->indexCommand('index', $columns, $name, $algorithm);\n }\n \n+ /**\n+ * Specify a functional index for the table.\n+ *\n+ * This method allows you to create a functional index using MySQL expressions\n+ * like `LOWER`, `UPPER`, or other supported functions directly on columns.\n+ *\n+ * @param string|array $columns The column(s) to include in the index.",
"comment_created_at": "2024-12-27T19:42:45+00:00",
"comment_author": "shaedrich",
"comment_body": "This could be improved to read:\r\n```suggestion\r\n * @param string|string[] $columns The column(s) to include in the index.\r\n```",
"pr_file_module": null
}
]
}
]

301
_reviewers/framework-use-modern-phpunit-attributes.md Normal file
View File

File diff suppressed because one or more lines are too long

360
_reviewers/framework-use-semantic-exceptions.md Normal file
View File

@@ -0,0 +1,360 @@
---
title: Use semantic exceptions
description: Choose exception types that accurately reflect the nature of the error.
Use `LogicException` for developer errors like incorrect usage, `InvalidArgumentException`
for invalid inputs, `OutOfBoundsException` for range violations, and specialized
exceptions for domain-specific errors.
repository: laravel/framework
label: Error Handling
language: PHP
comments_count: 8
repository_stars: 33763
---
Choose exception types that accurately reflect the nature of the error. Use `LogicException` for developer errors like incorrect usage, `InvalidArgumentException` for invalid inputs, `OutOfBoundsException` for range violations, and specialized exceptions for domain-specific errors.
This improves error diagnostics, enables selective exception handling, and communicates intent more clearly than using generic exceptions or returning magic values.
```php
// Instead of this
throw new Exception("Invalid compression mode");
// Do this
if ($mode < 1 || $mode > 9) {
throw new OutOfBoundsException('Compression mode must be between 1 and 9.');
}
// Instead of this
throw (new ModelNotFoundException)->setModel(get_class($this), $value);
// Do this
throw (new InvalidIdFormatException)->setModel(get_class($this), $value);
// Instead of using assertions that may be disabled
assert(is_object($model), 'Resource collection guesser expects objects.');
// Do this
if (!is_object($model)) {
throw new InvalidArgumentException('Resource collection guesser expects objects.');
}
// For related exceptions, create hierarchies for better error handling
class ViteException extends Exception {}
class ViteManifestNotFoundException extends ViteException {}
```
When creating exception hierarchies, consider extending from framework exceptions like `HttpException` for web applications to ensure proper HTTP status codes are returned. This allows consumers to handle entire categories of errors with a single catch block while still providing appropriate responses.
Good exception selection is the foundation of effective error handling and leads to more maintainable, self-documenting code.
[
{
"discussion_id": "2080817660",
"pr_number": 55685,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"created_at": "2025-05-09T02:51:43+00:00",
"commented_code": "protected function bootIfNotBooted()\n {\n if (! isset(static::$booted[static::class])) {\n static::$booted[static::class] = true;\n if (isset(static::$booting[static::class])) {\n throw new Exception('\"'.__METHOD__.'\" cannot be called on the \"'.static::class.'\" class while it is already being booted.');",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2080817660",
"repo_full_name": "laravel/framework",
"pr_number": 55685,
"pr_file": "src/Illuminate/Database/Eloquent/Model.php",
"discussion_id": "2080817660",
"commented_code": "@@ -286,12 +292,20 @@ public function __construct(array $attributes = [])\n protected function bootIfNotBooted()\n {\n if (! isset(static::$booted[static::class])) {\n- static::$booted[static::class] = true;\n+ if (isset(static::$booting[static::class])) {\n+ throw new Exception('\"'.__METHOD__.'\" cannot be called on the \"'.static::class.'\" class while it is already being booted.');",
"comment_created_at": "2025-05-09T02:51:43+00:00",
"comment_author": "rodrigopedra",
"comment_body": "Shouldn't it be a `LogicException`?\r\n\r\n> Exception that represents error in the program logic. This kind of exception should lead directly to a fix in your code. \r\n\r\nhttps://www.php.net/manual/en/class.logicexception.php",
"pr_file_module": null
}
]
},
{
"discussion_id": "2009314457",
"pr_number": 55107,
"pr_file": "src/Illuminate/Database/Eloquent/Collection.php",
"created_at": "2025-03-24T00:28:42+00:00",
"commented_code": "return $model->newModelQuery()->whereKey($this->modelKeys());\n }\n\n /**\n * Create a new resource collection instance for the given resource.\n *\n * @param class-string<JsonResource>|null $resourceClass\n * @return ResourceCollection\n */\n public function toResourceCollection(?string $resourceClass = null): ResourceCollection\n {\n if ($resourceClass === null) {\n return $this->guessResourceCollection();\n }\n\n return $resourceClass::collection($this);\n }\n\n /**\n * Guess the resource collection for the items.\n *\n * @return ResourceCollection\n */\n protected function guessResourceCollection(): ResourceCollection\n {\n if ($this->isEmpty()) {\n return new ResourceCollection($this);\n }\n\n $model = $this->first();\n\n assert(is_object($model), 'Resource collection guesser expects the collection to contain objects.');",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "2009314457",
"repo_full_name": "laravel/framework",
"pr_number": 55107,
"pr_file": "src/Illuminate/Database/Eloquent/Collection.php",
"discussion_id": "2009314457",
"commented_code": "@@ -843,4 +845,44 @@ public function toQuery()\n \n return $model->newModelQuery()->whereKey($this->modelKeys());\n }\n+\n+ /**\n+ * Create a new resource collection instance for the given resource.\n+ *\n+ * @param class-string<JsonResource>|null $resourceClass\n+ * @return ResourceCollection\n+ */\n+ public function toResourceCollection(?string $resourceClass = null): ResourceCollection\n+ {\n+ if ($resourceClass === null) {\n+ return $this->guessResourceCollection();\n+ }\n+\n+ return $resourceClass::collection($this);\n+ }\n+\n+ /**\n+ * Guess the resource collection for the items.\n+ *\n+ * @return ResourceCollection\n+ */\n+ protected function guessResourceCollection(): ResourceCollection\n+ {\n+ if ($this->isEmpty()) {\n+ return new ResourceCollection($this);\n+ }\n+\n+ $model = $this->first();\n+\n+ assert(is_object($model), 'Resource collection guesser expects the collection to contain objects.');",
"comment_created_at": "2025-03-24T00:28:42+00:00",
"comment_author": "mohammadrasoulasghari",
"comment_body": "Using assertions in production code is risky as they may be disabled in production environments (zend.assertions=-1). When assertions fail in this state, no proper errors are thrown, leading to silent failures or unexpected behavior. Better to use explicit exceptions with meaningful error messages for reliable error handling.\r\n\r\nhttps://www.php.net/manual/en/ini.core.php#ini.zend.assertions\r\n\r\nsee : \\Illuminate\\Database\\Eloquent\\Collection::toQuery",
"pr_file_module": null
}
]
},
{
"discussion_id": "1973796962",
"pr_number": 54824,
"pr_file": "src/Illuminate/Validation/InvalidSoftDeleteQueryException.php",
"created_at": "2025-02-27T15:13:30+00:00",
"commented_code": "<?php\n\nnamespace Illuminate\\Validation;\n\nuse RuntimeException;\n\nclass InvalidSoftDeleteQueryException extends RuntimeException",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1973796962",
"repo_full_name": "laravel/framework",
"pr_number": 54824,
"pr_file": "src/Illuminate/Validation/InvalidSoftDeleteQueryException.php",
"discussion_id": "1973796962",
"commented_code": "@@ -0,0 +1,10 @@\n+<?php\n+\n+namespace Illuminate\\Validation;\n+\n+use RuntimeException;\n+\n+class InvalidSoftDeleteQueryException extends RuntimeException",
"comment_created_at": "2025-02-27T15:13:30+00:00",
"comment_author": "kapersoft",
"comment_body": "Since it's a logic issue introduces by the developer, extending [LogicException](https://www.php.net/manual/en/class.logicexception.php) suits this exception better.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1667219375",
"pr_number": 52039,
"pr_file": "src/Illuminate/Support/Str.php",
"created_at": "2024-07-06T00:52:34+00:00",
"commented_code": "return mb_convert_case($string, $mode, $encoding);\n }\n\n /**\n * Compresses a string using gzip compression.\n *\n * @param string $string The string to compress.\n * @param int $mode Compression level (1 to 9, defaults to 5).\n * @return string|false The compressed string, or false on failure.\n */\n public static function compress(string $string, int $mode = 5)\n {\n // Ensure compression mode is within valid range",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1667219375",
"repo_full_name": "laravel/framework",
"pr_number": 52039,
"pr_file": "src/Illuminate/Support/Str.php",
"discussion_id": "1667219375",
"commented_code": "@@ -341,6 +341,38 @@ public static function convertCase(string $string, int $mode = MB_CASE_FOLD, ?st\n return mb_convert_case($string, $mode, $encoding);\n }\n \n+ /**\n+ * Compresses a string using gzip compression.\n+ *\n+ * @param string $string The string to compress.\n+ * @param int $mode Compression level (1 to 9, defaults to 5).\n+ * @return string|false The compressed string, or false on failure.\n+ */\n+ public static function compress(string $string, int $mode = 5)\n+ {\n+ // Ensure compression mode is within valid range",
"comment_created_at": "2024-07-06T00:52:34+00:00",
"comment_author": "rodrigopedra",
"comment_body": "```php\r\n$mode = min(1, max($mode, 9));\r\n```",
"pr_file_module": null
},
{
"comment_id": "1667219605",
"repo_full_name": "laravel/framework",
"pr_number": 52039,
"pr_file": "src/Illuminate/Support/Str.php",
"discussion_id": "1667219375",
"commented_code": "@@ -341,6 +341,38 @@ public static function convertCase(string $string, int $mode = MB_CASE_FOLD, ?st\n return mb_convert_case($string, $mode, $encoding);\n }\n \n+ /**\n+ * Compresses a string using gzip compression.\n+ *\n+ * @param string $string The string to compress.\n+ * @param int $mode Compression level (1 to 9, defaults to 5).\n+ * @return string|false The compressed string, or false on failure.\n+ */\n+ public static function compress(string $string, int $mode = 5)\n+ {\n+ // Ensure compression mode is within valid range",
"comment_created_at": "2024-07-06T00:53:28+00:00",
"comment_author": "rodrigopedra",
"comment_body": "Although I'd prefer it to throw an `\\OutOfBoundsException`\r\n\r\nhttps://www.php.net/manual/en/class.outofboundsexception.php",
"pr_file_module": null
},
{
"comment_id": "1667348175",
"repo_full_name": "laravel/framework",
"pr_number": 52039,
"pr_file": "src/Illuminate/Support/Str.php",
"discussion_id": "1667219375",
"commented_code": "@@ -341,6 +341,38 @@ public static function convertCase(string $string, int $mode = MB_CASE_FOLD, ?st\n return mb_convert_case($string, $mode, $encoding);\n }\n \n+ /**\n+ * Compresses a string using gzip compression.\n+ *\n+ * @param string $string The string to compress.\n+ * @param int $mode Compression level (1 to 9, defaults to 5).\n+ * @return string|false The compressed string, or false on failure.\n+ */\n+ public static function compress(string $string, int $mode = 5)\n+ {\n+ // Ensure compression mode is within valid range",
"comment_created_at": "2024-07-06T10:52:37+00:00",
"comment_author": "rmunate",
"comment_body": "According to you, I adjusted the method.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1898695609",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"created_at": "2024-12-27T20:05:40+00:00",
"commented_code": "return static::$currency;\n }\n\n /**\n * Generate a random number of the given length.\n *\n * @param int $length\n * @return int\n */\n public static function random(int $length = 6): int\n {\n $maxLength = strlen((string) PHP_INT_MAX);\n\n if ($length < 1 || $length > $maxLength) {\n return 0;",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1898695609",
"repo_full_name": "laravel/framework",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "1898695609",
"commented_code": "@@ -367,6 +367,30 @@ public static function defaultCurrency()\n return static::$currency;\n }\n \n+ /**\n+ * Generate a random number of the given length.\n+ *\n+ * @param int $length\n+ * @return int\n+ */\n+ public static function random(int $length = 6): int\n+ {\n+ $maxLength = strlen((string) PHP_INT_MAX);\n+\n+ if ($length < 1 || $length > $maxLength) {\n+ return 0;",
"comment_created_at": "2024-12-27T20:05:40+00:00",
"comment_author": "shaedrich",
"comment_body": "Is `0` helpful here? If I can't generate a _random_ integer, I'd expect to get an exception thrown (maybe [`Random\\RandomException`](https://www.php.net/manual/en/class.random-randomexception.php) or [`Random\\RandomError`](https://www.php.net/manual/en/class.random-randomerror.php) \ud83e\udd14)",
"pr_file_module": null
},
{
"comment_id": "1898701582",
"repo_full_name": "laravel/framework",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "1898695609",
"commented_code": "@@ -367,6 +367,30 @@ public static function defaultCurrency()\n return static::$currency;\n }\n \n+ /**\n+ * Generate a random number of the given length.\n+ *\n+ * @param int $length\n+ * @return int\n+ */\n+ public static function random(int $length = 6): int\n+ {\n+ $maxLength = strlen((string) PHP_INT_MAX);\n+\n+ if ($length < 1 || $length > $maxLength) {\n+ return 0;",
"comment_created_at": "2024-12-27T20:24:48+00:00",
"comment_author": "shaedrich",
"comment_body": "Also, doesn't `random_int()` handle validation itself?",
"pr_file_module": null
},
{
"comment_id": "1900456930",
"repo_full_name": "laravel/framework",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "1898695609",
"commented_code": "@@ -367,6 +367,30 @@ public static function defaultCurrency()\n return static::$currency;\n }\n \n+ /**\n+ * Generate a random number of the given length.\n+ *\n+ * @param int $length\n+ * @return int\n+ */\n+ public static function random(int $length = 6): int\n+ {\n+ $maxLength = strlen((string) PHP_INT_MAX);\n+\n+ if ($length < 1 || $length > $maxLength) {\n+ return 0;",
"comment_created_at": "2025-01-01T20:30:17+00:00",
"comment_author": "dilovanmatini",
"comment_body": "I hesitated to get an exception or just return 0. Then I checked Str:random it uses `random_int()` without throwing any exception. An that is why I chose to return 0.",
"pr_file_module": null
},
{
"comment_id": "1900457686",
"repo_full_name": "laravel/framework",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "1898695609",
"commented_code": "@@ -367,6 +367,30 @@ public static function defaultCurrency()\n return static::$currency;\n }\n \n+ /**\n+ * Generate a random number of the given length.\n+ *\n+ * @param int $length\n+ * @return int\n+ */\n+ public static function random(int $length = 6): int\n+ {\n+ $maxLength = strlen((string) PHP_INT_MAX);\n+\n+ if ($length < 1 || $length > $maxLength) {\n+ return 0;",
"comment_created_at": "2025-01-01T20:36:04+00:00",
"comment_author": "shaedrich",
"comment_body": "If a function doesn't throw an exception, I'd at least expect it to return `null` or `false`, since `0` is a number but it's not random, so this can be quite confusing",
"pr_file_module": null
},
{
"comment_id": "1900458031",
"repo_full_name": "laravel/framework",
"pr_number": 54027,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "1898695609",
"commented_code": "@@ -367,6 +367,30 @@ public static function defaultCurrency()\n return static::$currency;\n }\n \n+ /**\n+ * Generate a random number of the given length.\n+ *\n+ * @param int $length\n+ * @return int\n+ */\n+ public static function random(int $length = 6): int\n+ {\n+ $maxLength = strlen((string) PHP_INT_MAX);\n+\n+ if ($length < 1 || $length > $maxLength) {\n+ return 0;",
"comment_created_at": "2025-01-01T20:39:21+00:00",
"comment_author": "dilovanmatini",
"comment_body": "That one is also possible \ud83d\udc4d",
"pr_file_module": null
}
]
},
{
"discussion_id": "1898945139",
"pr_number": 54033,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasUniqueStringIds.php",
"created_at": "2024-12-28T15:52:23+00:00",
"commented_code": "*/\n protected function handleInvalidUniqueId($value, $field)\n {\n throw (new ModelNotFoundException)->setModel(get_class($this), $value);\n throw (new ModelNotFoundException)->setModel(get_class($this), $value)->setStatus(422);",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1898945139",
"repo_full_name": "laravel/framework",
"pr_number": 54033,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasUniqueStringIds.php",
"discussion_id": "1898945139",
"commented_code": "@@ -103,6 +103,6 @@ public function getIncrementing()\n */\n protected function handleInvalidUniqueId($value, $field)\n {\n- throw (new ModelNotFoundException)->setModel(get_class($this), $value);\n+ throw (new ModelNotFoundException)->setModel(get_class($this), $value)->setStatus(422);",
"comment_created_at": "2024-12-28T15:52:23+00:00",
"comment_author": "shaedrich",
"comment_body": "Wouldn't it make more sense to throw a different exception (like [`\\Ramsey\\Uuid\\Exception\\InvalidUuidStringException`](https://github.com/ramsey/uuid/blob/4.x/src/Exception/InvalidUuidStringException.php)) as you suggested?\r\n\r\n> Create an entirely new exception for this case. I'd be happy to pivot to this if it makes more sense for the framework.\r\n\r\nI mean, the whole point is, that it's not about that the model isn't found.\r\n\r\n> Throw a ValidationException instead. Something like ValidationException::withMessages([$field => '???']). I'm not sure what the message should be in that case.\r\n\r\nI don't think, we should mix the validation functionality of the framework with the rest of the framework. A dedicated exception like in your first suggestion sounds better",
"pr_file_module": null
},
{
"comment_id": "1899151726",
"repo_full_name": "laravel/framework",
"pr_number": 54033,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasUniqueStringIds.php",
"discussion_id": "1898945139",
"commented_code": "@@ -103,6 +103,6 @@ public function getIncrementing()\n */\n protected function handleInvalidUniqueId($value, $field)\n {\n- throw (new ModelNotFoundException)->setModel(get_class($this), $value);\n+ throw (new ModelNotFoundException)->setModel(get_class($this), $value)->setStatus(422);",
"comment_created_at": "2024-12-29T15:32:55+00:00",
"comment_author": "cosmastech",
"comment_body": "Sharing ModelNotFoundException doesn't seem like the right approach, I agree. Went with a new exception, `InvalidIdFormatException`.\r\n\r\nThe problem with using `InvalidUuidStringException` is that any time a user requests an endpoint with an invalid UUID, it would be reported (to Sentry, DataDog, et cetera) which isn't desirable.\r\n\r\nThe user (or the framework) could add it to the exception handler's dontReport array, but there may be times where the user does want it reported in parts of the code outside of route model binding.\r\n\r\nIt also would mean that every time a user creates their own unique string ID trait, they would have to cook up their own exception. By creating a more generic exception, it saves users from having to follow the additional steps to avoid eating up their exception reporting limits.",
"pr_file_module": null
},
{
"comment_id": "1899153828",
"repo_full_name": "laravel/framework",
"pr_number": 54033,
"pr_file": "src/Illuminate/Database/Eloquent/Concerns/HasUniqueStringIds.php",
"discussion_id": "1898945139",
"commented_code": "@@ -103,6 +103,6 @@ public function getIncrementing()\n */\n protected function handleInvalidUniqueId($value, $field)\n {\n- throw (new ModelNotFoundException)->setModel(get_class($this), $value);\n+ throw (new ModelNotFoundException)->setModel(get_class($this), $value)->setStatus(422);",
"comment_created_at": "2024-12-29T15:45:57+00:00",
"comment_author": "shaedrich",
"comment_body": "> Went with a new exception, `InvalidIdFormatException`. The problem with using `InvalidUuidStringException` is that any time a user requests an endpoint with an invalid UUID, it would be reported (to Sentry, DataDog, et cetera) which isn't desirable.\r\n\r\nAgreed. A new exception sounds good \ud83d\udc4d\ud83c\udffb Also, this way, we are less dependent on `ramsey/uuid` (I'm remembering what chaos the introduction of the lazy classes into `ramsey/uuid` caused for some who used the package).\r\n\r\n> By creating a more generic exception, it saves users from having to follow the additional steps to avoid eating up their exception reporting limits.\r\n\r\nMy only problem now with this is that the naming is too generic for the implementation. For the implementation to match the level of genericness the name has, it would have to have some kind of property holding the format or a description of it.",
"pr_file_module": null
}
]
},
{
"discussion_id": "1818276038",
"pr_number": 53318,
"pr_file": "src/Illuminate/Support/Number.php",
"created_at": "2024-10-28T01:53:10+00:00",
"commented_code": "*/\n protected static $currency = 'USD';\n\n\n /**\n * @param string $locale\n * @return string\n * @throws \\Exception\n */\n public static function validateLocale(string $locale): string\n {\n $availableLocales = ResourceBundle::getLocales('');\n\n if ( ! in_array($locale, $availableLocales, true)){\n throw new \\Exception(\"Locale is invalid\");",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1818276038",
"repo_full_name": "laravel/framework",
"pr_number": 53318,
"pr_file": "src/Illuminate/Support/Number.php",
"discussion_id": "1818276038",
"commented_code": "@@ -24,6 +25,23 @@ class Number\n */\n protected static $currency = 'USD';\n \n+\n+ /**\n+ * @param string $locale\n+ * @return string\n+ * @throws \\Exception\n+ */\n+ public static function validateLocale(string $locale): string\n+ {\n+ $availableLocales = ResourceBundle::getLocales('');\n+\n+ if ( ! in_array($locale, $availableLocales, true)){\n+ throw new \\Exception(\"Locale is invalid\");",
"comment_created_at": "2024-10-28T01:53:10+00:00",
"comment_author": "crynobone",
"comment_body": "Should be `InvalidArgumentException`",
"pr_file_module": null
}
]
},
{
"discussion_id": "1681129407",
"pr_number": 52169,
"pr_file": "src/Illuminate/Foundation/ViteFileFromManifestNotFoundException.php",
"created_at": "2024-07-17T14:10:07+00:00",
"commented_code": "<?php\n\nnamespace Illuminate\\Foundation;\n\nuse Exception;\n\nclass ViteFileFromManifestNotFoundException extends Exception",
"repo_full_name": "laravel/framework",
"discussion_comments": [
{
"comment_id": "1681129407",
"repo_full_name": "laravel/framework",
"pr_number": 52169,
"pr_file": "src/Illuminate/Foundation/ViteFileFromManifestNotFoundException.php",
"discussion_id": "1681129407",
"commented_code": "@@ -0,0 +1,10 @@\n+<?php\n+\n+namespace Illuminate\\Foundation;\n+\n+use Exception;\n+\n+class ViteFileFromManifestNotFoundException extends Exception",
"comment_created_at": "2024-07-17T14:10:07+00:00",
"comment_author": "innocenzi",
"comment_body": "Would be better to extend from some common, Vite-related base exception (eg. `\\Illuminate\\Foundation\\ViteException`). \r\n\r\nThis way, all Vite-related exceptions can be caught/matched by a single class.\r\n\r\n```php\r\ntry {\r\n // ...\r\n} catch (ViteException $ex) {\r\n // All Vite-related exceptions\r\n}\r\n```",
"pr_file_module": null
},
{
"comment_id": "1681135043",
"repo_full_name": "laravel/framework",
"pr_number": 52169,
"pr_file": "src/Illuminate/Foundation/ViteFileFromManifestNotFoundException.php",
"discussion_id": "1681129407",
"commented_code": "@@ -0,0 +1,10 @@\n+<?php\n+\n+namespace Illuminate\\Foundation;\n+\n+use Exception;\n+\n+class ViteFileFromManifestNotFoundException extends Exception",
"comment_created_at": "2024-07-17T14:13:28+00:00",
"comment_author": "SamuelWei",
"comment_body": "I would be fine with that as well.\r\n\r\nShould ViteException just be the new parent, or should there only exists a single ViteException for all three cases?\r\n\r\nShould I change this PR or open an other one instead?\r\n\r\nI could make ViteManifestNotFoundException extend this new ViteException, so that ViteManifestNotFoundException would still work for backward compatibility and at the same time it could be deprecated. What do you think?",
"pr_file_module": null
},
{
"comment_id": "1681895034",
"repo_full_name": "laravel/framework",
"pr_number": 52169,
"pr_file": "src/Illuminate/Foundation/ViteFileFromManifestNotFoundException.php",
"discussion_id": "1681129407",
"commented_code": "@@ -0,0 +1,10 @@\n+<?php\n+\n+namespace Illuminate\\Foundation;\n+\n+use Exception;\n+\n+class ViteFileFromManifestNotFoundException extends Exception",
"comment_created_at": "2024-07-17T23:39:12+00:00",
"comment_author": "timacdonald",
"comment_body": "I'd prefer to see just the single `Illuminate\\Foundation\\ViteException` class for all of these.\r\n\r\nWe don't have a usecase for catching them on an individual basis.",
"pr_file_module": null
},
{
"comment_id": "1682374900",
"repo_full_name": "laravel/framework",
"pr_number": 52169,
"pr_file": "src/Illuminate/Foundation/ViteFileFromManifestNotFoundException.php",
"discussion_id": "1681129407",
"commented_code": "@@ -0,0 +1,10 @@\n+<?php\n+\n+namespace Illuminate\\Foundation;\n+\n+use Exception;\n+\n+class ViteFileFromManifestNotFoundException extends Exception",
"comment_created_at": "2024-07-18T07:51:27+00:00",
"comment_author": "SamuelWei",
"comment_body": "I changed it to the single `Illuminate\\Foundation\\ViteException` but keeping `ViteManifestNotFoundException` for backwards compatibility reasons. Searching in Github i found a few projects that depend on this. \r\n\r\nShould we keep the return type of the manifest method or also change it to the new ViteException?",
"pr_file_module": null
}
]
}
]