Site template: set empty url in config file by default #5338

Merged
merged 1 commit into from Sep 23, 2016

Projects

None yet

7 participants

@ashmaroli
Contributor
ashmaroli commented Sep 8, 2016 edited

Addresses minima/#34 at the upstream.
This bug occurs due a recent change shipped with minima-1.1.0

/cc @jekyll/ecosystem
/cc @pathawks

@envygeeks envygeeks commented on the diff Sep 8, 2016
lib/site_template/_config.yml
@@ -20,7 +20,8 @@ description: > # this means to ignore newlines until "baseurl:"
line in _config.yml. It will appear in your document head meta (for
Google search results) and in your feed.xml site description.
baseurl: "" # the subpath of your site, e.g. /blog
-url: "http://example.com" # the base hostname & protocol for your site
+url: "" # the base hostname & protocol for your site, e.g. http://example.com
@envygeeks
envygeeks Sep 8, 2016 Member

This isn't required by Jekyll. Put it downstream.

@ashmaroli
ashmaroli Sep 8, 2016 Contributor

but Jordon, jekyll new creates the _config.yml with the url included.

@pathawks
Member
pathawks commented Sep 8, 2016

Thanks @ashmaroli for opening this here. This is not a minima issue.

If the site template is going to have a url in config, it should be an empty string with instructions on how to configure.

Jekyll is a static site generator, and generating URLs is foundational to that. Many many plugins (and now themes) rely on site.url.

See also this comment from parkr

Big 👍 from me.

@pathawks pathawks and 1 other commented on an outdated diff Sep 8, 2016
lib/site_template/_config.yml
@@ -20,7 +20,8 @@ description: > # this means to ignore newlines until "baseurl:"
line in _config.yml. It will appear in your document head meta (for
Google search results) and in your feed.xml site description.
baseurl: "" # the subpath of your site, e.g. /blog
-url: "http://example.com" # the base hostname & protocol for your site
+url: "" # the base hostname & protocol for your site, e.g. http://example.com
+ # [Required] for production environment.
@pathawks
pathawks Sep 8, 2016 Member

I don't know if this comment is necessary. Some plugins require absolute URLs, but most themes should not.

@ashmaroli
ashmaroli Sep 8, 2016 Contributor

Added that based on DirtyF's comment

@pathawks
pathawks Sep 8, 2016 Member

I assume when he said "you're gonna have to set this value" just meant to change it from example.com

@ashmaroli
ashmaroli Sep 8, 2016 Contributor

Righhtt... that makes sense 👍 😃
so.. remove that comment entirely or update to:

Update this value before going into production.

?

@pathawks
pathawks Sep 8, 2016 Member

My vote is to just remove line 24 and leave the comment on 23.

@envygeeks
Member

It appears you are right, so it's got a LGTM from me.

@envygeeks
Member

LGTM.

@ashmaroli
Contributor

@envygeeks, Thanks. Whats your take on the comment at Line 24? Update or Discard?

@envygeeks
Member

It's not required, none of my sites use that variable and it's not even in any of my sites configuration's.

@ashmaroli ashmaroli set empty url in config file by default
8813173
@DirtyF
Member
DirtyF commented Sep 13, 2016

@envygeeks 😮 interesting, I figured out site.url was needed as some point or another.

But if it's not useful, why embed it by default in _config_yml?

@envygeeks
Member

interesting, I figured out site.url was needed as some point or another.

@DirtyF I'll concede that I pretty much program all my own plugins and even had to fork Jekyll Sitemap and Jekyll Feed because of the way it handle's URL's, so I probably haven't run into that problem because I do tend to fork and customize a lot, most people don't.

But if it's not useful, why embed it by default in _config_yml?

I don't know if it's useful or not, that's up to the community, for me it's not useful, but for you it is, you've found that eventually you need it, that means that we should warn about it in some context and this is probably the best context to warn about it in.

@ashmaroli
Contributor

Taking abishov.com's config file ( the line with url: variable has been commented-out ), as a working example, my thoughts echo that of Jordon's comment above.

the url: variable seems to be an unnecessary definition.
So, I now propose to have that variable and its references removed from the default template.

@jaybe-jekyll
Member

Doesn't site.url provide for explicit linkages within feed.xml?

@ashmaroli
Contributor
ashmaroli commented Sep 19, 2016 edited

Doesn't site.url provide for explicit linkages within feed.xml?

@jaybe-jekyll, Sorry, I dont understand what you are asking.. Can you please rephrase?

@jaybe-jekyll
Member

@ashmaroli It was asked where site.url is used/employed/needed. The default/new minima theme' feed.xml references site.url to build proper feed links.

@ashmaroli
Contributor

Jekyll (HEAD) now uses jekyll-feed instead of hard-coded feed.xml. Since the plugin doesn't depend on site.url's presence to generate a site's feeds, even if we were to move in favor of removing the key from _config.yml altogether, it wont break things.
But giving a benefit of doubt to its dependency from other plugins, this PR is good to 🚢

@pathawks
Member

Since the plugin doesn't depend on site.url's presence to generate a site's feeds…

Yes it does, unless the site is hosted on GitHub Pages. Relative URLs in a feed cause problems.

@ashmaroli
Contributor

lobbying to have this merged a.s.a.p if its LGTM to the maintainers and release a pre-release version. At least we can give some respite to the users in despair

@envygeeks
Member

@ashmaroli Github doesn't work on our release cycle and us not theirs, so even if we did, it only fixes the problem locally really and I don't know that it would affect Github Pages unless they update Gem themes everyday.

@ashmaroli
Contributor

so even if we did, it only fixes the problem locally really

This would have us meet our users halfway in solving the issue. Pushing a new site into production with an empty url: is less punishing than having one with http://www.example.com
(IMHO)

@ashmaroli ashmaroli referenced this pull request Sep 23, 2016
Closed

Jekyll 3.3 Release Gameplan #5400

9 of 9 tasks complete
@parkr
parkr approved these changes Sep 23, 2016 View changes
@parkr parkr changed the title from set empty url in config file by default to Site template: set empty url in config file by default Sep 23, 2016
@parkr
Member
parkr commented Sep 23, 2016

@DirtyF @jaybe-jekyll Check out #5399. You might find you like that PR. 😄

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 992be25 into jekyll:master Sep 23, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Awaiting approval from at least 2 maintainers.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jekyllbot jekyllbot added bug fix labels Sep 23, 2016
@ashmaroli ashmaroli deleted the ashmaroli:config-patch branch Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment