Skip to content
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

[9.x] Fix PHP warnings when rendering long blade string #41956

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

a-bashtannik
Copy link
Contributor

\Illuminate\View\Component::resolveView checks the string for being a file

$factory->exists($view) ? $view : $this->createBladeViewFromString($factory, $view);

by calling \Illuminate\Filesystem\Filesystem::exists method, which is a pure standard file_exists function.

PHP has a hard limit on path length as defined by the constant PHP_MAXPATHLEN which depends on the operating system (Windows 260 characters, Unix: as specified in the c-runtime - on Linux this is PATH_MAX in limits.h being 4096).

PHP detects the program is trying to check plain file existence and calls php_check_open_basedir

  1. https://github.com/php/php-src/blob/master/ext/standard/filestat.c#L757

If the PHP configuration has no open_basedir value, then another method virtual_file_ex will be called:

  1. https://github.com/php/php-src/blob/master/main/fopen_wrappers.c#L282
  2. https://github.com/php/php-src/blob/master/ext/standard/filestat.c#L767
  3. https://github.com/php/php-src/blob/master/Zend/zend_virtual_cwd.c#L1008

But virtual_file_ex doesn't throw warnings, just returns false.

If open_basedir is configured, php_check_open_basedir_ex is called, it cares about open_basedir and throws a warning, otherwise file_exists just returns false. The program behavior depends of open_basedir setting.

Although it's OK in the PHP design paradigm, users meet problems from time to time having this warning, see #32254 #41745 livewire/livewire#918

This PR adds PHP_MAXPATHLEN check before calling file_exists.

Although I can add a test case for different open_basedir states, it will affect other tests running in parallel, and considering the issue is rare, I don't think there is a sense to make a custom php.ini environment for the case.

@a-bashtannik a-bashtannik changed the title [9.x] Fix PHP warnings when rendering long blade string (#41745) [9.x] Fix PHP warnings when rendering long blade string Apr 13, 2022
@taylorotwell
Copy link
Member

Thanks for the PR, but can you succinctly describe the before / after behavior of this PR?

@a-bashtannik
Copy link
Contributor Author

a-bashtannik commented Apr 13, 2022

@taylorotwell Sure

Blade::render(string $string, array $data = [], bool $deleteCachedView = false)

Before:

$string argument may be a raw blade template string (e.g. from the database) or a file path. If the length > PHP_MAXPATHLEN Blade::render fails because file_exists function throws a warning and createBladeViewFromString method is never called even if $string argument is a valid, but long blade template string.

After

$string argument may have length > PHP_MAXPATHLEN, and considering it's impossible to have a file on the path with length > PHP_MAXPATHLEN, file_exists call is omitted, createBladeViewFromString is called and Blade::render returns compiled blade template as expected.

@taylorotwell taylorotwell merged commit bb0da4a into laravel:9.x Apr 13, 2022
@taylorotwell
Copy link
Member

Thanks!

BrandonSurowiec pushed a commit to BrandonSurowiec/framework that referenced this pull request Apr 14, 2022
* [9.x] Fix PHP warnings when rendering long blade string (laravel#41745)

* Apply fixes from StyleCI

(cherry picked from commit bb0da4a)
taylorotwell pushed a commit that referenced this pull request Apr 14, 2022
)

* [9.x] Fix PHP warnings when rendering long blade string (#41745)

* Apply fixes from StyleCI

(cherry picked from commit bb0da4a)

Co-authored-by: Andrew Bashtannik <[email protected]>
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.

2 participants