-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Permalink unit tests and frontend route improvements #4322
Comments
I'd like to take this on! |
@jillesme Cool, did you make a start? |
@ErisDS I haven't, really busy with work this week.. I plan on doing it this weekend! If it's a pressing issue and someone else can & needs do it before I can, feel free to take this from me! |
@jillesme nope not at all pressing was just saying hi 👍 |
#4445 should actually remove most of the complexity of this issue. Looking up a post by slug or id will return the correct URL and if it doesn't match the request path then we can 404. |
closes TryGhost#4322 - removes verifying "sections" of permalinks in favor of checking the url returned with the post - fixes unit tests to define post.url in mock post requests
Having looked around our current permalink implementation, I believe there are a number of small issues with the handling in the frontend.single controller, that will result in bugs once we add #1631.
I've added the
beginner
label to this because I believe that this issue should be a good fit for someone who is comfortable with server-side JS & unit testing, but is brand new to Ghost's codebase.Essentially, the problem is that the frontend.single route is a wildcard but a post must only ever be served from a single, valid permalink. The structure of the permalink is set in the database, and valid structures are defined in #1631.
The frontend controller needs to take the permalink setting, the URL from which it is being called and validate that all parts of the URL strictly match / are valid according to the permalink.
At the moment, there is validation for the date, and after #3858 is completed, there should also be validation for the author, but this needs to be refactored and added to, to validate that all items in the URL are complete and correct.
Take for example, the slightly weird, but perfectly valid permalink structure of
/blog/:slug/:author/:year/:id/
.The frontend controller should split the provided URL into 5 parts and then:
slug
ANDid
blog
) is exactly 'blog'published_at
fieldIf any of these steps fail, a 404 should be returned.
The point of this issue is to refactor this validation step so that the code is clear and easily readable, to add validation so that string sections are definitely checked, and to add thorough unit tests for the code.
It probably makes sense to do the lookup in the
frontend.single
controller, but split the checks out into a new method inserver/config/url.js
which verifies that a given URL is a valid URL for a given post. So very similar to theurlPathForPost
function,checkUrlPathForPost
would also take the post data and the url provided and return whether or not it is valid.This would drastically improve the code quality in
server/controllers/frontend.js
The text was updated successfully, but these errors were encountered: