Skip to content

Conversation

@browner12
Copy link
Contributor

this is a breaking change, as previously users could pass any type of value to $default. with this change it forces all $defaults to be string|array|null.

this is a reversion of part of #56684

for the following code:

dump(new \Illuminate\Support\Collection(['foo', 'bar']));
dd(old('tags', new \Illuminate\Support\Collection(['foo', 'bar'])))

prior to change:

Screenshot 2025-08-26 at 10 22 37 AM

after changes:

Screenshot 2025-08-26 at 10 22 23 AM

this is a breaking change, as previously users could pass any type of value to `$default`. with this change it forces all `$default`s to be `string|array|null`.
@taylorotwell taylorotwell merged commit 9a4c76c into laravel:12.x Aug 26, 2025
62 checks passed
@calebdw
Copy link
Contributor

calebdw commented Aug 26, 2025

It would have been nice to been tagged on this.

The phpdoc is lying, if the method truly returns mixed then the return type should be updated to mixed

@calebdw
Copy link
Contributor

calebdw commented Aug 26, 2025

Additionally, passing a collection violates the allowable parameter types defined for $default

* @param  \Illuminate\Database\Eloquent\Model|string|array|null  $default

@browner12
Copy link
Contributor Author

I tagged your PR, figured that would alert you.

I'd be okay going to a mixed return type. Just needed to get something in quick because this was a breaking change on a minor release.

Yah, it violates the docblock, but unfortunately docblocks don't mean jack squat. I think this will be the next major shift Laravel could/should go through and tighten its types. Would be a major hurdle, but think it would pay off in the long run.

@browner12 browner12 deleted the AB-revert-old branch August 26, 2025 16:32
@calebdw
Copy link
Contributor

calebdw commented Aug 26, 2025

I tagged your PR, figured that would alert you.

Unfortunately it doesn't look like github sends any notification that way, has to be a directy CC, but thanks for trying 👍

I'd be okay going to a mixed return type. Just needed to get something in quick because this was a breaking change on a minor release.

We can probably add a @template TDefault to the function and the return so that we can more accurately determine the return type based on the provided default:

    /**
     * Retrieve an old input item.
     *
     * @template TDefault
     * @param  string|null  $key
     * @param  TDefault  $default
     * @return string|array|null|TDefault
     */
    function old($key = null, $default = null): mixed

Yah, it violates the docblock, but unfortunately docblocks don't mean jack squat. I think this will be the next major shift Laravel could/should go through and tighten its types. Would be a major hurdle, but think it would pay off in the long run.

100% agree, this is one of my biggest frustrations is that functions/methods don't return or accept what they say they should. Would be willing to help out with this for v13 if that's something that Taylor is willing to do. Rector/shift can be used to automatically add native types for users

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants