[7.x] Throw a TypeError if concrete is not a string or closure by mr-feek · Pull Request #33539 · laravel/framework

mr-feek · 2020-07-14T16:07:29Z

There is currently an assumption that if the concrete parameter passed to bind is not a Closure, then it is a string. Let's fail loudly if that is not the case.

Problem surfaced here: facade/ignition#291

…osure

GrahamCampbell

phpdoc says null is allowed, but your PR doesn't allow it

mr-feek · 2020-07-14T17:06:09Z

phpdoc says null is allowed, but your PR doesn't allow it

I'm not sure how $concrete could be null here, considering it is set to $abstract if it is null in the few lines above.

mr-feek · 2020-07-14T17:14:10Z

Would you like me to squash or do you squash when merging?

GrahamCampbell · 2020-07-14T17:23:02Z

Don't worry about that. Taylor can squash (or not) if he wants. :)

mr-feek · 2020-07-14T17:24:58Z

Thanks for the feedback @GrahamCampbell , it's appreciated 🤝

GrahamCampbell · 2020-07-14T17:40:01Z

        // up inside its own Closure to give us more convenience when extending.
        if (! $concrete instanceof Closure) {
+            if (! is_string($concrete)) {
+                throw new \TypeError(self::class.'::bind(): Argument #2 ($concrete) must be of type Closure|string|null');


Note for Taylor: This error message format is what PHP 8.0 does when there's a genuine union type.

taylorotwell · 2020-07-15T12:34:44Z

We may just eventually use a real union type here.

GrahamCampbell · 2020-07-15T13:13:21Z

@taylorotwell the reason for this PR was to stop real bugs, not just to add a type for the sake of it.

mr-feek · 2020-07-19T21:17:00Z

I have to agree with @GrahamCampbell here -- this issue took me several hours to realize. Even if we decide to use a real union type here in the future, I strongly believe it would be beneficial to add this validation now.

ssmusoke · 2020-07-28T14:25:01Z

Not sure what I need to do, but it seems that this PR has created all sorts of bind errors not sure how to resolve them for example when I run php artisan migrate

` TypeError

Illuminate\Container\Container::bind(): Argument #2 ($concrete) must be of type Closure|string|null

at vendor/laravel/framework/src/Illuminate/Container/Container.php:238
234| // bound into this container to the abstract type and we will just wrap it
235| // up inside its own Closure to give us more convenience when extending.
236| if (! $concrete instanceof Closure) {
237| if (! is_string($concrete)) {

238| throw new \TypeError(self::class.'::bind(): Argument #2 ($concrete) must be of type Closure|string|null');
239| }
240|
241| $concrete = $this->getClosure($abstract, $concrete);
242| }

  +10 vendor frames

11 artisan:37
Illuminate\Foundation\Console\Kernel::handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
`

GrahamCampbell · 2020-07-28T14:25:59Z

This is because there's a mistake in your code, which is now being caught and showed to you. That was the exact purpose of this. PR. :)

GrahamCampbell · 2020-07-28T14:26:30Z

Can you show the whole stack trace?

ssmusoke · 2020-07-28T14:33:06Z

That is all the stack trace that I get, I do not know which migration has the issue since it is not highlighted

GrahamCampbell · 2020-07-28T14:42:48Z

If you run the command in verbose mode, more should show. -vv

ssmusoke · 2020-07-28T14:48:36Z

I get the same output

My environment is

PHP 7.3.20 (cli) (built: Jul 9 2020 23:50:54) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.20, Copyright (c) 1998-2018 Zend Technologies
with Zend OPcache v7.3.20, Copyright (c) 1999-2018, by Zend Technologies

GrahamCampbell · 2020-07-28T14:52:09Z

php artisan migrate -vv

dkulyk · 2020-07-28T14:54:51Z

You should use $app->instance(Service::class, $serviceInstance) instead $app->bind(Service::class, $serviceInstance)

ssmusoke · 2020-07-28T14:55:39Z

Tried that and php artisan --vv migrate

Interesting I have these two dependencies

"laravel/framework": "^7.0",
"laravel/nova": "^3.0"

php artisan -vv nova:publish gives the same error, so I am suspecting that the issue may be within artisan command

GrahamCampbell · 2020-07-28T14:56:59Z

php artisan -vv nova:publish gives the same error, so I am suspecting that the issue may be within artisan command

I don't think so. The issue will be in one of your service providers, or possibly one provided in the core. Please show us the stack trace, or this will never be fixed (we can only fix it if we know where it is).

ssmusoke · 2020-07-28T15:31:07Z

Maybe my composer.json may help

{
"name": "laravel/laravel",
"type": "project",
"description": "The Laravel Framework.",
"keywords": [ "framework", "laravel" ],
"license": "MIT",
"require": {
"php": "^7.3",
"doctrine/dbal": "^2.10",
"facade/ignition": "^2.0",
"fideloper/proxy": "^4.0",
"intervention/image": "~2.4",
"kalnoy/nestedset": "^5.0",
"laravel/framework": "^7.0",
"laravel/nova": "^3.0",
"laravel/telescope": "^3.5",
"laravel/tinker": "^2.0",
"league/flysystem-aws-s3-v3": "^1.0",
"maatwebsite/excel": "^3.1",
"maatwebsite/laravel-nova-excel": "^1.2",
"orlyapps/nova-belongsto-depend": "^2.0",
"pragmarx/health": "^0.10.2",
"reportico/laravel-reportico": "^7.0",
"rap2hpoutre/laravel-log-viewer": "^1.6",
"spatie/laravel-backup": "^6.8",
"spatie/laravel-medialibrary": "^7.0.0",
"spatie/laravel-permission": "^3.10",
"spatie/nova-backup-tool": "^4.0",
"themsaid/wink": "^1.0",
"titasgailius/search-relations": "^1.0",
"twilio/sdk": "^5.39",
"tymon/jwt-auth": "dev-develop",
"vyuldashev/nova-permission": "^2.5"
},
"require-dev": {
"arrilot/laravel-data-anonymization": "^1.1",
"barryvdh/laravel-ide-helper": "^2.6",
"beyondcode/laravel-dump-server": "^1.0",
"filp/whoops": "^2.0",
"fzaninotto/faker": "^1.4",
"laravel/ui": "^2.0",
"mockery/mockery": "^1.0",
"nunomaduro/collision": "^4.1",
"phpunit/phpunit": "^8.5",
"themsaid/laravel-mail-preview": "^3.0"
},
"config": {
"optimize-autoloader": true,
"preferred-install": "dist",
"sort-packages": true,
"post-update-cmd": [ "@php artisan nova:publish" ]
},
"extra": {
"laravel": {
"dont-discover": []
}
},
"autoload": {
"psr-4": {
"App\": "app/"
},
"classmap": [ "database/seeds", "database/factories", "database/anonymization" ]
},
"autoload-dev": {
"psr-4": {
"Tests\": "tests/"
}
},
"minimum-stability": "dev",
"prefer-stable": true,
"scripts": {
"post-autoload-dump": [
"Illuminate\Foundation\ComposerScripts::postAutoloadDump",
"@php artisan package:discover --ansi"
],
"post-root-package-install": [ "@php -r "file_exists('.env') || copy('.env.example', '.env');"" ],
"post-create-project-cmd": [ "@php artisan key:generate --ansi" ],
"post-update-cmd": [ "@php artisan nova:publish", "@reportico-configure-auth" ],
"reportico-configure-auth": ["php -r "copy('work/ReporticoServiceProvider.php', 'vendor/reportico/laravel-reportico/src/Reportico/Reportico/ReporticoServiceProvider.php');""]
},
"repositories": [
{
"type": "composer",
"url": "https://nova.laravel.com"
}
]
}

mr-feek · 2020-07-28T15:39:10Z

What's your actual version of facade / ignition? If you look at my original PR description, you'll see this was actually as a result of a bug in facade/ignition which I fixed in the latest release. Please try updating it.

Otherwise we are just speculating without seeing a full stack trace.

ssmusoke · 2020-07-28T15:54:24Z

Thank you very much for your patience, I was only looking at the console for the error message, forgetting the logs

I found that the pragmarx/health dependency was the one causing the closure issue. I will go ahead and open an issue there

AustinW · 2020-07-28T16:38:18Z

Thank you very much for your patience, I was only looking at the console for the error message, forgetting the logs

I found that the pragmarx/health dependency was the one causing the closure issue. I will go ahead and open an issue there

I was having the same issue and also have that package installed. Thank you!

throw an invalid argument exception if concrete is not a string or cl…

c927197

…osure

GrahamCampbell requested changes Jul 14, 2020

View reviewed changes

mr-feek added 2 commits July 14, 2020 10:12

throw type error instead of invalid argument exception

80c2cfa

add throws back in

b82101f

mr-feek changed the title ~~[7.x] Throw an invalid argument exception if concrete is not a string or closure~~ [7.x] Throw a TypeError if concrete is not a string or closure Jul 14, 2020

GrahamCampbell approved these changes Jul 14, 2020

View reviewed changes

GrahamCampbell reviewed Jul 14, 2020

View reviewed changes

taylorotwell closed this Jul 15, 2020

taylorotwell reopened this Jul 21, 2020

taylorotwell merged commit a66caa5 into laravel:7.x Jul 21, 2020

ssmusoke mentioned this pull request Jul 28, 2020

Type Error issue with latest Laravel 7.x antonioribeiro/health#183

Closed

Uh oh!

Conversation

mr-feek commented Jul 14, 2020

Uh oh!

GrahamCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mr-feek commented Jul 14, 2020

Uh oh!

mr-feek commented Jul 14, 2020

Uh oh!

GrahamCampbell commented Jul 14, 2020

Uh oh!

mr-feek commented Jul 14, 2020

Uh oh!

GrahamCampbell Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

taylorotwell commented Jul 15, 2020

Uh oh!

GrahamCampbell commented Jul 15, 2020

Uh oh!

mr-feek commented Jul 19, 2020

Uh oh!

ssmusoke commented Jul 28, 2020

Uh oh!

GrahamCampbell commented Jul 28, 2020

Uh oh!

GrahamCampbell commented Jul 28, 2020

Uh oh!

ssmusoke commented Jul 28, 2020

Uh oh!

GrahamCampbell commented Jul 28, 2020

Uh oh!

ssmusoke commented Jul 28, 2020

Uh oh!

GrahamCampbell commented Jul 28, 2020

Uh oh!

dkulyk commented Jul 28, 2020

Uh oh!

ssmusoke commented Jul 28, 2020

Uh oh!

GrahamCampbell commented Jul 28, 2020

Uh oh!

ssmusoke commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mr-feek commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ssmusoke commented Jul 28, 2020

Uh oh!

AustinW commented Jul 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ssmusoke commented Jul 28, 2020 •

edited

Loading

mr-feek commented Jul 28, 2020 •

edited

Loading