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

Split remote fecthing in resources.Get into its own function #9285

Closed
regisphilibert opened this issue Dec 13, 2021 · 18 comments
Closed

Split remote fecthing in resources.Get into its own function #9285

regisphilibert opened this issue Dec 13, 2021 · 18 comments

Comments

@regisphilibert
Copy link
Member

regisphilibert commented Dec 13, 2021

I'm sorry to bring this up so late, it did not occur to me before upgrading but I'm having second thought about this functionnality sharing its name with a very commonly used method.

Many people are using CMS, with markdown images added to the body content. We then try and improve generated images with render hooks.

This meant a simple with resources.Get .Destination to find the local image, and if not found use the existing .Destination. But with resources.Get going "remote capable", now any URL passed through resource.Get is a resource created and potentially processed by Hugo.

I envision several problems. A website publishing its image on imgix, and relying on the imgix server to transform image like https://xxxx.imgix.net/home_slide_kayak.jpg?auto=format&ch=Width,DPR&q=95&w=2000. The latter being accidentally passed through resource.Get would trigger an uneccessary fetch and transformation.

Not that many use case comes to mind beside render-hooks, and it's of course something we can "guard" by looking at the passed string and making sure it's relative.

But I'm opening a discussion here, in case other use cases could fall into the same problem. I do think, a bit late sorry again, that something as powerfull, could have its own name/method and its own entry in the documentation site, rather than share it with another commonly used one. It would also ensure that we have 100% control on what is locally "got" and/or "remotely" fetched.

We have

  • resources.Get
  • resources.GetMatch
  • resources.Match

Adding a fourth one, resources.Fetch or other wouldn't hurt.

@bep
Copy link
Member

bep commented Dec 14, 2021

Hmmm... resources.Fetch is not ... great, but I can live with resources.GetRemote (which was my second choice when I thought hard about this).

I'm doing another release later this week. I will let the Hugo community decide:


@bep bep changed the title Add resources.Fetch for remote resources and limit resources.Get to local assets Split remote fecthing in resources.Get into its own function? Dec 14, 2021
@davidsneighbour
Copy link
Contributor

davidsneighbour commented Dec 14, 2021

Voting for .GetRemote but dark-mode in Github breaks the voting images. Transparent is bad these days.

@bep bep pinned this issue Dec 14, 2021
@bep
Copy link
Member

bep commented Dec 14, 2021

The latter being accidentally passed through resource.Get would trigger an uneccessary fetch and transformation.

@regisphilibert assuming you control the code, you can do

{{ if not (urls.Parse $url).IsRemote }}
{{ end }}

There are no accidents in the above? And you would do something similar if you would use the new GetRemote func.

@regisphilibert
Copy link
Member Author

And you would do something similar if you would use the new GetRemote func

Yes I would if the method was not renamed, but if it is renamed, then I'll stick with the original test (is there a local resource at the passed location?).
And yes, if I don't control the value passed to a .GetRemote, I'd safeguard it with your suggestion. That's helpful thanks!

@bep
Copy link
Member

bep commented Dec 15, 2021

Hmm ... The vote is clear.

@jmooring
Copy link
Member

This was bit like a trial, with the prosecution presenting evidence, and the jury voting to convict. I'm not sure the defense attorney was able to present their case.

@bep
Copy link
Member

bep commented Dec 15, 2021

@jmooring what did you vote?

@jmooring
Copy link
Member

.GetRemote and now I'm having second thoughts.

@bep
Copy link
Member

bep commented Dec 15, 2021

Yea, I'm still not sure ... One of Hugo's "problems" is a little too many moving parts. What I like about the Resource (which is a rather vague term in itself) thing is that is this rather simple interface that can be used for everything. And there is a a rather nice thing to also have only one way of getting these resources (there are more this this story, I know), especially when explaining it to the user (via documentation etc.).

@bep
Copy link
Member

bep commented Dec 15, 2021

