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

Allow custom file extensions if defined in permalink YAML front matter #4314

Merged
merged 2 commits into from Jan 10, 2016

Conversation

parkr
Copy link
Member

@parkr parkr commented Jan 5, 2016

Still a failing test. Fixes #4182 once it's passing.

What I'm trying to do is have output_ext return File.extname(doc.permalink) in the case that the doc has a permalink. This means that checking if the path ends with the output extension will always return true, thus skipping the addition.

I'm running into an issue where permalink = /slides/example-slide-7.php, but output_ext = .html, so it's being appended. If I print the document, the data hasn't been read, which is awfully strange. permalink = nil, actually, when we ask for the output_ext for the first time. Super weird.

@envygeeks, have a second to look at this? Super weird – why would @site.process in the tests not yield a fully-read document?

@parkr parkr added this to the 3.1 milestone Jan 5, 2016
@envygeeks
Copy link
Contributor

Your problem surfaces here: https://github.com/jekyll/jekyll/blob/master/lib/jekyll/cleaner.rb#L56. Cleaner messages Document#destination before Document#read is done, resulting in Document#permalink returning nil which leads to converters.first.output_ext being messaged with document.extname instead of File.extname with document.permalink in Renderer#output_ext which leads to Document@output_ext being cached with ".html"

@envygeeks
Copy link
Contributor

To add, the Document is actually being read and is fully populated. Document@output_ext just ended up being cached early leading to this order problem. Your source works, it's higher up the chain an order problem surfaces a bug.

@parkr
Copy link
Member Author

parkr commented Jan 10, 2016

Cleaner messages Document#destination before Document#read is done, resulting in Document#permalink returning nil which leads to converters.first.output_ext being messaged with document.extname instead of File.extname with document.permalink in Renderer#output_ext which leads to Document@output_ext being cached with ".html"

I think you're right, but I'm not sure why quite yet.

Consider Site#process:

    def process
      reset
      read
      generate
      render
      cleanup
      write
      print_stats
    end

cleanup is called after read, and read has to complete before cleanup can be called. cleanup calls Cleaner#cleanup! which calls Cleaner#obsolete_files which calls Cleaner#new_dirs which calls Cleaner#new_files, which calls, for each site document, Document#destination. So I'm not really sure what's up. Especially because the failing tests use Site#process...

Fixes race issue.
Will introduce perf issues, though...
@parkr parkr changed the title wip: allow custom extensions Allow custom file extensions if defined in permalink YAML front matter. Jan 10, 2016
@parkr parkr changed the title Allow custom file extensions if defined in permalink YAML front matter. Allow custom file extensions if defined in permalink YAML front matter Jan 10, 2016
@parkr
Copy link
Member Author

parkr commented Jan 10, 2016

Only test failures are the ones currently on master:

Failure:
TestKramdown#test_: kramdown when a custom highlighter is chosen should support legacy enable_coderay... for now.  [/home/travis/build/jekyll/jekyll/test/test_kramdown.rb:93]
Minitest::Assertion: Failed refutation, no message given
Failure:
TestKramdown#test_: kramdown when a custom highlighter is chosen should use the chosen highlighter if it's available.  [/home/travis/build/jekyll/jekyll/test/test_kramdown.rb:73]
Minitest::Assertion: Failed refutation, no message given

@parkr
Copy link
Member Author

parkr commented Jan 10, 2016

@jekyllbot: merge +bug

jekyllbot added a commit that referenced this pull request Jan 10, 2016
@jekyllbot jekyllbot merged commit 7355540 into master Jan 10, 2016
@jekyllbot jekyllbot deleted the allow-custom-php-extensions branch January 10, 2016 02:11
jekyllbot added a commit that referenced this pull request Jan 10, 2016
@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to output .php files with 3.x
4 participants