Vapor fix - Delete temp file via queued closure instead of app terminating

[SanderMuller]

Exports in Laravel Nova on Vapor are giving a Illuminate\Contracts\Filesystem\FileNotFoundException while working locally.

Exception message:

File not found at path: temp/laravel-excel-e0Eeo8ZF7f6SYKKmzbqDI5GTKLBAlLtt.xlsx

This PR lets the disk handle the creation of the temporary URL to download the file when a remote disk is configured.
Without this change, it seems that it will try and download the local file instead of the remote file.

[patrickbrouwers]

Hey thanks for your PR,

There's already handling for files on remote disk, perhaps you can have a look over there why it doesn't work correctly;

https://github.com/SpartnerNL/Laravel-Nova-Excel/blob/1.3/src/Http/Controllers/ExcelController.php#L39

[SanderMuller]

Hey thanks for your PR,

There's already handling for files on remote disk, perhaps you can have a look over there why it doesn't work correctly;

https://github.com/SpartnerNL/Laravel-Nova-Excel/blob/1.3/src/Http/Controllers/ExcelController.php#L39

Thanks, I see what you mean. It's a bit hard to debug because it only happens on Vapor.
Maybe app()->terminating() which deletes the file is ran before the download is served on Vapor.

I'll see if I can have a look what is happening.

[SanderMuller]

@patrickbrouwers

I can confirm the app()->terminating() is the issue, removing the delete action makes it work on our Vapor environment.

I have not found a solution for this yet that works with Vapor (other than using a job or cron).

[SanderMuller]

With the help of the Serverless Laravel slack I came up with a solution to delete the file after the request on Vapor. It uses a queued closure. The PR is updated to reflect the new change.

Ideally we could check if it's running on Vapor, then use the queued closure, otherwise (on non Vapor) use app()->terminating().
This would require updating the config, though. Since it requires reading the ENV to check if the application is running on Vapor.

If you want I can add this to the PR, let me know @patrickbrouwers

[patrickbrouwers]

@SanderMuller what happens when you don't use queuing? (Even though it's probably safe to assume people using remote disks are probably also using queuing)

[SanderMuller]

@patrickbrouwers You're right, with sync as queue driver it deletes the file before serving it. A solution could be to do both: app terminate and a queued closure. Not the prettiest code, tho.

[patrickbrouwers]

But you mentioned terminating didn't work with Vapor?

[SanderMuller]

Terminating happens immediately on Vapor. So on Vapor it would immediately dispatch the job, which is fine. On non-Vapor with sync queue driver it would on terminate dispatch the job which is executed immediately (but after terminating)

[patrickbrouwers]

Okay that sounds pretty good then, I'm out of office until Monday, will check it when I'm back!

[SanderMuller]

Perfect, I've updated the PR with the combination of app terminating and the queued closure as we discussed

[SanderMuller]

Okay that sounds pretty good then, I'm out of office until Monday, will check it when I'm back!

Let me know what you think.
This PR would solve quite some things for our helpdesk team, since atm the default DownloadExcel actions give a file not found exception, and it would also mean we can drop the custom logic in one of our custom export actions.

[timothepearce]

This is a much needed feature (bug fix ?), @patrickbrouwers any update regarding the review?

Thanks :)

[SanderMuller]

@patrickbrouwers we are also still experiencing this issue on our live environment, could you have a look at it?
We've been using this solution for months on a custom export, but I would rather have this PR merged than refactor all our standard nova exports to custom exports to work around this issue

[SanderMuller]

@patrickbrouwers would appreciate if you could take a look

[SanderMuller]

@FrankPeters could assist in getting this PR reviewed/merged so we can export in Nova again on our live environment?

[patrickbrouwers]

Sorry haven't had any time for open source the last few months. I can't promise anything, but will try to squeeze in some time next week. Until then, you can always deploy your own fork.

[bretto36]

@SanderMuller @patrickbrouwers This is still a problem for me. Is there a solution for it, or should i just use the fork that SanderMuller referenced above?

[SanderMuller]

@SanderMuller @patrickbrouwers This is still a problem for me. Is there a solution for it, or should i just use the fork that SanderMuller referenced above?

We still use the fork ourselves

[patrickbrouwers]

This fix was merged right? What else is missing in the latest release that's on your fork?

[bretto36]

@patrickbrouwers I just don't think a tag has been made with these changes now that i look at 1.3.2
its in 1.3 just no new tag. If you could do that that would solve my problem

[patrickbrouwers]

I tagged a new release

[bretto36]

@patrickbrouwers Legend. Thank you very much

[bretto36]

@patrickbrouwers - hmm it doesn't seem like the change is in there - 1.3.8...1.3.10

I think i was looking at SanderMullers fork instead of this one

#183 i've made this one based on their fork