jekyll / jekyll-redirect-from

:twisted_rightwards_arrows: Seamlessly specify multiple redirections URLs for your pages and posts.
MIT License
780 stars 112 forks source link

Detect redirect from/to collisions and warn user at build time #164

Closed ZiroKyl closed 6 years ago

ZiroKyl commented 6 years ago

My Reproduction Steps

  1. The author wrote two posts:

File: _posts/2017-09-01-a-part1.html

---
title: 'A Part1'
author: Copy-Paste Man
redirect_from: 
  - /abc/1/
---

AND

File: _posts/2017-09-20-a-part2.html

---
title: 'A Part2'
author: Copy-Paste Man
redirect_from: 
  - /abc/1/
---
  1. Every time the author run: bundle exec jekyll serve

Expected Behavior

Get error or warning. Prefer error.

Actual Behavior

/abc/1/ redirect to 2017-09-20-a-part2.


P.S. unfortunately, permalink have the same problem:

/p1.html          --{permalink to}--> /abc/1/
/p2.html          --{permalink to}--> /abc/1/
/abc/1/index.html

Result: /_site/abc/1/index.html is /p2.html

ZiroKyl commented 6 years ago

I taped :vhs: some workaround to this problem.

Stuped Slow Workaround

Create file /checker.html:

---
empty_array: []
---
{%- assign pathnames = page.empty_array -%}

{%- for post in site.posts -%}
    {%- for pathname in post.redirect_from -%}
        {%- if pathnames contains pathname -%}
            Collision detected! Comment this line to print all collisions. {{ 0 | divided_by:0 }}
            `redirect_from` pathname "{{ pathname }}" in post "{{ post.path }}"
        {%- else -%}
            {%- assign pathnames = pathnames | push: pathname -%}
        {%- endif -%}
    {%- endfor -%}
{%- endfor -%}
{%- comment %} Additionally check site.pages, site.documents and site.static_files {% endcomment -%}

If checker.html detect collision you get error message:

Liquid Exception: Liquid error (line 7): divided by 0 in /checker.html
Error: Run jekyll build --trace for more information.

Pefomance testing

And I did some performance tests of array.add(string) implementations.

---
empty_array: []
new_line: "\n"
---
{% assign arr = page.empty_array %}
===================================

{% for i in (0..10000) %}
    {% assign str_arr = "123" | split: page.new_line %}
    {% assign arr     = str_arr | concat: arr %}
{% endfor %}

 VS

{% for i in (0..10000) %}
    {% assign str_arr = "123" | split: page.new_line %}
    {% assign arr     = arr | concat: str_arr %}
{% endfor %}

===================================

Fastest(x4): `str_arr | concat: arr`

===================================

{% for i in (0..10000) %}
    {% assign arr = "123" | split: page.new_line | concat: arr %}
{% endfor %}

 VS

{% for i in (0..10000) %}
    {% assign arr = arr | push: "123" %}
{% endfor %}

 VS

{% for i in (0..10000) %}
    {% assign arr = arr | unshift: "123" %}
{% endfor %}

===================================

Parity.

Update

Little Better Workaround

Add to your layout file for posts:

{%- for post in site.posts -%}
    {%- if post == page -%}
        {% break %}
    {%- endif -%}

    {%- for pathname in page.redirect_from -%}
        {%- if post.redirect_from contains pathname -%}
            Collision detected! Comment this line to print collision details. {{ 0 | divided_by:0 }}
            `redirect_from` "{{ pathname }}" has already been used in "{{ post.path }}"
        {%- endif -%}
    {%- endfor -%}
{%- endfor -%}
pathawks commented 6 years ago

In general, I don't think that Jekyll does any checking at all to make sure each URL is unique. I don't think this plugin should be a special case.

I certainly agree that something like this would be useful when debugging, and I can remember a couple error reports that would have benefitted from this check, but I think that any duplicate URL checks should be done by Jekyll itself rather than by individual plugins.

pathawks commented 6 years ago

https://github.com/jekyll/jekyll/issues/6207

benbalter commented 6 years ago

I'm in agreement with @pathawks and think this is better solved upstream in https://github.com/jekyll/jekyll/issues/6207 as conflicts are not specific to this plugin.