Skip to content
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

Misc permalinks adjustments #11185

Merged
merged 1 commit into from
Jun 29, 2023
Merged

Misc permalinks adjustments #11185

merged 1 commit into from
Jun 29, 2023

Conversation

bep
Copy link
Member

@bep bep commented Jun 28, 2023

Fixes #9448
Fixes #11184
See #8523

@bep bep force-pushed the misc/permalinks branch 2 times, most recently from a7f5957 to e1992b6 Compare June 28, 2023 14:12
// In Hugo 0.115.0 we expanded permalink configuration support to also include sections, taxonomies and terms.
// For sections not backed by a content file, the title below will be auto generated,
// but it will make the URL symetric with the content pages.
// A permalink config entry would possibly then look like :sections[:last]/:title.
return l.urlize(p.Title()), nil
Copy link
Member Author

@bep bep Jun 28, 2023

Choose a reason for hiding this comment

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

@jmooring the only functional change in this PR is the above. My take is that if people use :title in their permalinks config for sections, they mean it. I guess skipping this for branch pages was motivated by not repeating the section name, but then it's not possible to use the front matter title in the URLs and it feels a little bit too magic.

A permalink config for sections that would make sense is :sections[:last]/:title.

Agree?

Copy link
Member

Choose a reason for hiding this comment

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

Testing with hugo v0.115.0-DEV-e1992b66...

Given this content structure:

content/
├── books/
│   └── fiction/
│       ├── 2023/
│       │   ├── book-1.md
│       │   └── _index.md
│       └── _index.md
└── _index.md

And to produce this:

public/
├── libros/
│   ├── fiction/
│   │   ├── 2023/
│   │   │   ├── book-1/
│   │   │   │   └── index.html
│   │   │   └── index.html
│   │   └── index.html
│   └── index.html
└── index.html

I can do this (it works):

[permalinks.page]
books = '/libros/:sections[1:]/:title'

[permalinks.section]
books = '/libros/:sections[1:]'

If I understand your previous comment, I should also be able to do this:

[[permalinks.page]
books = '/libros/:sections[1:]/:title'

[permalinks.section]
books = '/libros/:sections[1:last]/:title'

But it panics with panic: runtime error: slice bounds out of range [1:0]. I am using :last incorrectly in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

But it panics with panic: runtime error: slice bounds out of range [1:0]. I am using :last incorrectly in this case?

The above looks like an (odd) bug. There should be no "slice bounds out of range" with these constructs, but I have obviously a hole in my test coverage. I will fix that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jmooring I have pushed a commit that fixes your example.

Copy link
Member

Choose a reason for hiding this comment

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

hugo v0.115.0-DEV-e1992b6 still panics with panic: runtime error: slice bounds out of range [1:0]. Same config...

[permalinks.page]
books = '/libros/:sections[1:]/:title'

[permalinks.section]
books = '/libros/:sections[1:last]/:title'

Copy link
Member

Choose a reason for hiding this comment

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

Testing with hugo v0.115.0-DEV-5c0216ed

No panic, but it is creating an index page that I don't expect.

config

[permalinks.page]
books = '/libros/:sections[1:]/:title'

# Works as expected
# [permalinks.section]
# books = '/libros/:sections[1:]'

# Does not work as I expect
[permalinks.section]
books = '/libros/:sections[1:last]/:title'

published

public/
├── libros/
│   ├── books/       <-- this section should not exist
│   │   └── index.html 
│   └── fiction/
│       ├── 2023/
│       │   ├── book-1/
│       │   │   └── index.html
│       │   └── index.html
│       └── index.html
└── index.html

Test site if you need it:

git clone --single-branch -b hugo-github-issue-11185 https://github.com/jmooring/hugo-testing hugo-github-issue-11185
cd hugo-github-issue-11185
rm -rf public/ && hugo && tree public

Copy link
Member Author

@bep bep Jun 28, 2023

Choose a reason for hiding this comment

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

books/ <-- this section should not exist

Well, it is how you have configured your permalinks :-) But I see your point, I will think about it. But if you had said:

[permalinks.section]
books = '/libros/:sections[1:]'

You would get closer. It's the :title that creates your "unwanted section".

But I need to think about this a little ...

Copy link
Member Author

@bep bep Jun 28, 2023

Choose a reason for hiding this comment

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

@jmooring I have thought a little, and I haven't found a simpler solution than expanding the syntax a little:

[permalinks.section]
books = '/libros/:sections[1:last]/:slug|:sections[last]'

This would make both of these the same:

[permalinks.section]
books = '/:slugorfilename'
[permalinks.section]
books = '/:slug|:filename'

??

Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer to document what currently works:

[permalinks.page]
books = '/libros/:sections[1:]/:title'

[permalinks.section]
books = '/libros/:sections[1:]'

The OR pipe is interesting, but I can see it being misused, generating "why doesn't this work" questions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, lets hold off on that idea until later.

}
}
} else {
return nil, fmt.Errorf("permalinks configuratio not supported for kind %q, supported kinds are %v", k, permalinksKindsSuppurt)
Copy link
Contributor

@alexandear alexandear Jun 28, 2023

Choose a reason for hiding this comment

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

there is a typo in configuration

Suggested change
return nil, fmt.Errorf("permalinks configuratio not supported for kind %q, supported kinds are %v", k, permalinksKindsSuppurt)
return nil, fmt.Errorf("permalinks configuration not supported for kind %q, supported kinds are %v", k, permalinksKindsSuppurt)

// [permalinks]
// key = '...'

// To sucessfully be backward compatible, "default" patterns need to be set for both page and term
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// To sucessfully be backward compatible, "default" patterns need to be set for both page and term
// To successfully be backward compatible, "default" patterns need to be set for both page and term

@bep bep force-pushed the misc/permalinks branch 2 times, most recently from 30f19f8 to d25ce22 Compare June 29, 2023 07:08
* Move config loading to the page package
* Fix a lower bound panic for the `:sections` slice syntax.
* Always return the `:title`
* Add some permalinks integration tests
* Also see issues below

Fixes gohugoio#9448
Fixes gohugoio#11184
See gohugoio#8523
@bep bep merged commit 7917961 into gohugoio:master Jun 29, 2023
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants