benjaminkott / bootstrap_package

Bootstrap Package delivers a full configured theme for TYPO3, based on the Bootstrap CSS Framework.
https://www.bootstrap-package.com/
MIT License
338 stars 205 forks source link

url()-function in ScssParser doesn´t resolve urls to public resources in composer-mode #1445

Open Q4U-mspitz opened 1 year ago

Q4U-mspitz commented 1 year ago

Hello,

this is my first Bug-Report.

Bug Report

Prerequisites

Description

The url-parser inside the ScssParser doesn´t resolve the paths to public resources correct in a composer-environment. Instead of resolving the "Resources/Public"-path of an extension to "_asset/id", it delivers the server-path.

Steps to Reproduce

I build my own theme.scss in my sitepackage and want to link to resources inside the public-folder: background-image: url('../../../Public/Icons/test.svg');

As a result in the parsed css, i get: background-image: url("../../../../var/www/html/vendor/q4u/sitepackage/Resources/Public/Icons/test.svg");

In composer-mode the public resources are linked in the _asset-folder, so i expect something like this: background-image: url("../../../../_assets/1aed38cddafbbebcfacb851cbb5e0b11/Icons/test.svg");

This behavoir can also be reproduced with absolute paths.

Expected behavior

Resolving the resources to the _assets-Folder for composer-installations.

Actual behavior

The resources are resolved to an absolute server path prefixed with some "../"

Possible solution

I debugged a bit. Maybe this could work (just tested with relative paths in composer-mode):

Inside Classes/Parser/ScssParser.php: Instead of:

                if (substr_compare($result, 'data:', 0, 5, true) !== 0) {
                    if (is_file(PathUtility::getCanonicalPath($absoluteFilePath . '/' . $result))) {
                        $result = PathUtility::getCanonicalPath($relativeFilePath . '/' . $result);
                    } elseif (is_file(PathUtility::getCanonicalPath($absoluteBootstrapPackageThemePath . '/' . $result))) {
                        $result = PathUtility::getCanonicalPath($relativeBootstrapPackageThemePath . '/' . $result);
                    }
                    $result = substr($result, 0, 1) === '/' ? substr($result, 1) : $result;
                }

surround the results with PathUtility::getAbsoluteWebPath() like this:

                if (substr_compare($result, 'data:', 0, 5, true) !== 0) {
                    if (is_file(PathUtility::getCanonicalPath($absoluteFilePath . '/' . $result))) {
                        $result = PathUtility::getAbsoluteWebPath(PathUtility::getCanonicalPath($relativeFilePath . '/' . $result));
                    } elseif (is_file(PathUtility::getCanonicalPath($absoluteBootstrapPackageThemePath . '/' . $result))) {
                        $result = PathUtility::getAbsoluteWebPath(PathUtility::getCanonicalPath($relativeBootstrapPackageThemePath . '/' . $result));
                    }
                    $result = substr($result, 0, 1) === '/' ? substr($result, 1) : $result;
                }

Versions

TYPO3: 12.4.8 bootstrap_package: 14.0.7

klodeckl commented 10 months ago

Same here. TYPO3 11.5.33 cms-composer-installers 4.0rc1

Your solution did not fix it in my setup, I then got a path like ../../../../_assets/Public/anotherpath (instead of Public there should be the random id folder name.

Q4U-mspitz commented 10 months ago

@klodeckl You have to change something in your scss-file to get the css-parser triggered. Do you use a relative path in your scss to your image? Do you stay inside your Extension?

I had a scss-file in: extname/Resources/Private/SCSS/theme.scss

And an icon in: extname/Resources/Public/Icons/icon.svg

For me this relative path inside the scss works with my modification: url(' ../../Public/Icons/icon.svg')

Q4U-mspitz commented 10 months ago

Maybe it´s a good idea, to allow extension-paths inside the scss-parser: For example: url('EXT:extname/Resources/Public/Icons/icon.svg')

To do this, add another elseif to this block inside Classes/Parser/ScssParser.php starting at line 151:

                if (substr_compare($result, 'data:', 0, 5, true) !== 0) {
                    if (is_file(PathUtility::getCanonicalPath($absoluteFilePath . '/' . $result))) {
                        $result = PathUtility::getAbsoluteWebPath(PathUtility::getCanonicalPath($relativeFilePath . '/' . $result));
                    } elseif (is_file(PathUtility::getCanonicalPath($absoluteBootstrapPackageThemePath . '/' . $result))) {
                        $result = PathUtility::getAbsoluteWebPath(PathUtility::getCanonicalPath($relativeBootstrapPackageThemePath . '/' . $result));
                    } elseif (str_starts_with($result, 'EXT:') && is_file(GeneralUtility::getFileAbsFileName($result))) {
                        $result = PathUtility::getAbsoluteWebPath(GeneralUtility::getFileAbsFileName($result));
                    }

                    $result = substr($result, 0, 1) === '/' ? substr($result, 1) : $result;
                }
jokumer commented 10 months ago

@Q4U-mspitz

Remove /Public from your url. Just enter the path within the Public file folder, and prepend with ../../. Depth depends on your starting scss file given in includeCSS.theme relative to the Public folder your scss file is located in. This depth is the same for all imported scss files.

eg. background-image: url('../../Icons/test.svg'); instead of background-image: url('../../Public/Icons/test.svg');

It results in expected path background-image: url('../../../../_assets/[id]/Icons/test.svg');

Version TYPO3: 12.4.10 bootstrap_package: 14.0.7

Shall we document it in the docs or wiki with a pull request?

I like your Idea, with full path url('EXT:extname/Resources/Public/Icons/icon.svg'). This would be the same pattern as recommended in FLUID files, and would make the world for frontend-development a bit easier.

klodeckl commented 10 months ago

I can confirm this works with relative paths (same extension) and full paths like EXT:extname/Resources/Public/… It would be great if this code could be integrated into the next bootstrap_package version. Using full path via EXT:extname/Resources/Public/ is necessary otherwise linking to a file from another extension would not be possible.

jokumer commented 10 months ago

... had the same thoughts about "linking to a file from another extension"

klodeckl commented 10 months ago

Explaining the feature in the documentation for others would be good.

vicluber commented 9 months ago

Thanks for this issue. It made me stop looking for the proper solution to this. This needs a better solution. Using the EXT: prefix seems perfectly intuitive in the context. What about the hashy looking of the folder names in _assets? Does anybody know if it will change at some point?

klodeckl commented 9 months ago

@vicluber Sorry, I don’t get your point. IMO this is an optimal solution. Yes, the folder name in _assets could change.

vicluber commented 9 months ago

@klodeckl What I meant is that if these "hashy" looking folder names for each extension's public folder could change, this background-image: url("../../../../_assets/1aed38cddafbbebcfacb851cbb5e0b11/Icons/test.svg"); is not a proper solution (if "1aed38cddafbbebcfacb851cbb5e0b11" would be something else at some point the resource won't be found)

This would be a good solution: background-image: url('EXT:extname/Resources/Public/Icons/icon.svg') Or _assets/extension_name/Icons is also a good solution.

I'm gonna try the comment from @Q4U-mspitz

klodeckl commented 4 months ago

For including fonts src should also be considered.