always include file extension on destination files #3490

willnorris · 2015-02-21T02:31:36Z

This is an initial take at always including file extensions on destination files written to disk. This is a continuation of the discussion that's happened on #2294 and #3463, and coincides with the WEBrick changes in #3452.

This should only have any impact when the site or post permalink setting is configured without either a trailing slash or the file extension for that file type. In that case, the file written to disk will contain the correct extension for that type, while the URL will remain as specified in the permalink setting.

Notably, this does break the test that was added in #2925 to support files ending in ".htm" rather than ".html" (cc @pathawks to discuss), because this is using the output_ext value to determine the expected extension for the destination file. I believe this is the correct behavior. I think that any attempt to support alternate extensions (like ".htm") should make sure one way or another that output_ext returns the desired alternate extension. Not doing so seems likely to cause more problems down the road. Either way, we certainly need to resolve this breaking test before this can be safely merged.

parkr · 2015-02-21T02:52:45Z

lib/jekyll/document.rb

@@ -164,6 +164,8 @@ def destination(base_directory)
      dest = site.in_dest_dir(base_directory)
      path = site.in_dest_dir(dest, URL.unescape_path(url))
      path = File.join(path, "index.html") if url =~ /\/$/
+      output_ext = Jekyll::Renderer.new(site, self).output_ext
+      path += output_ext if path !~ /#{output_ext}$/


if path.end_with?(output_ext)?

parkr · 2015-02-21T02:53:37Z

So dope. Thanks for the PR!

parkr · 2015-02-21T22:49:21Z

What about checking for the existence of a file extension before adding this one?

>> File.extname("path/to/file.htm")
=> ".htm"
>> File.extname("path/to/file.htm").empty?
=> false
>> File.extname("path/to/file")
=> ""
>> File.extname("path/to/file").empty?
=> true

If it's empty, then add the output extension. Otherwise, pass.

Also, let's use << instead of +=.

parkr · 2015-02-21T22:54:38Z

test/source/_posts/2015-02-20-extensionless-permalink.markdown

+permalink: /:title
+---
+
+Test


Can we output {{ page.url }} here to ensure the Liquid URL is being set properly?

willnorris · 2015-02-21T23:27:26Z

What about checking for the existence of a file extension before adding this one?

So I did think about that. It will certainly work, but there's two things I didn't like about it:

it prevents having a period in your post title if you have an extensionless permalink structure. Perhaps a theoretical argument, as I'm not sure how often people actually use periods in post titles (I know I never do)
it doesn't address the issue of always being able to trust output_ext to accurately identify the extension of the destination file. Based on how it's being used today, I don't actually see any immediate issues, but it just feels wrong to me.

That said, I'm happy to add it in if you'd like.

Also, let's use << instead of +=.

Done.

willnorris · 2015-02-22T03:27:05Z

okay, thinking more about it, I agree that respecting existing extensions in permalink is probably the way to go for right now at least. This next commit should be safe to merge I think.

parkr · 2015-02-22T10:16:34Z

okay, thinking more about it, I agree that respecting existing extensions in permalink is probably the way to go for right now at least. This next commit should be safe to merge I think.

You make good points... people should be able to use dots in their files, and we don't want to limit file extensions... Hm...

parkr · 2015-02-22T10:17:23Z

Would you mind adding a test for Document and Page as well? Just until they're all one class...

willnorris · 2015-02-22T19:47:07Z

okay, thinking more about it, I agree that respecting existing extensions in permalink is probably the way to go for right now at least. This next commit should be safe to merge I think.

You make good points... people should be able to use dots in their files, and we don't want to limit file extensions... Hm...

okay, yeah... I now see that there are test cases that specifically focus on files with dots in their name, so this is definitely something we'll want to support. So now I'm back to feeling like we will need to back out the test case from #2925 and find another way to support that (if at all). As @pathawks mentioned on that bug, he could setup redirects for the legacy URLs, which may be the better option here.

Right now, I'm fairly certain that Pages still end up with URLs contain the ".html" extension. That never actually impacted me, so I sort of forgot about it. I'll look into what it will take to fix that, and add tests for that and Document (as well as additional tests for dotted filenames).

parkr · 2015-02-25T07:23:15Z

I think we should be able to support htm extensions, even if just to maintain backwards compatability with the hundreds of thousands (guessing, maybe more or less) of GitHub Pages sites that are presently out there... Perhaps we do check against a list of possible file extensions? That may slow down this method considerably...

One idea is to release this in a beta, have @pathawks take a look, and come up with a fix for that later.

Right now, I'm fairly certain that Pages still end up with URLs contain the ".html" extension. That never actually impacted me, so I sort of forgot about it. I'll look into what it will take to fix that, and add tests for that and Document (as well as additional tests for dotted filenames).

Yes, I believe they are presently just as flexible as Posts, so it would be good to have them under test. Thank you!!

pathawks · 2015-02-25T14:59:17Z

I'd be happy to test 👍

You want I should test this pull request?

parkr · 2015-02-25T15:49:32Z

@pathawks Yes please.

willnorris · 2015-02-25T17:29:34Z

Yes, I believe they are presently just as flexible as Posts, so it would be good to have them under test. Thank you!!

Actually pages are the worst in terms of permalink flexibility (from _config.yaml that is). If you're not using :pretty, then pages always use a template of /:path/:basename:output_ext. This is a large part of #2691, which I'm trying to fix here as well (haven't uploaded that commit yet.. that's what prompted #3507)

parkr · 2015-02-25T18:33:40Z

Ok then this PR looks good to me.

@pathawks, let me know what you figure out. It's hacky, but we could also check for .htm...

parkr · 2015-02-25T18:45:51Z

lib/jekyll/post.rb

@@ -280,6 +280,7 @@ def destination(dest)
      # The url needs to be unescaped in order to preserve the correct filename
      path = site.in_dest_dir(dest, URL.unescape_path(url))
      path = File.join(path, "index.html") if self.url =~ /\/$/


also this should totally be .end_with?("/")... not sure if that's for this PR but it's an optimization we should take advantage of. i'll write a benchmark to ensure this

willnorris · 2015-03-02T01:15:05Z

@parkr: as previously noted, we do have tests that ensure that files with dots in them work, which this method of simply looking for a file extension doesn't work with. As a result, this next commit switches back to enforcing output_ext exactly. This also necessitates removing @pathawks' feature test for .htm permalinks. Supporting non-standard file extensions like this will likely need to be done in a plugin, which I'll take a look at separately, just to show how it would be done for those that are interested.

This next commit also includes tests for posts and documents.

parkr · 2015-03-02T02:23:23Z

What about allowing configuration of HTML files' file extension? is that crazy?

willnorris · 2015-03-02T02:27:42Z

What about allowing configuration of HTML files' file extension? is that crazy?

personally I think so, yes. :)

Also keep in mind that I'm fairly certain @pathawks was only interested in having ".htm" extension for legacy, imported-in content. I don't think there is any real demand for having .htm versus .html long-term, we just need a solution that is good enough to handle Pat's imported content (which I personally would still argue that just redirecting to the new URLs would be the much simpler approach)

parkr · 2015-03-02T02:29:24Z

Also keep in mind that I'm fairly certain @pathawks was only interested in having ".htm" extension for legacy, imported-in content. I don't think there is any real demand for having .htm versus .html long-term

👍

parkr · 2015-03-02T02:29:46Z

also /cc @benbalter for that great image above. ben's always talking about our 🎄 of options.

pathawks · 2015-03-02T02:36:17Z

I'm fairly certain @pathawks was only interested in having ".htm" extension for legacy, imported-in content.

Yup.

just redirecting to the new URLs would be the much simpler approach

Yup.

willnorris · 2015-03-03T02:55:10Z

@pathawks: in my brief testing, this plugin seems to do the trick of preserving the ".htm" extension when a post's custom permalink contains that extension:

module Jekyll
  module Convertible
    def output_ext_with_htm
      ext = output_ext_without_htm
      ext = ".htm" if self.permalink.to_s.end_with?(".htm")
      ext
    end

    alias_method :output_ext_without_htm, :output_ext
    alias_method :output_ext, :output_ext_with_htm
  end
end

Or of course, you could also just redirect from within the web server itself.

Assuming that is okay and we're fine removing this behavior from jekyll core, then this should be ready for final review. @parkr could you take another look (there's a few additional tests now as well)

This ensures that destination files for HTML posts, pages and collections always include the proper file extension (as defined by output_ext) regardless of permalink structure. This allows for URLs that contain no extension or trailing slash to still result in proper destination files with .html extensions. Because this change relies so heavily on output_ext accurately identifying the extension of the destination file, this change also removes the feature test that tested support for permalinks with a .htm extension. In order to support alternate file extensions, a future patch or plugin will need to modify the output_ext value, at which point everything else should work as expected.

parkr · 2015-03-03T06:23:12Z

I'm fine with removing this feature. We'll need to include it on our upgrading docs page.

parkr · 2015-03-03T06:52:08Z

This looks great to me!

Thanks, Will!

willnorris · 2015-03-03T07:09:21Z

I don't think it necessarily needs mentioning in the upgrade docs, since this never actually worked until #3014, which has only shipped with 3.0.0.beta1.

(and just to make sure, "ship-it squirrel" == "go ahead and merge"? Or should I wait for you or someone else to merge?)

pathawks · 2015-03-03T14:51:42Z

👍

parkr · 2015-03-03T18:23:25Z

(and just to make sure, "ship-it squirrel" == "go ahead and merge"? Or should I wait for you or someone else to merge?)

Yep, means I'm cool merging it. Thanks for checking :)

alfredxing · 2015-03-03T18:45:18Z

Looks good! Though this only works for Markdown and converted files, right?

So if I had a a-page.html:

---
layout: default
permalink: /a-page
---
<strong>This is a page</strong>

It would output as /a-page, not /a-page.html?

willnorris · 2015-03-03T18:49:30Z

It works just as well for non-converted files. If there is no converter registered for the file extension, then output_ext returns the extension of the source file. So in your example, the page would have a url of /a-page, but the destination file would be /a-page.html.

always include file extension on destination files

alfredxing · 2015-03-03T20:56:30Z

@willnorris Right, forgot about that. Thanks!

snickers0129 · 2015-04-27T18:43:13Z

I like the idea of configuring the html extension for .html files as well... I'm hosting on Amazon S3, which supports serving files that are extension-less and doesn't really offer configuration

willnorris · 2015-04-27T19:38:24Z

@snickers0129 you could do a variation of #3490 (comment) to customize the extension for HTML output files.

willnorris · 2015-04-27T19:39:28Z

err, you might actually need to patch the Markdown converter, but it would still be the output_ext method.

envygeeks · 2015-04-27T19:39:37Z

Why not just do folder based navigation? The AWS docs states that's supported?

snickers0129 · 2015-04-27T19:50:09Z

@willnorris Thanks! I saw that... I just thought I'd cast my vote!

@envygeeks You mean putting folders with index.html in them? That causes 302 redirects from url/page to url/page/

envygeeks · 2015-04-27T20:48:29Z

Oh fair enough, I didn't notice it would do that crazy mess, but I guess I shouldn't be surprised it is a CDN.

parkr reviewed Feb 21, 2015
View reviewed changes

parkr added the Enhancement label Feb 21, 2015

parkr reviewed Feb 21, 2015
View reviewed changes

willnorris mentioned this pull request Feb 24, 2015

several tests still use .textile files #3507

Closed

parkr reviewed Feb 25, 2015
View reviewed changes

willnorris mentioned this pull request Mar 1, 2015

'pretty' permalinks do not match URL TEMPLATE #2691

Closed

willnorris added a commit that referenced this pull request Mar 3, 2015

Merge pull request #3490 from willnorris/ext

5d1a24e

always include file extension on destination files

willnorris merged commit 5d1a24e into jekyll:master Mar 3, 2015

willnorris added a commit that referenced this pull request Mar 3, 2015

Update history to reflect merge of #3490 [ci skip]

5e9b734

willnorris deleted the ext branch March 3, 2015 18:55

bdesham mentioned this pull request Mar 8, 2015

Permalink setting assumes that filenames will end with “.html” #3554

Closed

This was referenced Mar 9, 2015

update permalinks docs #3556

Merged

URL Structure changes #3463

Closed

Content Negotiation and Pretty URLs #3345

Closed

willnorris mentioned this pull request Mar 20, 2015

3.0.0.pre.beta2 breaks permalinks on existing sites #3602

Closed

willnorris mentioned this pull request Jun 23, 2015

Cannot produce files without extension #3809

Closed

pathawks mentioned this pull request Feb 2, 2016

Blog posts with trailing slashes no longer work in Jekyll 3 #4440

Closed

jekyll locked and limited conversation to collaborators Feb 27, 2017

jekyllbot added the frozen-due-to-age label Feb 27, 2017

always include file extension on destination files #3490

always include file extension on destination files #3490

Conversation

willnorris commented Feb 21, 2015

parkr Feb 21, 2015

Choose a reason for hiding this comment

willnorris Feb 21, 2015

Choose a reason for hiding this comment

parkr commented Feb 21, 2015

parkr commented Feb 21, 2015

parkr Feb 21, 2015

Choose a reason for hiding this comment

willnorris Feb 21, 2015

Choose a reason for hiding this comment

willnorris commented Feb 21, 2015

willnorris commented Feb 22, 2015

parkr commented Feb 22, 2015

parkr commented Feb 22, 2015

willnorris commented Feb 22, 2015

parkr commented Feb 25, 2015

pathawks commented Feb 25, 2015

parkr commented Feb 25, 2015

willnorris commented Feb 25, 2015

parkr commented Feb 25, 2015

parkr Feb 25, 2015

Choose a reason for hiding this comment

willnorris commented Mar 2, 2015

parkr commented Mar 2, 2015

willnorris commented Mar 2, 2015

parkr commented Mar 2, 2015

parkr commented Mar 2, 2015

pathawks commented Mar 2, 2015

willnorris commented Mar 3, 2015

parkr commented Mar 3, 2015

parkr commented Mar 3, 2015

willnorris commented Mar 3, 2015

pathawks commented Mar 3, 2015

parkr commented Mar 3, 2015

alfredxing commented Mar 3, 2015

willnorris commented Mar 3, 2015

alfredxing commented Mar 3, 2015

snickers0129 commented Apr 27, 2015

willnorris commented Apr 27, 2015

willnorris commented Apr 27, 2015

envygeeks commented Apr 27, 2015

snickers0129 commented Apr 27, 2015

envygeeks commented Apr 27, 2015