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

[5.x] Throw 404 if taxonomy isn’t assigned to collection #10438

Open
wants to merge 5 commits into
base: 5.x
Choose a base branch
from

Conversation

aerni
Copy link
Contributor

@aerni aerni commented Jul 11, 2024

The Taxonomy Routing docs state, that "For each taxonomy assigned to a collection you will also get these routes …", talking about the routes for Collection Taxonomy Details and Collection Term Details pages.

However, I've noticed that these routes were also available for taxonomies that haven't been assigned the collection in question. This PR fixes this issue by throwing a 404 if you're visiting a collection taxonomy or collection term route of a taxonomy that hasn't been assigned to the collection in question.

A 404 will be thrown if …

  • … you are visiting /{collection-url}/{taxonomy-slug} and the taxonomy hasn't been assigned to the collection
  • … you are visiting /{collection-url}/{taxonomy slug}/{term-slug} and the taxonomy hasn't been assigned to the collection

@aerni aerni changed the title Throw 404 if taxonomy isn’t assigned to collection [5.x] Throw 404 if taxonomy isn’t assigned to collection Jul 11, 2024
@aerni aerni marked this pull request as draft July 11, 2024 13:50
@aerni aerni marked this pull request as ready for review July 11, 2024 19:44
@aerni
Copy link
Contributor Author

aerni commented Jul 12, 2024

Additionally, I've come across a fundamental question that should be considered as well.

If you attach a taxonomy to a collection, but don't assign any term of that taxonomy to an entry of that collection, should the collection term routes throw a 404? Currently, they don't. So this would technically be a breaking change on the frontend. However, I think throwing a 404 is how it should be in the first place. Else, you'll have collection term routes with no entries to show.

@jasonvarga
Copy link
Member

I don't think it should 404.

Similar question to say if you have a blog listing page, should it 404 if there are no blog posts? I wouldn't think so.

If you want it to 404 you could do it in your template. {{ if !terms }}{{ 404 }}{{ /if }}

@jasonvarga
Copy link
Member

I just want to clarify something here.

Let's say you have an blog collection and a tags taxonomy. You get /blog/tags for free if you have a blog/tags/index.antlers.html.

This PR is making /blog/tags 404 if you haven't assigned tags to blog. That's fine, but... How are you in a situation where the view exists but you don't want the url to work?

@aerni
Copy link
Contributor Author

aerni commented Jul 19, 2024

Creating the views is something very conscious. You'd only create them if you'd want the automagic taxonomy routes. Else, there's no reason to have those views in the first place. What situation are you thinking of where the view exists, but you don't want the URL to work?

@jasonvarga
Copy link
Member

That's what I'm saying. There's already a 404 if you haven't created the view.

@aerni
Copy link
Contributor Author

aerni commented Jul 22, 2024

That's correct. I don't think I follow your question/thought/concern?

@jasonvarga
Copy link
Member

I have a feeling I'm going to be the one proved wrong here 😅 but, my point is that since you already get a 404 if the view is missing, why would you create the view if you don't intend to see something at the url?

If you don't want the URL, don't create the view.

@aerni
Copy link
Contributor Author

aerni commented Jul 22, 2024

Ah, gotcha. The reason for this PR was to make it work the way the docs say it should. For collection related taxonomy routes, the docs state two conditions:

  1. Taxonomy routes are automatically created for you if the corresponding view exists
  2. For each taxonomy assigned to a collection

As of now, only the first condition applies. This PR also applies the second condition.

CleanShot 2024-07-22 at 13 52 55@2x

@jasonvarga
Copy link
Member

Ok, fine. 😄 But is this actually a problem for you? Why do you create that view if you don't also intend to assign the taxonomy to the collection?

@aerni
Copy link
Contributor Author

aerni commented Jul 22, 2024

No, not a problem for me. I was just reading through the docs and was already on a roll fixing other taxonomy-related bugs. That being said, a scenario where I could imagine this PR coming in handy is when you're changing a collection's settings and forget about cleaning up the views.

@jasonvarga
Copy link
Member

Cool cool. All cleared up. Thanks! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants