Merge pull request #1198 from mojombo/fix-pagination

Fix Pagination

features/pagination.feature

@@ -52,3 +52,31 @@ Feature: Site pagination
       | 2     | 1     | 5         |
       | 3     | 1     | 6         |
       | 4     | 1     | 7         |
+
+  Scenario Outline: Setting a custom pagination path without an index.html in it
+    Given I have a configuration file with:
+      | key           | value                          |
+      | paginate      | 1                              |
+      | paginate_path | /blog/page/:num                |
+      | permalink     | /blog/:year/:month/:day/:title |
+    And I have a blog directory
+    And I have an "blog/index.html" page that contains "{{ paginator.posts.size }}"
+    And I have an "index.html" page that contains "Don't pick me!"
+    And I have a _posts directory
+    And I have the following posts:
+      | title     | date       | layout  | content                                |
+      | Wargames  | 2009-03-27 | default | The only winning move is not to play.  |
+      | Wargames2 | 2009-04-27 | default | The only winning move is not to play2. |
+      | Wargames3 | 2009-05-27 | default | The only winning move is not to play3. |
+      | Wargames4 | 2009-06-27 | default | The only winning move is not to play4. |
+    When I run jekyll
+    Then the _site/blog/page/<exist> directory should exist
+    And the "_site/blog/page/<exist>/index.html" file should exist
+    And I should see "<posts>" in "_site/blog/page/<exist>/index.html"
+    And the "_site/blog/page/<not_exist>/index.html" file should not exist
+
+    Examples:
+      | exist | posts | not_exist |
+      | 2     | 1     | 5         |
+      | 3     | 1     | 6         |
+      | 4     | 1     | 7         |

features/step_definitions/jekyll_steps.rb

@@ -4,11 +4,6 @@ Before do
   Dir.chdir(TEST_DIR)
 end
 
-After do
-  Dir.chdir(File.expand_path("..", TEST_DIR))
-  FileUtils.rm_rf(TEST_DIR)
-end
-
 Given /^I have a blank site in "(.*)"$/ do |path|
   FileUtils.mkdir(path)
 end

lib/jekyll/configuration.rb

@@ -29,7 +29,7 @@ module Jekyll
       'baseurl'       => '/',
       'include'       => ['.htaccess'],
       'exclude'       => [],
-      'paginate_path' => 'page:num',
+      'paginate_path' => '/page:num',
 
       'markdown_ext'  => 'markdown,mkd,mkdn,md',
       'textile_ext'   => 'textile',

lib/jekyll/generators/pagination.rb

@@ -10,8 +10,13 @@ module Jekyll
       #
       # Returns nothing.
       def generate(site)
-        site.pages.dup.each do |page|
+        if Pager.pagination_enabled?(site)
-          paginate(site, page) if Pager.pagination_enabled?(site.config, page)
+          if template = template_page(site)
+            paginate(site, template)
+          else
+            Jekyll.logger.warn "Pagination:", "Pagination is enabled, but I couldn't find" +
+            "an index.html page to use as the pagination template. Skipping pagination."
+          end
         end
       end
 
@@ -33,11 +38,11 @@ module Jekyll
         all_posts = site.site_payload['site']['posts']
         pages = Pager.calculate_pages(all_posts, site.config['paginate'].to_i)
         (1..pages).each do |num_page|
-          pager = Pager.new(site.config, num_page, all_posts, pages)
+          pager = Pager.new(site, num_page, all_posts, pages)
           if num_page > 1
             newpage = Page.new(site, site.source, page.dir, page.name)
             newpage.pager = pager
-            newpage.dir = File.join(page.dir, Pager.paginate_path(site.config, num_page))
+            newpage.dir = Pager.paginate_path(site, num_page)
             site.pages << newpage
           else
             page.pager = pager
@@ -45,6 +50,32 @@ module Jekyll
         end
       end
 
+      # Static: Fetch the URL of the template page. Used to determine the
+      #         path to the first pager in the series.
+      #
+      # site - the Jekyll::Site object
+      #
+      # Returns the url of the template page
+      def self.first_page_url(site)
+        if page = Pagination.new.template_page(site)
+          page.url
+        else
+          nil
+        end
+      end
+
+      # Public: Find the Jekyll::Page which will act as the pager template
+      #
+      # site - the Jekyll::Site object
+      #
+      # Returns the Jekyll::Page which will act as the pager template
+      def template_page(site)
+        site.pages.dup.select do |page|
+          Pager.pagination_candidate?(site.config, page)
+        end.sort do |one, two|
+          two.path.size <=> one.path.size
+        end.first
+      end
     end
   end
 
@@ -62,39 +93,78 @@ module Jekyll
       (all_posts.size.to_f / per_page.to_i).ceil
     end
 
-    # Determine if pagination is enabled for a given file.
+    # Determine if pagination is enabled the site.
     #
-    # config - The configuration Hash.
+    # site - the Jekyll::Site object
-    # page   - The Jekyll::Page with which to paginate
     #
     # Returns true if pagination is enabled, false otherwise.
-    def self.pagination_enabled?(config, page)
+    def self.pagination_enabled?(site)
-     !config['paginate'].nil? &&
+     !site.config['paginate'].nil? &&
+       site.pages.size > 0
+    end
+
+    # Static: Determine if a page is a possible candidate to be a template page.
+    #         Page's name must be `index.html` and exist in any of the directories
+    #         between the site source and `paginate_path`.
+    #
+    # config - the site configuration hash
+    # page   - the Jekyll::Page about which we're inquiring
+    #
+    # Returns true if the
+    def self.pagination_candidate?(config, page)
+      page_dir = File.dirname(File.expand_path(remove_leading_slash(page.path), config['source']))
+      paginate_path = remove_leading_slash(config['paginate_path'])
+      paginate_path = File.expand_path(paginate_path, config['source'])
       page.name == 'index.html' &&
-        subdirectories_identical(config['paginate_path'], page.dir)
+        in_hierarchy(config['source'], page_dir, File.dirname(paginate_path))
     end
 
     # Determine if the subdirectories of the two paths are the same relative to source
     #
-    # paginate_path - the paginate_path configuration setting
+    # source        - the site source
     # page_dir      - the directory of the Jekyll::Page
+    # paginate_path - the absolute paginate path (from root of FS)
     #
     # Returns whether the subdirectories are the same relative to source
-    def self.subdirectories_identical(paginate_path, page_dir)
+    def self.in_hierarchy(source, page_dir, paginate_path)
-      File.dirname(paginate_path).gsub(/\A\./, '') == page_dir.gsub(/\/\z/, '')
+      return false if paginate_path == File.dirname(paginate_path)
+      return false if paginate_path == Pathname.new(source).parent
+      page_dir == paginate_path ||
+        in_hierarchy(source, page_dir, File.dirname(paginate_path))
     end
 
     # Static: Return the pagination path of the page
     #
-    # site_config - the site config
+    # site     - the Jekyll::Site object
     # num_page - the pagination page number
     #
     # Returns the pagination path as a string
-    def self.paginate_path(site_config, num_page)
+    def self.paginate_path(site, num_page)
-      return nil if num_page.nil? || num_page <= 1
+      return nil if num_page.nil?
-      format = site_config['paginate_path']
+      return Generators::Pagination.first_page_url(site) if num_page <= 1
+      format = site.config['paginate_path']
       format = format.sub(':num', num_page.to_s)
-      File.basename(format)
+      ensure_leading_slash(format)
+    end
+
+    # Static: Return a String version of the input which has a leading slash.
+    #         If the input already has a forward slash in position zero, it will be
+    #         returned unchanged.
+    #
+    # path - a String path
+    #
+    # Returns the path with a leading slash
+    def self.ensure_leading_slash(path)
+      path[0..0] == "/" ? path : "/#{path}"
+    end
+
+    # Static: Return a String version of the input without a leading slash.
+    #
+    # path - a String path
+    #
+    # Returns the input without the leading slash
+    def self.remove_leading_slash(path)
+      ensure_leading_slash(path)[1..-1]
     end
 
     # Initialize a new Pager.
@@ -104,9 +174,9 @@ module Jekyll
     # all_posts - The Array of all the site's Posts.
     # num_pages - The Integer number of pages or nil if you'd like the number
     #             of pages calculated.
-    def initialize(config, page, all_posts, num_pages = nil)
+    def initialize(site, page, all_posts, num_pages = nil)
       @page = page
-      @per_page = config['paginate'].to_i
+      @per_page = site.config['paginate'].to_i
       @total_pages = num_pages || Pager.calculate_pages(all_posts, @per_page)
 
       if @page > @total_pages
@@ -119,9 +189,9 @@ module Jekyll
       @total_posts = all_posts.size
       @posts = all_posts[init..offset]
       @previous_page = @page != 1 ? @page - 1 : nil
-      @previous_page_path = Pager.paginate_path(config, @previous_page)
+      @previous_page_path = Pager.paginate_path(site, @previous_page)
       @next_page = @page != @total_pages ? @page + 1 : nil
-      @next_page_path = Pager.paginate_path(config, @next_page)
+      @next_page_path = Pager.paginate_path(site, @next_page)
     end
 
     # Convert this Pager's data to a Hash suitable for use by Liquid.

test/test_pager.rb

@@ -2,6 +2,17 @@ require 'helper'
 
 class TestPager < Test::Unit::TestCase
 
+  def build_site(config = {})
+    base = Jekyll::Configuration::DEFAULTS.deep_merge({
+      'source'      => source_dir,
+      'destination' => dest_dir,
+      'paginate'    => 1
+    })
+    site = Jekyll::Site.new(base.deep_merge(config))
+    site.process
+    site
+  end
+
   should "calculate number of pages" do
     assert_equal(0, Pager.calculate_pages([], '2'))
     assert_equal(1, Pager.calculate_pages([1], '2'))
@@ -12,49 +23,32 @@ class TestPager < Test::Unit::TestCase
   end
 
   should "determine the pagination path" do
-    assert_nil(Pager.paginate_path(Jekyll::Configuration::DEFAULTS, 1))
+    assert_equal("/index.html",  Pager.paginate_path(build_site, 1))
-    assert_equal("page2", Pager.paginate_path(Jekyll::Configuration::DEFAULTS, 2))
+    assert_equal("/page2",       Pager.paginate_path(build_site, 2))
-    assert_nil(Pager.paginate_path(Jekyll::Configuration::DEFAULTS.merge('paginate_path' => '/blog/page-:num'), 1))
+    assert_equal("/index.html",  Pager.paginate_path(build_site({'paginate_path' => '/blog/page-:num'}), 1))
-    assert_equal("page-2", Pager.paginate_path(Jekyll::Configuration::DEFAULTS.merge('paginate_path' => '/blog/page-:num'), 2))
+    assert_equal("/blog/page-2", Pager.paginate_path(build_site({'paginate_path' => '/blog/page-:num'}), 2))
+    assert_equal("/index.html",  Pager.paginate_path(build_site({'paginate_path' => '/blog/page/:num'}), 1))
+    assert_equal("/blog/page/2", Pager.paginate_path(build_site({'paginate_path' => '/blog/page/:num'}), 2))
+    assert_equal("/contacts/index.html", Pager.paginate_path(build_site({'paginate_path' => '/contacts/page:num'}), 1))
+    assert_equal("/contacts/page2",      Pager.paginate_path(build_site({'paginate_path' => '/contacts/page:num'}), 2))
+    assert_equal("/contacts/index.html", Pager.paginate_path(build_site({'paginate_path' => '/contacts/page/:num'}), 1))
+    assert_equal("/contacts/page/2",     Pager.paginate_path(build_site({'paginate_path' => '/contacts/page/:num'}), 2))
   end
 
   context "pagination disabled" do
-    setup do
-      stub(Jekyll).configuration do
-        Jekyll::Configuration::DEFAULTS.merge({
-          'source'      => source_dir,
-          'destination' => dest_dir
-        })
-      end
-      @config = Jekyll.configuration
-    end
-
     should "report that pagination is disabled" do
-      page = OpenStruct.new({ :name => 'index.html', :dir => '/' })
+      assert !Pager.pagination_enabled?(build_site('paginate' => nil))
-      assert !Pager.pagination_enabled?(@config, page)
     end
-
   end
 
   context "pagination enabled for 2" do
     setup do
-      stub(Jekyll).configuration do
+      @site  = build_site('paginate' => 2)
-        Jekyll::Configuration::DEFAULTS.merge({
-          'source'      => source_dir,
-          'destination' => dest_dir,
-          'paginate'    => 2
-        })
-      end
-
-      @config = Jekyll.configuration
-      @site = Site.new(@config)
-      @site.process
       @posts = @site.posts
     end
 
     should "report that pagination is enabled" do
-      page = OpenStruct.new({ :name => 'index.html', :dir => '/' })
+      assert Pager.pagination_enabled?(@site)
-      assert Pager.pagination_enabled?(@config, page)
     end
 
     context "with 4 posts" do
@@ -63,7 +57,7 @@ class TestPager < Test::Unit::TestCase
       end
 
       should "create first pager" do
-        pager = Pager.new(@config, 1, @posts)
+        pager = Pager.new(@site, 1, @posts)
         assert_equal(2, pager.posts.size)
         assert_equal(2, pager.total_pages)
         assert_nil(pager.previous_page)
@@ -71,7 +65,7 @@ class TestPager < Test::Unit::TestCase
       end
 
       should "create second pager" do
-        pager = Pager.new(@config, 2, @posts)
+        pager = Pager.new(@site, 2, @posts)
         assert_equal(2, pager.posts.size)
         assert_equal(2, pager.total_pages)
         assert_equal(1, pager.previous_page)
@@ -79,7 +73,7 @@ class TestPager < Test::Unit::TestCase
       end
 
       should "not create third pager" do
-        assert_raise(RuntimeError) { Pager.new(@config, 3, @posts) }
+        assert_raise(RuntimeError) { Pager.new(@site, 3, @posts) }
       end
 
     end
@@ -90,7 +84,7 @@ class TestPager < Test::Unit::TestCase
       end
 
       should "create first pager" do
-        pager = Pager.new(@config, 1, @posts)
+        pager = Pager.new(@site, 1, @posts)
         assert_equal(2, pager.posts.size)
         assert_equal(3, pager.total_pages)
         assert_nil(pager.previous_page)
@@ -98,7 +92,7 @@ class TestPager < Test::Unit::TestCase
       end
 
       should "create second pager" do
-        pager = Pager.new(@config, 2, @posts)
+        pager = Pager.new(@site, 2, @posts)
         assert_equal(2, pager.posts.size)
         assert_equal(3, pager.total_pages)
         assert_equal(1, pager.previous_page)
@@ -106,7 +100,7 @@ class TestPager < Test::Unit::TestCase
       end
 
       should "create third pager" do
-        pager = Pager.new(@config, 3, @posts)
+        pager = Pager.new(@site, 3, @posts)
         assert_equal(1, pager.posts.size)
         assert_equal(3, pager.total_pages)
         assert_equal(2, pager.previous_page)
@@ -114,7 +108,7 @@ class TestPager < Test::Unit::TestCase
       end
 
       should "not create fourth pager" do
-        assert_raise(RuntimeError) { Pager.new(@config, 4, @posts) }
+        assert_raise(RuntimeError) { Pager.new(@site, 4, @posts) }
       end
 
     end