Production hashed images not handled

[hybridvision]

Hi @Log1x,

Thanks for making this package - it's very helpful :)

As mentioned in #9, if Sage's yarn build:production has been run, the svg_image function doesn't find the hashed file name.

The problem is on this line:

https://github.com/Log1x/blade-svg-sage/blob/e764ca0a2733b5c3e437ee84dcac7bbc52409a77/src/helpers.php#L29

Specifically, the call to sage('assets')->get($icon), where $icon is just the icon name (eg. star). If you pass a simple string like 'star', Sage will never find any hashed assets, so it passes the original string back, which works in development (the filename hasn't changed) but breaks in production because the name has been hashed.

Assuming my SVG path is 'images/svg', when I call @icon('star'), the sage('assets')->get() function should be given the value images/svg/star.svg so it can correctly return the hashed filename (if one exists).

This is simple enough but due to the way the configuration is stored by the BladeSvg\SvgFactory class (it's private) and the way your package uses a filter, I couldn't see a very elegant way of fixing this without making too many changes to the packages.

Maybe you can suggest a better way but for now here's my ugly hack to get it to work properly:

function svg_image($icon, $class = '', $attrs = [])
{
    $extension = '.svg'; // Extension of SVG files
    $base_dist_path = get_dist_path(''); // Base path to "dist" folder
    $svg_path = apply_filters('bladesvg_image_path', get_dist_path('images/svg/icons')); // SVG path that has been set
    $relative_path = trailingslashit(str_replace($base_dist_path, '', $svg_path)); // The relative path inside the dist folder (needed for sage('assets')->get())
    $relative_icon_path = $relative_path . $icon . $extension;

    // Convert the final asset name back to original format (no extension or extra path added)
    // It's possible that we might use @icon('subdir/name') so we can't use the basename() function
    $icon = str_replace($extension, '', sage('assets')->get($relative_icon_path));
    $icon = str_replace($relative_path, '', $icon);

    return sage(\BladeSvg\SvgFactory::class)->svg($icon, $class, $attrs);
}

What do you think? I hope someone can find a more elegant solution but either way, it would be good to have a fix merged because the package is unusable in production without it.

@strarsis: this might interest you too.

[isabisa]

I'm having this exact same issue. I added the hack suggested by @hybridvision into my app/setup.php file and changed all the references to the @svg directive to App\svg_image() temporarily. Look forward to this being resolved @Log1x, but thanks for the workaround @hybridvision!

[strarsis]

@isabisa, @hybridvision, @Log1x: I forked it and added sage asset path handling, PR https://github.com/Log1x/blade-svg-sage/pull/12.

While the PR is being reviewed you can already try out the fix by installing it from the fork: Adjust the composer.json of the theme:

    "repositories": [

        {
            "type": "vcs",
            "url": "https://github.com/strarsis/blade-svg-sage"
        }

    ],

    "require": {

        "log1x/blade-svg-sage": "dev-sage-paths"

    }

Update dependencies:

$ composer update

This works well for me in development and production builds.

[hybridvision]

Thanks @strarsis! It seems to work well for me :)

I noticed one minor bug - the URL returned has too many slashes in it (eg. path/to///file.svg) and with some debugging, I see it's because of your strip_file_ext function - it returns the filename with a ./ in the front and then the BladeSvg\getSvg function is replacing the . with another /. The extra slashes seem to be ignored by the server so it's not a big deal but maybe it could cause problems on some systems so it would be good to fix.

If you simply do return pathinfo($path, PATHINFO_FILENAME); in your function, it works correctly. I'll leave you a comment in your PR also.

[strarsis]

@isabisa, @hybridvision: Edit: It has been merged now, packagist has been updated by @Log1x, you can pull it now directly from it.

[hybridvision]

@Log1x, @strarsis - revisiting this, I just realised that when I suggested using pathinfo() to get the base filename without extension, it creates another problem: it also strips out any subdirectories that are in the relative path.

I ran into this problem now because I have most of my SVG files in resources/images and that is the SVG path I have configured. However, I also have other SVGs in subdirectories like resources/images/icons and when I reference these from the production build, I get an error because although I pass the name as @svg('icons/myicon'), it returns the hashed path without the "icons" part of the filename and then I get an exception.

To solve this, we could use the pathinfo() function to include the directory name but that would result in the same situation as mentioned here where some paths are returned with ./ at the beginning.

I think the simplest way to remove the extension while leaving everything else in place is to find the last . and then remove everything after that. This allows filenames to have extra dots in them and also to have different extensions.

I'll submit a PR but this is the change I suggest here:

-$icon_imgpath_noext = pathinfo($icon_path_rel, PATHINFO_FILENAME);
+$icon_imgpath_noext = substr($icon_path_rel, 0, strrpos($icon_path_rel, '.'));

[strarsis]

@hybridvision: What about normalizing the path first (resolving all relative segments (./, ../)) using realpath?

[hybridvision]

@strarsis: normally there shouldn't be any relative segments like ../ in the path because you pass a path that is relative only to the bladesvg_image_path - e.g @svg('icons/myicon'). Using realpath gives the full absolute file system path but all we want is the path of the hashed file relative to bladesvg_image_path, without an extension.

Maybe you could refactor to use realpath but I don't see how it solves the specific problem I had...

Do you see an issue with the solution I proposed in my PR?

[strarsis]

@hybridvision: When there are no other side effects then this is a nice fix, I hope this is merged so I wouldn't run into this issue either in the future. Though IMHO I prefer existing path manipulation methods, ideally a method can be found that just handles the redundant ./ correctly.

[hybridvision]

I completely agree about using the standard PHP path manipulation functions whenever possible.

The first thing I tried was incorporating pathinfo($icon_path_rel, PATHINFO_DIRNAME); but then I needed to concatenate that with the filename and add a slash between them... In many cases using something like @svg('mySVGfile') (no subdirectory reference), the returned dirname is simply ., so you'd end up with ./ at the beginning of the string. I could have added checks for this case but it would make the code way more convoluted...

It's a relatively small detail so I think my solution is robust and clean enough :)

[strarsis]

@Log1x: Would it be possible to merge the PR from @hybridvision?