Push redirect logic to the redirect page model #131

benbalter · 2016-12-22T22:32:24Z

I did exactly what you're not supposed to do, but I did it, so I'm hoping we could still use it.

I went to tackle #125, but realized the way things were architechted, it'd be a bit tricky to do, since so much of the redirect logic was in the generator model, which was procedural and already large.

This all began when I opened Atom, and Rubocop started complaining about method length and complexity. So I started to add Rubocop, but in order to get Generator#render small enough, I needed to keep pushing logic into smaller methods, and it felt very un-OO to be passing the document/page around.

Then I thought, that instead, the RedirectPage should know all the things it needs to know to build itself. That's how object-oriented programming is supposed to work! So I began to pull the redirect logic out of the generator (which should concern itself with looping through things), and pushed it into the RedirectPage model.

That's all a long way to say that this embarrassingly large PR does a few things, while (theoretically) maintaining the existing functionality:

Add Rubocop, and ensure Rubocop compliance with Jekyll coding standards/style
Require Jekyll >= 3.0 (since we're on 3.3 now, and remove a lot of 2.x logic)
Push the redirect_from and redirect_to YAML parsing logic into the Page and Document models directly (with #redirect_from and redirect_to helper methods that return consistent output)
Moved the template into a standard layout and render it, rather than string interpolate it
Sites can now override the layout by simply adding a _layout/redirect.html to their site
By stubbing Page#read_yaml, the RedirectPage uses the standard Page#initialize method (less custom code)
Use Jekyll's native absolute_url filter to generate canonical URLs
Use Jekyll's native permalink/output_ext logic, rather than rolling our own for redirect_to's
Removed all the is_*? and has_*? helpers from the generator
Tests no longer build and write the entire site between each example

From a developer benefit, the big benefit here is adherence to Jekyll style (including method length and complexity), and a more object-oriented architecture.

From an end-user perspective, the benefit is relying more heavily on Jekyll internals, meaning theoretically, more consistent behavior, and new features going forward, in addition to custom template support.

From a going forward perspective, #125 can now be as simple as site.pages.map { |p| p.data["redirect"] }.compact.to_json.

I believe @parkr's 🚗, but /cc @pathawks for 👀?

Fixes #128, #130

I think it should also fix #129 and #114.

parkr · 2016-12-22T23:45:48Z

For me, as long as all the pre-existing integration-level tests pass without alteration (i.e. it still generates all the redirect_from and redirect_to pages as it did previously), then I'm happy with whatever architecture is easiest to maintain. I can't take a close look at this now, but I'll try to get to it soon. I gladly defer to @pathawks's judgement here as well, so if he thinks it's 👍 then merge and release!

pathawks · 2016-12-23T01:24:17Z

.travis.yml

@@ -9,18 +9,11 @@ matrix:
    - # GitHub Pages
      rvm: 2.1.1


Note to self that this should be bumped to 2.3.3 at some point.

benbalter · 2016-12-23T16:36:47Z

as long as all the pre-existing integration-level tests pass without alteration

Explicitly, there are two integration tests, which I expanded on.

There are also a handful of integration-ish specs (does this file exist?) in the redirector spec, which I believe I ported over,

pathawks

Let's separate the extensionless HTML files discussion into a separate PR and for now continue the previous behavior.

Otherwise, the only change in behavior I can find is that in one particular circumstance, the previous version would output a relative URL where this outputs an absolute URL.

Because in other circumstances the previous version would output an absolute URL, I am willing to call this a bug in the old code and say the _new_behavior is correct.

pathawks · 2016-12-23T21:31:00Z

lib/jekyll-redirect-from/redirectable.rb

+    # to which the document should be redirected
+    def redirect_to
+      if to_liquid["redirect_to"].is_a?(Array)
+        to_liquid["redirect_to"].compact.first


I see this is the current behavior, but something has gone wrong if this is an array, right?

We can save that for later.

Correct. redirect_to can be an array, an we absorb the complexity of YAML and just deal with it. I'm fine with that, as many users think that array-style markup with a single entry is the proper way to do things (and I'd rather add a few method calls then break their site).

pathawks · 2016-12-23T21:57:00Z

lib/jekyll-redirect-from/redirect.html

+<!DOCTYPE html>
+<html lang="en-US">
+  <meta charset="utf-8">
+  <title>Redirecting...</title>


Can we change these back to …?

Not strongly opinionated, but what's the value?

We mean to use an ellipsis. We can use an ellipsis. We should use an ellipsis.

#93

pathawks · 2016-12-23T22:09:18Z

lib/jekyll-redirect-from/redirect_page.rb

+    # Overwrite the default read_yaml method since the file doesn't exist
+    def read_yaml(_base, _name, _opts = {})
+      self.content = ""
+      self.data ||= DEFAULT_DATA.dup


Why ||=? It seems like it should either be just an assignment or a merge.

When would self.data already be set at this point?

Ahh. You are correct.

pathawks · 2016-12-23T22:10:08Z

lib/jekyll-redirect-from/redirect_page.rb

  class RedirectPage < Jekyll::Page
-    # Initialize a new RedirectPage.
+    # Use Jekyll's native absolute_url filter
+    include Jekyll::Filters::URLFilters


I really wish there were a better way to do this.

Maybe the relative URL logic should be broken into a utility method?

pathawks · 2016-12-23T22:20:34Z

lib/jekyll-redirect-from/redirect_page.rb


-      Jekyll::Hooks.trigger :pages, :post_init, self if JekyllRedirectFrom.jekyll_3?
+    # Redirects without a trailing slash should produce extentionless files


This is a regression.

Some servers will not serve extensionless files as text/html, which causes the browser to just download the binary file instead of loading it and following the redirect.

I agree, but was going off of what was in the README. What's the desired behavior here?

benbalter · 2016-12-23T22:42:04Z

Let's separate the extensionless HTML files discussion into a separate PR and for now continue the previous behavior.

Can you clarify? Although I personally disagree with it, I thought I was implementing the behavior described in the README, no? (or is that what you're suggesting?)

Because in other circumstances the previous version would output an absolute URL, I am willing to call this a bug in the old code and say the _new_behavior is correct.

That was my thinking, especially because there's a canonical URL field, that must be absolute. Also glad to expose relative and absolute URL fields, but I feel like that's overly complicated what should be a decision on our part.

pathawks · 2016-12-23T23:01:23Z

Can you clarify? Although I personally disagree with it, I thought I was implementing the behavior described in the README, no?

It seems the behavior changed for Jekyll 3.

Current tests check to make sure that files have extensions.

spec_helper.rb:

  def forced_output_ext
    JekyllRedirectFrom.jekyll_3? ? ".html" : ""
  end

redirector_spec.rb:

    it "generates the refresh page for the post properly" do
      expect(dest_dir("posts/23128432159832/mary-had-a-little-lamb#{forced_output_ext}")).to exist
    end

If output_ext is changed (removed) then 👍 from me

pathawks · 2016-12-24T04:35:13Z

I thought I was implementing the behavior described in the README

If you compare the fixture site built with old code vs this code, you will see the difference in behavior.

benbalter · 2016-12-27T14:16:16Z

you will see the difference in behavior.

So /foo should be rendered as /foo.html or /foo?

pathawks · 2016-12-28T04:31:07Z

According to the fixture site:

---
redirect_from: /articles/23128432159832/mary-had-a-little-lamb
---

is rendered as /articles/23128432159832/mary-had-a-little-lamb.html

That is the current behavior as of v0.11.0.

benbalter · 2016-12-29T14:50:34Z

Here are the relevant tests:

Just want to confirm before I implement the change, this line from the README:

Redirects including a trailing slash will generate a corresponding subdirectory containing an index.html, while redirects without a trailing slash will generate a corresponding filename without an extension, and without a subdirectory.

Is not true? If we're not implementing the documented behavior, is that a bug or are the docs just out of date?

Basically, it sounds like the desired behavior is that a redirect from without an extension should be given a .html extension, correct?

pathawks · 2016-12-29T19:19:01Z

Yes, it sounds like the docs and the code are in conflict.

Relevant PR: #96

benbalter · 2016-12-30T19:08:28Z

@pathawks take a look at 124f85a, which should now implement that logic (always append .html to extensionless redirects), such that redirect_from: /articles/23128432159832/mary-had-a-little-lamb renders as /articles/23128432159832/mary-had-a-little-lamb.html as tested in

jekyll-redirect-from/spec/integrations_spec.rb

Lines 51 to 56 in 124f85a

    
           let(:relative_path) { "articles/23128432159832/mary-had-a-little-lamb.html" } 
        
           it "exists in the built site" do 
        
             expect(path).to exist 
        
             expect(contents).to match("http://jekyllrb.com/articles/redirect-me-plz.html") 
        
           end

.

benbalter · 2016-12-30T19:36:39Z

@pathawks, thanks, as always, for the critical 👀.

pathawks · 2016-12-30T19:44:15Z

jekyll-redirect-from.gemspec

@@ -18,10 +18,11 @@ Gem::Specification.new do |spec|
  spec.test_files    = spec.files.grep(%r{^(test|spec|features)/})
  spec.require_paths = ["lib"]

-  spec.add_runtime_dependency "jekyll", ">= 2.0"
+  spec.add_runtime_dependency "jekyll", "~> 3.0"


3.3 since we're using URL filters

pathawks · 2016-12-30T19:50:18Z

Thank you for all of the work here @benbalter

This will make the plugin much more robust 👍

benbalter · 2016-12-30T20:41:10Z

@jekyllbot: merge +major.

parkr · 2017-04-29T16:24:57Z

Can we make a note in the release notes about respecting a custom redirect.html layout? There is no mention of it in the release notes and there are several prior releases which explicitly removed the layouts for this file.

Thanks!

benbalter · 2017-05-01T16:57:47Z

Can we make a note in the release notes about respecting a custom redirect.html layout? There is no mention of it in the release notes

https://github.com/jekyll/jekyll-redirect-from/releases/tag/v0.12.0 had "Support for custom redirect templates" as the first major change. Updated to "Use _layouts/redirect.html as the redirect template if present in the site source"

benbalter added 6 commits December 22, 2016 17:03

push redirect logic to the redirect page model

cb30dcf

jekyll core only checks against 2.1, 2.2, 2.3

816bd76

remove custom gemset logic

a789795

add github pages logic back in

32c3ed6

try 2.3.0

0f42abe

enable line length enforcement

ee0af24

pathawks self-assigned this Dec 23, 2016

pathawks self-requested a review December 23, 2016 01:22

pathawks reviewed Dec 23, 2016

View reviewed changes

.travis.yml

@@ -9,18 +9,11 @@ matrix:

- # GitHub Pages

rvm: 2.1.1

Copy link

Member

pathawks Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self that this should be bumped to 2.3.3 at some point.

pathawks requested changes Dec 23, 2016

View reviewed changes

append .HTML to extensionlist redirects

124f85a

pathawks approved these changes Dec 30, 2016

View reviewed changes

use real elipsis

e368724

pathawks reviewed Dec 30, 2016

View reviewed changes

require 3.3 since we use URL filters

2c1e818

pathawks approved these changes Dec 30, 2016

View reviewed changes

jekyllbot merged commit f449475 into master Dec 30, 2016

jekyllbot added the feature label Dec 30, 2016

jekyllbot deleted the bb-refactor branch December 30, 2016 20:41

jekyllbot added a commit that referenced this pull request Dec 30, 2016

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

8f66476

benbalter mentioned this pull request Dec 30, 2016

redirect_to does not use baseurl #130

Closed

benbalter added a commit that referenced this pull request Dec 30, 2016

Update history with more detailed description of #131

6b077ec

This was referenced Dec 30, 2016

Redirect page is processed as Markdown #114

Closed

Drop filename when redirecting to index.html #129

Closed

Inherit Jekyll's rubocop config for consistency #117

Closed

jchapin mentioned this pull request Jan 12, 2017

Bump jekyll-redirect-from to 0.12.1 - Allows for a custom redirect.html template to be included in _layouts. github/pages-gem#387

Closed

benbalter mentioned this pull request Mar 15, 2017

Create redirects.json file #147

Merged

parkr mentioned this pull request Apr 26, 2017

Move custom redirect.html to a new filename to fix compatibility with jekyll-redirect-from github/government.github.com#562

Merged

jekyll locked and limited conversation to collaborators Apr 27, 2019

jekyllbot added the frozen-due-to-age label Apr 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push redirect logic to the redirect page model #131

Push redirect logic to the redirect page model #131

benbalter commented Dec 22, 2016

parkr commented Dec 22, 2016

pathawks Dec 23, 2016

benbalter commented Dec 23, 2016

pathawks left a comment

pathawks Dec 23, 2016

benbalter Dec 23, 2016

pathawks Dec 23, 2016

benbalter Dec 23, 2016

pathawks Dec 24, 2016

pathawks Dec 23, 2016

benbalter Dec 23, 2016

pathawks Dec 23, 2016

pathawks Dec 23, 2016

benbalter Dec 23, 2016

benbalter commented Dec 23, 2016

pathawks commented Dec 23, 2016 •

edited

Loading

pathawks commented Dec 24, 2016

benbalter commented Dec 27, 2016

pathawks commented Dec 28, 2016

benbalter commented Dec 29, 2016

pathawks commented Dec 29, 2016

benbalter commented Dec 30, 2016

benbalter commented Dec 30, 2016 •

edited

Loading

pathawks Dec 30, 2016

pathawks commented Dec 30, 2016

benbalter commented Dec 30, 2016

parkr commented Apr 29, 2017

benbalter commented May 1, 2017


		Jekyll::Hooks.trigger :pages, :post_init, self if JekyllRedirectFrom.jekyll_3?
		# Redirects without a trailing slash should produce extentionless files

Push redirect logic to the redirect page model #131

Push redirect logic to the redirect page model #131

Conversation

benbalter commented Dec 22, 2016

parkr commented Dec 22, 2016

Choose a reason for hiding this comment

benbalter commented Dec 23, 2016

pathawks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbalter commented Dec 23, 2016

pathawks commented Dec 23, 2016 • edited Loading

pathawks commented Dec 24, 2016

benbalter commented Dec 27, 2016

pathawks commented Dec 28, 2016

benbalter commented Dec 29, 2016

pathawks commented Dec 29, 2016

benbalter commented Dec 30, 2016

benbalter commented Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

pathawks commented Dec 30, 2016

benbalter commented Dec 30, 2016

parkr commented Apr 29, 2017

benbalter commented May 1, 2017

pathawks commented Dec 23, 2016 •

edited

Loading

benbalter commented Dec 30, 2016 •

edited

Loading