metalsmith / permalinks

A Metalsmith plugin for permalinks.
MIT License
62 stars 67 forks source link

Front-matter `permalink` override not working #81

Closed lokesh closed 5 years ago

lokesh commented 6 years ago

Describe the bug

Adding permalink to the front-matter is not overriding the permalink for the page as described in the Readme.md.

To Reproduce

Create a page with the following front-matter:

---
title: "Hello Metalsmiths"
permalink: "test"
---

Set up build process:

var permalinks = require('metalsmith-permalinks');
..

var siteBuild = metalsmith(__dirname)
  .use(permalinks({
    'pattern': ':title'
  }))
  ...

Expected behaviour The page should have a permalink of /test. Instead it has a permalink of /hello-metalsmiths

Additional context It looks like the code that enabled this feature was removed from master. The original feature PR.

The feature works when the code from the PR above is added back to lib/index.js:

if(data.hasOwnProperty('permalink')) {
    path = data['permalink'];
}
Ajedi32 commented 6 years ago

That code is on master: https://github.com/leolabs/metalsmith-permalinks/blob/master/lib/index.js#L81-L83

Are you using the version from master? Or are you using the latest stable release?

doublejosh commented 6 years ago

The permalink front-matter is a boolean value, eg...

---
title: "Hello Metalsmiths"
permalink: true
---

The URL is determined by the filename, just like all other pages.

Please close this issue.

lokesh commented 6 years ago

@Ajedi32 - The code does seem to be in master. My mistake and sorry for the confusion.

Closing issue. I'll evaluate my local env issues.

@doublejosh The readme.md example doesn't show it as a Boolean: https://github.com/segmentio/metalsmith-permalinks/#overriding-the-permalink-for-a-file

doublejosh commented 6 years ago

Huh, you're right. The docs do show it as both boolean (using the false case to exclude) AND a string to explicitly name the desired path. I had only used the boolean case.

Good luck, please let us know what you find.

JonathanReeve commented 5 years ago

As far as I can tell, this bug is still valid, and should be reopened. @Ajedi32, the code you linked to seems to be on a different fork. Unless I'm missing something, that code isn't in master, at the moment.

Ajedi32 commented 5 years ago

It definitely was on master. Here's the commit: d5f50700e9c61d8823c45560629b00eb5c0af49b Merged with PR #65.

That code seems to have been removed since then though, in deaa8b9ae00dece46bd377b7c8398a50b1e2163b.

Ajedi32 commented 5 years ago

Root cause seems to be that the code was removed when #69 was rebased, and nobody noticed because there were no tests for that feature. To prevent this from happening again, any future fixes for this issue should include tests.