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

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

Closed
ZiroKyl opened this Issue Oct 16, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@ZiroKyl

ZiroKyl commented Oct 16, 2017

  • I believe this to be a bug, not a question about using jekyll-redirect-from.
  • I updated to the latest Jekyll (or) if on GitHub Pages to the latest github-pages

  • I am on (or have tested on) Debian/Ubuntu GNU/Linux

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

This comment has been minimized.

ZiroKyl commented Oct 17, 2017

I taped 📼 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 -%}

@benbalter benbalter changed the title from Two posts, One redirect_from pathname, No errors, No warnings to Detect redirect from/to collisions and warn user at build time Oct 17, 2017

@pathawks

This comment has been minimized.

Member

pathawks commented Oct 17, 2017

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

This comment has been minimized.

Member

pathawks commented Oct 18, 2017

@benbalter

This comment has been minimized.

Contributor

benbalter commented Oct 18, 2017

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

@pathawks pathawks closed this Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment