Skip to content

[8.x] Remove invalid Blade compiler test #37112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

jrmajor
Copy link
Contributor

@jrmajor jrmajor commented Apr 24, 2021

I'm not sure what is ViewBladeCompilerTest::testDontIncludeNullPath() meant to test, but within this test null is passed to BladeCompiler::setPath(), which docblock says it only accepts strings. It's a setter for BladeCompiler::$path which, according to docblock, also isn't nullable.

$compiler->setPath(null);

Unless I'm missing something, I'll send a PR to 9.x to remove this test and revert changes in BladeCompiler. If you decide this is the desired behavior, I'll update this PR to change types in docblocks to ?string.

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 24, 2021

I've updated this PR to just remove the invalid test (no point changing null to an empty string in this one, there's another test for that).

@jrmajor jrmajor requested a review from GrahamCampbell April 24, 2021 19:06
$files->shouldReceive('get')->once()->with(null)->andReturn('Hello World');
$files->shouldReceive('exists')->once()->with(__DIR__)->andReturn(true);
$files->shouldReceive('put')->once()->with(__DIR__.'/'.sha1(null).'.php', 'Hello World');
$compiler->setPath(null);
Copy link
Member

Choose a reason for hiding this comment

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

Please find all places in the framework that might call this will null, and fix them.

Copy link
Member

Choose a reason for hiding this comment

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

There is at least one, in the service provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is at least one, in the service provider.

Sorry, didn't see this comment. Where exactly it is? Even using normal search, without any PhpStorm magic, I can't find it.

@jrmajor jrmajor requested a review from GrahamCampbell April 24, 2021 19:15
@jrmajor jrmajor changed the title [8.x] Fix for PHP 8.1 in Illuminate\View [8.x] Remove invalid Blade compiler test Apr 24, 2021
@taylorotwell
Copy link
Member

I don't see any reason to remove the test at the moment?

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 26, 2021

@taylorotwell This test passes null to BladeCompiler::setPath(), which accepts only a string, causing this test to fail on PHP 8.1. In the first version of this PR I've just added ?? '' in this method to make sure that BladeCompiler::$path is always a string, but @GrahamCampbell told me, that “any code that calls this with null is wrong and needs to be fixed”. Fixing this test would mean changing null to '', but there's another test for that, so I've removed this one.

@taylorotwell taylorotwell reopened this Apr 26, 2021
@taylorotwell taylorotwell merged commit 7ad5e59 into laravel:8.x Apr 26, 2021
@jrmajor jrmajor deleted the php81/null-param branch April 26, 2021 13:01
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.

None yet

4 participants