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

Removed excess /posts/ part from blog-posts resources and fixed localization bug. #463

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sintro
Copy link
Contributor

@sintro sintro commented Jul 22, 2016

This is extended PR (included #462). With fix of localization bug.

@parndt
Copy link
Member

parndt commented Jul 26, 2016

@bricesanchez I'm happy with this, how about you?

@@ -1,7 +1,9 @@
Refinery::Core::Engine.routes.draw do
namespace :blog, :path => Refinery::Blog.page_url do
root :to => "posts#index"
resources :posts, :only => [:show]
resources :posts, path: '', :only => [:show] do
Copy link
Member

Choose a reason for hiding this comment

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

That's a breaking change. We should do a redirect from blog/posts/1 to blog/1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? What is the point of this 'posts' level of url? And what redirect are you talking about (what action)?

Copy link
Member

Choose a reason for hiding this comment

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

Was something like:
http://localhost:3000/blog/posts/blog-post-name
Now it is:
http://localhost:3000/blog/blog-post-name

If I've an existing blog indexed on google, all indexed posts will now point to a 404.

I suggest to add a permanent redirect from blog/posts/* to blog/*` in order to keep SEO index fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be
get "posts/:id", to: :show, on: :collection
is better?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I don't know what I was thinking. What's wrong with /blog/posts/:id and /blog/posts ?

Copy link
Member

@bricesanchez bricesanchez Jul 26, 2016

Choose a reason for hiding this comment

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

I'm ok if we want to use blog urls without posts level but i would also like to add a redirect to be backward compatible with old urls

Copy link
Contributor Author

@sintro sintro Jul 26, 2016

Choose a reason for hiding this comment

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

@parndt, such url structure (without posts) even appers in this routes set in https://github.com/refinery/refinerycms-blog/blob/master/config/routes.rb#L8 , when you are trying to create the comment to post with invalid fields, you get failed validations and now see the url like 'http://example.com/blog/post-1/comment'. If you try to refresh the page, you will get 404 error. But as for me, this structure is perfect, and 'posts' is unnecessary (even non-friendly) part here, because 'blog' plays the role of 'posts' in this case.
Actually, the same (path: ' ') done in Refinery's extenstions generator, https://github.com/refinery/refinerycms/blob/master/core/lib/generators/refinery/engine/templates/config/routes.rb.erb#L5 , because there is no sense in urls like 'example.com/events/events/1',

@@ -3,6 +3,7 @@
root :to => "posts#index"
resources :posts, path: '', :only => [:show] do
get 'comments', on: :member, to: :show
get 'posts/:id(/comments)', on: :collection, to: redirect("#{Refinery::Blog.page_url}/%{id}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way there will be the 301 redirect if someone tries to get the post from old uri

@bricesanchez bricesanchez modified the milestone: 3.0 Nov 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants