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

function call is_uploaded_file is not valid #133

Closed
see0 opened this issue Mar 4, 2019 · 25 comments
Closed

function call is_uploaded_file is not valid #133

see0 opened this issue Mar 4, 2019 · 25 comments

Comments

@see0
Copy link

see0 commented Mar 4, 2019

Symphony and Laravel UploadedFile class consider all uploded as invalid due to the is_uploaded_file check.

vendor/symfony/http-foundation/File/UploadedFile.php

public function isValid()
{
    $isOk = UPLOAD_ERR_OK === $this->error;

    return $this->test ? $isOk : $isOk && is_uploaded_file($this->getPathname());
}

Swoole solution: swoole/swoole-src#407

My current workaround - I subclassed both HttpFoundationFactory, UploadedFile to get around this issue.

Let me know if this issue is going to be solved here (upstream) or should I send a pull request for my workaround to https://github.com/UPDG/roadrunner-laravel.

Thanks

@wolfy-j
Copy link
Contributor

wolfy-j commented Mar 4, 2019

@Alex-Bond i guess this issue is for you.

@Alex-Bond
Copy link
Contributor

@see0 hey! Can you create PR with some use cases? At this moment I don't really understand where the problem is.

@see0
Copy link
Author

see0 commented Mar 4, 2019

@see0 hey! Can you create PR with some use cases? At this moment I don't really understand where the problem is.

Hmm, i'm surprised that I am the only one facing this issue.

The problem is caused by all (JSON) Data are relayed thru goridge, and then restored to PSR-7 Message in the worker.

Hence, PHP is unable to add all the uploaded_files into their rfc1867_uploaded_files hash table - any function call to is_uploaded_file will return a FALSE result.

When you try to perform any File Validation in Laravel/Symphony Request - the result will always be invalid.

Well, I supposed is_uploaded_file is a php core function used in "olden/PHP 4" days to validate the file is uploaded. It's really an edge case, since solving it will only makes it easier to adopt other frameworks.

My take is:

  1. Don't solve it but document the caveat in the wiki and let individual integration handles them.
  2. (Optionally - not exactly useful) Establish a signing token between the RoadRunner Server and client - the relayed data is trustable.

I'm planning to clean up my Laravel integration (with all the jingles - #103) and open source them in the coming weekend.

@wolfy-j
Copy link
Contributor

wolfy-j commented Mar 6, 2019

I will describe in the documentation for now.

@wolfy-j
Copy link
Contributor

wolfy-j commented Mar 6, 2019

@wolfy-j
Copy link
Contributor

wolfy-j commented Mar 6, 2019

I think the reason on not meeting this issue earlier is that most of the people rely on PSR-7 UploadedFileInterface which manage uploaded state separately. This issue seems to be Laravel/Symfony specific while mapping data to Request. Doesn't Symfony HttpKernel support file uploads set manually?

@hotrush
Copy link

hotrush commented Mar 28, 2019

@see0 any luck on your code release?)

@tarampampam
Copy link
Contributor

tarampampam commented Apr 4, 2019

We (@jetexe) made fix for this case, (changes, working release).

@diolan12
Copy link

I fix this in PHP by copy and then unlink the temp file. in my case, the temp file is in the C: drive meanwhile my project want to move it in D: drive. I remembered that os.Rename doesn't move file to a different drive, so I fix it by copy then unlink, os.Rename is not the case but this works great.

@egonbraun
Copy link

I am getting this issue when uploading files to my Laravel 9 and PHP 8.1 based backend running Roadrunner (latest version):

2022-10-06T11:20:49.766Z	INFO	server      	In Psr17Factory.php line 49:
2022-10-06T11:20:49.767Z	INFO	server      	  The file "/tmp/upload607333973" cannot be opened:
2022-10-06T11:20:49.767Z	INFO	server      	start [--laravel-path [LARAVEL-PATH]] [--relay-dsn RELAY-DSN] [--refresh-app] [--worker-mode WORKER-MODE]

Does anyone have a workaround?

@rustatian
Copy link
Member

RR manages uploads itself, and I guess Laraver interrupts this process and deletes the temporary file.

@rustatian
Copy link
Member

@egonbraun Do you use octane or this integration: https://github.com/spiral/roadrunner-laravel?

@egonbraun
Copy link

I am using the latter. The spiral/roadrunner-laravel integration.

@egonbraun
Copy link

RR manages uploads itself, and I guess Laraver interrupts this process and deletes the temporary file.

Looks like it. Are you aware of any way to change this behaviour? I know you are not a PHP or Laravel specialist but I could definitely use some ideas.

@rustatian
Copy link
Member

We may introduce a special flag, but, you may have a look at this ticket: roadrunner-php/laravel-bridge#84, I guess it was precisely your problem.

@egonbraun
Copy link

egonbraun commented Oct 6, 2022

At least he was able to get the files, I am not even reaching the controller at this point. Seems like the issue is happening while the request object is being created.

The class that fails is part of this library:

vendor/nyholm/psr7/src/Factory/Psr17Factory.php

So it looks more like what @see0 was describing here:

The problem is caused by all (JSON) Data are relayed thru goridge, and then restored to PSR-7 Message in the worker.

Hence, PHP is unable to add all the uploaded_files into their rfc1867_uploaded_files hash table - any function call to > is_uploaded_file will return a FALSE result.

When you try to perform any File Validation in Laravel/Symphony Request - the result will always be invalid.

@rustatian
Copy link
Member

To say in truth, we don't actively support this integration. You may try to use a SpiralFramework, which has an official RR integration. Laravel/Octane has very few features of the RR (like ~10%).

@elijahchancey
Copy link

To be clear, we're using Spiral + Roadrunner, and not Octane + Roadrunner.

Is there a suggested way of handling file uploads with Laravel9+Php8.1?

Thanks!

@wolfy-j
Copy link
Contributor

wolfy-j commented Oct 6, 2022

Maybe @butschster can help.

@elijahchancey
Copy link

It looks like https://github.com/avto-dev/roadrunner-laravel/pull/12/files might fix it? Perhaps the ability to set --not-fix-symfony-file-validation would fix the issue?

Or maybe we're processing file uploads in some deprecated way the relies on is_uploaded_file? Originally, our app was written on Laravel 5 and it has slowly evolved into a behemoth on Laravel 9.

@elijahchancey
Copy link

Not sure if this is helpful or not, but this is the error we see:

dev_irl                      | 2022-10-06T15:35:18.412Z	DEBUG	server      	worker is allocated	{"pid": 13716, "internal_event_name": "EventWorkerConstruct"}
dev_irl                      | 2022-10-06T15:35:18.414Z	INFO	server
dev_irl                      | 2022-10-06T15:35:18.416Z	INFO	server      	In Psr17Factory.php line 49:
dev_irl                      |   The file "/tmp/upload3578791433" cannot be opened:
dev_irl                      | start [--laravel-path [LARAVEL-PATH]] [--relay-dsn RELAY-DSN] [--refresh-app] [--worker-mode WORKER-MODE]
dev_irl                      | 2022-10-06T15:35:18.419Z	ERROR	http        	execute	{"start": "2022-10-06T15:35:18.158Z", "elapsed": "260.795709ms", "error": "sync_worker_exec_worker_with_timeout: Network:\n\tsync_worker_exec_payload: EOF"}
dev_irl                      | 2022-10-06T15:35:18.419Z	INFO	http        	http log	{"status": 500, "method": "POST", "URI": "/2.0/user/update", "remote_address": "127.0.0.1:43024", "read_bytes": 360038, "write_bytes": 0, "start": "2022-10-06T15:35:18.158Z", "elapsed": "261.082333ms"}
dev_irl                      | nginx access_log 172.18.0.1 "POST /2.0/user/update HTTP/1.1" 500 "curl/7.79.1" 0 0.263

We're using Intervention/image; but we think this error will occur with any uploaded file.

@elijahchancey
Copy link

Also, files don't appear in /tmp; they never seem to be written. This is the permission on /tmp: drwxrwxrwt

@elijahchancey
Copy link

Sorry about flooding this issue with comments; we were just VERY excited to move from FPM to Roadrunner. It showed a 22% reduction in overall latency and a 45% reduction of CPU compared to FPM.

@rustatian
Copy link
Member

NP at all, this is not a flood. But it would be better to move the discussion to a separate ticket. Could you please create a new ticket and copy-paste the discussion above?

@elijahchancey
Copy link

Will do!

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

No branches or pull requests

9 participants