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

Improve resource paths #4501

Closed
wants to merge 3 commits into from

Conversation

LorenzCK
Copy link
Contributor

These two small changes improve the generation of target paths for bundle resources that are transformed (i.e., which use the image transformation methods such as Resize).

Issue description

Duplicated relative path

When generating paths for transformed resources in page bundles (both headless or not), the relative path of the bundle is added twice to the final image path.
For instance, source resource:

/section/page/name/123/resource.png

once transformed, will end up as:

/public/section/page/name/123/section/page/name/123/resource_hu49c03114a…q50_box.png

This is not an issue per se, but generates unnecessarily deep directory hierarchies and very long URLs.
Very deep directories can lead to problems with tools on Windows that are affected by the operating system's old MAX_PATH limitations (e.g., Filezilla, when uploading the generated website, which is how I found the problem).

Commit 9fd19dc fixes this by removing the duplicated call to resource.relTargetPathForRel when generating the transformed resource's cache key.
The transformed image in the example above will be correctly stored along the original resource:

/public/section/page/name/123/resource.png
/public/section/page/name/123/resource_hu49c03114a…q50_box.png

Relative resource paths

Transformed bundle resources do not keep their relative path to the owning bundle. For instance, given a bundle with the following files:

/section/bundle/index.md
/section/bundle/resource1.jpg
/section/bundle/subdir/resource2.jpg

the destination directory will keep the bundle's structure for the source files, but transformed files will be flattened to the bundle's root directory. That is, the final file structure will look like:

/public/section/bundle/index.html
/public/section/bundle/resource1.jpg
/public/section/bundle/resource1_hu123….jpg
/public/section/bundle/subdir/resource2.jpg
/public/section/bundle/resource2_hu234….jpg

Notice that the last file is not stored inside the subdir directory.

This is due to how the image key is generated in image.filenameFromConfig, using the FileAndExt helper that strips relative path information from the resource file.
This is fixed in commit 5a1a0c8 by keeping track of the resource's path relative to the owning bundle and adding it to the generated image key.

Note

This is my first contribution and I just recently started using both Go and Hugo, I hope there are no major mistakes. 😊

Removes (duplicated) application of relTargetPathForRel() function to the key of transformed resources. The final relative target path is already correctly determined by target() function (resource.go:554) when writing transformed resources to disk.
@CLAassistant
Copy link

CLAassistant commented Mar 12, 2018

CLA assistant check
All committers have signed the CLA.

@LorenzCK LorenzCK changed the title Feature resource paths Improve resource paths Mar 12, 2018
@bep
Copy link
Member

bep commented Mar 12, 2018

I think I understand the issue, which I admit I hadn't thought too hard about.

If would be good if you could provide a test that demos the problem (i.e. a test that failed before you applied your patch). Also, there are existing tests that are failing.

Since relative bundle paths are now maintained when generating final transformed resources (while the imagecache uses a flattened structure), this simple fix makes the assertFileCache function go look for the base filename of resources.
@LorenzCK
Copy link
Contributor Author

LorenzCK commented Mar 13, 2018

Hi @bep,
I did look into the failing test. I had to slightly change how the presence of a transformed image inside the image cache is verified in testhelpers_test.go. Since resource permalinks now maintain the relative path to their owning bundle, the helper would look inside the /a/ sub directory inside the image cache directory (which instead keeps a flat hierarchy).

Also, here is a sample Hugo site I used to test the feature while developing it.
In the ZIP you’ll find directories /public-0.37 (output using the current Hugo release) and /public-PR (output using the submitted changes).

Totally agree on providing some tests: I’ll need some more time to get a look around how everything works before I submit them, however.

Thanks!

@bep bep added this to the v0.38 milestone Mar 13, 2018
@bep
Copy link
Member

bep commented Mar 13, 2018

@LorenzCK your test fix is adjusting the test to fix the test, it would have been better to ... fix the code. I will fix it. Thanks for this. Odd that I have not noticed this.

@bep
Copy link
Member

bep commented Mar 13, 2018

I will look at this tomorrow, I need a clear head to understand my own path logic...

bep added a commit to bep/hugo that referenced this pull request Mar 14, 2018
bep added a commit to bep/hugo that referenced this pull request Mar 14, 2018
bep added a commit to bep/hugo that referenced this pull request Mar 14, 2018
bep added a commit to bep/hugo that referenced this pull request Mar 14, 2018
bep added a commit to bep/hugo that referenced this pull request Mar 14, 2018
@bep bep closed this in #4503 Mar 14, 2018
@bep
Copy link
Member

bep commented Mar 14, 2018

I had to take another approach to fix this. Your patch fixed the main issue, but it had a side effect in flattening the resource cache. This should work now and be part of the release on Monday. Thanks for the detailed report!

@LorenzCK
Copy link
Contributor Author

Great! Thank you for Hugo 👍

@github-actions
Copy link

github-actions bot commented Feb 6, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants