gohugoio / hugo

The world’s fastest framework for building websites.
https://gohugo.io
Apache License 2.0
75.89k stars 7.54k forks source link

getJSON etc. should log ERROR but return nil in failure situations #5076

Closed kaushalmodi closed 6 years ago

kaushalmodi commented 6 years ago

If getJSON fails to access a URL, it fails the Hugo build, and nothing gets built at all.

It would be better if it just returned nil. The user can then throw an errorf if needed on detecting the returned value as nil.

While this recently came up again in the forum, I've personally witnessed this issue too.

zabatonni commented 6 years ago

+1 soundcloud oembed returns error 404 instead of null and services could be down...

bep commented 6 years ago

It would be better if it just returned nil.

How is that better?

kaushalmodi commented 6 years ago

How is that better?

At times, the results of getJSON are not important enough to prevent building the site.

Real example: https://ox-hugo.scripter.co/doc/examples/

On that page, I get the "Last Updated" column dates using GitHub/Gitlab API. I have seen that I fail to retrieve the dates at times because of inability to fetch the JSON for whatever reason (rate-limiting from GitHub side may be?), and then rebuilding the site on Netlify usually fixes that.

I would rather leave those "Last Updated" dates blank than failing to build the site.

So.. depending on the application, that getJSON failure might not be important enough to fail the site build.

If it returns nil, user can choose to force fail the build using errorf. Also that behavior would be more consistent with behavior of .Get.

regisphilibert commented 6 years ago

I agree with @kaushalmodi here. Not in the context of https://github.com/gohugoio/hugo/issues/5074 but in the context of getJSON.

Could getJSON simply take a new bool param ? If set to true, it would only emit an warning and falls back on cached output (if exist).

kaushalmodi commented 6 years ago

falls back on cached output (if exist).

As I build on Netlify, I might not even have cached output; I'd be fine with getJSON returning nil.

zabatonni commented 6 years ago

nil is good, i can than output some default string or whatever fits.

bep commented 6 years ago

If it returns nil, user can choose to force fail the build using errorf. Also that behavior would be more consistent with behavior of .Get.

So, I agree that the current behaviour is broken. I just don't agree with the proposed "simple solution". I read a lot of these bug reports with statements saying "Hugo should ..." -- that comes from a single user's use of a given feature.

By .Get I assume you mean resources.Get, which:

So, .Get works exactly the same as getJSON-- it's just that getJSON is more flakey.

So, a fix for this needs to distinguish between "nothing found" and a failure. This may not seem too important if you use this to show the president's latest Tweets, but if you use this to build your net shop, you really want to know if it fails for some reason.

onedrawingperday commented 6 years ago

I would also like to see .Get errors not preventing a build, if it's possible.

This has come up in the Forum a couple of times. One report was about the default Tweet shortcode. The author of a tweet closed down her Twitter account. One of her tweets were embedded in a Hugo site and that caused the whole build to fail.

Current behavior is a bit restricting.

One has to keep an eye on whatever social media embeds are used in a project, and if the build is automated it will keep failing until someone notices.

regisphilibert commented 6 years ago
  • returns nil when a resource could not be found
  • returns an error when it fails to find the resource. This fails the build.

What's the difference between "not finding" and "fail to find"? I'm not grasping it.

kaushalmodi commented 6 years ago

the president's latest Tweets

Arghh, I am in US, don't sour this technical conversation.

zabatonni commented 6 years ago

large websites with tons of embeds are not maintainable because i have to fix (remove) them just to be able to publish new post.

kaushalmodi commented 6 years ago

that comes from a single user's use of a given feature.

It's more than a single user.

Let's create a poll (I cannot create one it seems):


getJSON failing to get the resource should:

  1. return nil (This would allow (getJSON <foo>) | default "bar").
  2. fail the site build.

onedrawingperday commented 6 years ago

@bep How about if .Get does a HTTP request by:

adding a httpHeadOnce with some caching to allow for checking HTTP response codes on remote resources.

The above quote is yours from https://github.com/gohugoio/hugo/issues/4755#issuecomment-391420482 about fetching video thumbnails for the youtube_simple shortcode.

If this is implemented I suppose that this issue could be fixed without having to return nil but instead a 404 warning, so that the build doesn't fail.

Also this would complete the work for the Privacy Enhanced shortcodes.

kaushalmodi commented 6 years ago

This issue is still not technically fixed for me. See https://github.com/gohugoio/hugo/pull/5199#issuecomment-420310515.

bep commented 6 years ago

Then open another issue.

github-actions[bot] commented 2 years ago

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.