-
-
Notifications
You must be signed in to change notification settings - Fork 548
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 on collection routes if taxonomy isn’t assigned to collection #10438
[5.x] Throw 404 on collection routes if taxonomy isn’t assigned to collection #10438
Conversation
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 |
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. |
I just want to clarify something here. Let's say you have an This PR is making |
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? |
That's what I'm saying. There's already a 404 if you haven't created the view. |
That's correct. I don't think I follow your question/thought/concern? |
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. |
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:
As of now, only the first condition applies. This PR also applies the second condition. |
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? |
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. |
Cool cool. All cleared up. Thanks! 😃 |
# Conflicts: # src/Taxonomies/Taxonomy.php # tests/Data/Taxonomies/ViewsTest.php
It gets assigned during the request.
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
andCollection 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 …/{collection-url}/{taxonomy-slug}
and the taxonomy hasn't been assigned to the collection/{collection-url}/{taxonomy slug}/{term-slug}
and the taxonomy hasn't been assigned to the collection