spatie / laravel-pdf

Create PDF files in Laravel apps
https://spatie.be/docs/laravel-pdf
MIT License
681 stars 53 forks source link

adding a fix to support variables as params #85

Closed ArielMejiaDev closed 6 months ago

ArielMejiaDev commented 7 months ago

What makes this PR

close #84

This does work:

<div>
   @inlinedImage('https://picsum.photos/200/300')
</div>

This does NOT work:

<div>
   @php $src = 'https://picsum.photos/200/300' @endphp
   @inlinedImage($src)
</div>

As far as I understand any service provider is executed too early in an application lifecycle, so it cannot evaluate a variable value that is set in a controller.

To solve this all the code is now executed at the same time to be executed in the blade file directly.

The solution is probably not so elegant, but solves both cases, static strings and variables as params

Explanation with the solution

https://www.loom.com/share/00cc167045d04310ae194ac2af157d07?sid=a984958f-c830-4077-b263-684eda2378d6

@freekmurze if you can imagine a better solution I would be glad to implement it, thanks

freekmurze commented 7 months ago

There's now quite some duplication in the try/catch block which makes it hard to see what exactly is being catched.

Refactor this so that a variable $content is being set in the try/catch block. Under the try/catch block return the full image HTML. This will make it more readable.

ArielMejiaDev commented 7 months ago

There's now quite some duplication in the try/catch block which makes it hard to see what exactly is being catched.

Refactor this so that a variable $content is being set in the try/catch block. Under the try/catch block return the full image HTML. This will make it more readable.

Hi @freekmurze I found a complicate scenario that is why the code is written in this way, with the current code in main branch:

@inlinedImage('assets/images/logo.png')

Works as this code inside the package service provider is getting:

Blade::directive('inlinedImage', function ($url) {
    dd($url); // returns the string "assets/images/logo.png"            
});

But with a variable from a controller or using php directive to set it:

@php $image = 'assets/images/logo.png'; @endphp

@inlinedImage($image)

It returns the variable name as a string

Blade::directive('inlinedImage', function ($url) {
    dd($url); // returns the string "$image"            
});

I think this is a limitation I think because it is too early in the application lifecycle, so a variable is replaced I think when Blade compiles, that is why I had to set all the code at once.

If I set a $content variable with a file_content at that moment of the runtime (with a variable param) the code would execute something like this:

$content = base64_encode(file_get_contents(asset("$url")));
// or
$content = base64_encode(Http::get("$url"));

I think I did not miss anything, I would be looking forward your feedback, and thanks!

ArielMejiaDev commented 7 months ago

The Last commits make @inlinedImage:

[x] Works with static string as params [x] Works with variables as params [x] Cleanup Blade Directive Code [x] Always returns an exception if an image fails to fetch (absolute path) or is not found (relative path)

Demo

https://www.loom.com/share/a7a760129c93452cb1006ff4cf21de53?sid=73be0c89-de9b-4367-aa6a-8b4c5118bdc1

It should close #84

ArielMejiaDev commented 6 months ago

Hi @freekmurze I know that you are probably a lot more busy, if you have any chance to check these new changes that add tests to generate images for headers and footers please let me know.

I am glad to help and if there is any feedback it would be welcome, I let you know how ingenious looks the workbench implementation is to test generated files from the package.

freekmurze commented 6 months ago

Thanks! I'll check on main if the tests pass there.

freekmurze commented 6 months ago

The tests are failing, could you send another PR to fix them?