... what about adding a resources.GetLocal function?

@regisphilibert
Copy link
Member Author

And there is a a rather nice thing to also have only one way of getting these resources (there are more this this story, I know), especially when explaining it to the user (via documentation etc.).

I'm focusing on the second part of that sentence. This functionality is extremely important as it makes Hugo capable of grabbing anything remote, (And yes API is what I'm mostly concerned about, and what I predict will make 80% of that feature's uses case), so let's make it stand out.

But also I think a function which grab a local file from the dedicated asset directory should not be the same as the one which give you access to the unlimited internet.

@regisphilibert
Copy link
Member Author

... what about adding a resources.GetLocal function?

That sounds like a big breaking change.

@jmooring
Copy link
Member

jmooring commented Dec 15, 2021

I'm a big fan of readme-driven development, because it forces you to explain it to someone before you do it.

I have a docs PR in progress for content-management/image-processing/index.md...


Image Resource

To process an image, you must access the image as either a page resource or a global resource.

Image as Page Resource

A page resource is a file within a [page bundle]. A page bundle is a directory with an index.md or _index.md file at its root.

content/
└── posts/
    └── post-1/           <-- page bundle
        ├── index.md
        └── sunset.jpg    <-- page resource

To access an image as a page resource:

{{ $image := .Resources.GetMatch "sunset.jpg" }}

Image as Global Resource

A global resource is a file:

  • Within the assets directory, or
  • Within any directory [mounted] to the assets directory, or
  • Located on a remote server accessible via http or https
assets/
└── images/
    └── sunset.jpg    <-- global resource

To access a local image as a global resource:

{{ $image := resources.Get "images/sunset.jpg" }}

To access a remote image as a global resource:

{{ $image := resources.Get "https://gohugo.io/img/hugo-logo.png" }}

@bep
Copy link
Member

bep commented Dec 15, 2021

That sounds like a big breaking change.

Not from Hugo 0.90 :-)

@regisphilibert
Copy link
Member Author

regisphilibert commented Dec 15, 2021

Not from Hugo 0.90 :-)

Oh so you mean a .GetLocal, limited to local (and .Get for both). So anybody willing to limit the previous .Get to local will have to search and replace in there project?

That seems confusing, but it's in your hand, I'm really grateful for that new feature! It's a Huge leap for Hugo, I think we should make it clear it's not limited to images, fonts etc... but... I'll stop now ;)

@bep
Copy link
Member

bep commented Dec 15, 2021

Why not?

How would adding a new function be a breaking change?

@bep
Copy link
Member

bep commented Dec 15, 2021

@regisphilibert never mind, I was just blabbering; this was a rather breaking change when adding resources.Get -- I will add a GetRemote and get that out some time this weak.

@jmooring I can also add the adjustment we discussed re this, if you have not already implemented it...

@bep bep changed the title Split remote fecthing in resources.Get into its own function? Split remote fecthing in resources.Get into its own function Dec 16, 2021
@bep bep added Enhancement and removed Proposal labels Dec 16, 2021
bep added a commit to bep/hugo that referenced this issue Dec 16, 2021
In Hugo 0.89 we added remote support to `resources.Get`.

In hindsight that was not a great idea, as a poll from many Hugo users showed. See Issue gohugoio#9285 for more details.

After this commit `resources.Get` only supports local resource lookups. If you want to support both, you need to use a construct similar to:

Also improve some option case handling.

```
{{ resource := "" }}
{{ if (urls.Parse $url).IsAbs }}
{{ $resource = resources.GetRemote $url }}
{{ else }}
{{ $resource = resources.Get $url }}
{{ end }}
```

Fixes gohugoio#9285
Fixes gohugoio#9296
@bep bep closed this as completed in 22ef5da Dec 17, 2021
@bep bep unpinned this issue Dec 23, 2021
@github-actions
Copy link

This issue 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 Jan 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants