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

Routing 404 errors to custom 404 page for development server #1899

Merged
merged 1 commit into from Jan 4, 2014

Conversation

Projects
None yet
5 participants
@soimort
Contributor

soimort commented Jan 1, 2014

Currently the development server of Jekyll (WEBrick) does not handle 404 errors properly, i.e., it displays only WEBrick's default 404 message, rather than user's custom 404 page.

People use their custom 404 pages configured in .htaccess, or just 404.html ( as GitHub Pages default). It would be nice if Jekyll's preview server is aware of these.

It's a monkey patch to add a new create_error_page method so that HTTPResponse used by HTTPServlet::FileHandler will call this method to replace its default error page. It reads the ErrorDocument 404 configuration in .htaccess first, defaults to /404.html (as used by GitHub Pages) then. While the custom 404 page does not exist, it simply serves WEBrick's default 404 message.

@parkr

View changes

lib/jekyll/commands/serve.rb Outdated
error_page = /^ErrorDocument\s+404\s+(.+)/.match(htaccess)[1]
rescue # use /404.html as default
error_page = '/404.html'
end

This comment has been minimized.

@parkr

parkr Jan 1, 2014

Member

What's the case for supporting .htaccess files? It is Apache-only, so it'd seem to me to be something we shouldn't support. If anything, we should mimic GitHub Pages behavior and serve a the 404.html page..

This comment has been minimized.

@soimort

soimort Jan 1, 2014

Contributor

I agree on mimicking GitHub Pages' 404.html. However, those who don't use GH Pages might want to customize other 404 pages, then it's not correct behavior.

Maybe adding an option in _config.yml would be better?

This comment has been minimized.

@parkr

parkr Jan 2, 2014

Member

I think if we do any custom 404 handling, it should be like GitHub Pages. Anything else will just cause confusion.

@soimort

This comment has been minimized.

Contributor

soimort commented Jan 2, 2014

I've made my change -- use only 404.html as the default 404 page, the same as GitHub Pages.

@parkr

This comment has been minimized.

Member

parkr commented Jan 2, 2014

Thank you!! LGTM. 👍

@benbalter, what do you think? I'm good to go.

Also @mattr-?

@benbalter

This comment has been minimized.

Contributor

benbalter commented Jan 2, 2014

My feedback would have been not to go the .htaccess route, as AFAIK webrick doesn't normally respect .htaccess, and if anything, a .htaccess-customizing user would either be sophisticated enough to be able to preview their 404, or we could just recommend they set their 404 to /404.html via .htaccess to minimize the surface area we have to maintain.

👍

@parkr

View changes

lib/jekyll/commands/serve.rb Outdated
if File.exists?(File.join(destination, error_page))
WEBrick::HTTPResponse.class_eval %Q"
def create_error_page
@body = IO.read(File.join('#{destination}', '#{error_page}'))

This comment has been minimized.

@parkr

parkr Jan 2, 2014

Member

Any reason you chose to use string interpolation here? It seems File.join(destination, error_page) should be just fine.

This comment has been minimized.

@soimort

soimort Jan 3, 2014

Contributor

@parkr It's a monkey patch to class WEBrick::HTTPResponse so destination and error_page are out of its scope. Using these variables won't work. However, I found this kind of interpolation to be a little unsafe (if someone wants to inject some code in their destination config). Now I have a better solution for that.

@parkr

This comment has been minimized.

Member

parkr commented Jan 3, 2014

A resounding 👍 to the updated version. Nice work, @soimort!

@ghost ghost assigned mattr- Jan 3, 2014

mattr- added a commit that referenced this pull request Jan 4, 2014

@mattr- mattr- merged commit a33e866 into jekyll:master Jan 4, 2014

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Jan 4, 2014

@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.