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

adding a fix to support variables as params #85

Merged
merged 5 commits into from
Feb 22, 2024

Conversation

ArielMejiaDev
Copy link
Contributor

@ArielMejiaDev ArielMejiaDev commented Feb 14, 2024

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
Copy link
Member

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
Copy link
Contributor Author

ArielMejiaDev commented Feb 14, 2024

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
Copy link
Contributor Author

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

Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

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

Could you add tests for this to ensure that it works correctly?

@ArielMejiaDev
Copy link
Contributor Author

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 freekmurze merged commit 0371d5f into spatie:main Feb 22, 2024
1 check passed
@freekmurze
Copy link
Member

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

@freekmurze
Copy link
Member

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

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.

[Bug]: @inlinedImage only works with static urls
2 participants