search

jbroadway / elefant

Elefant, the refreshingly simple PHP CMS and web framework.
http://www.elefantcms.com
MIT License
208 stars 39 forks source link

Patch for blog comments embedding #250

Closed techanon closed 9 years ago

techanon commented 9 years ago

This was a weird bug that took me a while to figure out that I had partially fixed it before. Apparently the embeds for comments (specifically the comments APP) was being sent the proper identifier value only part of the time. After scrubbing the code I noticed that index.php used the same method as postfeed.php, of which got fixed a few months ago. So I fixed that issue as well as removing the comments count link when the $post->full == true since that links to the same page (plus the count value doesn't auto update so it looks broken, lol).

I also removed the post title from the identifier URL. The reason this was done was because of the fact that the title is a non-essential part of the post's URL to the blog, but WAS for the comment embeds. So different comments would show up for the same post depending on whether the title was in the URL or not. This has been fixed.

(Feel free to ignore the extended field commits from a month ago, pushed them to the wrong branch lol)

jbroadway commented 9 years ago

Does removing the post title from the identifier URL change the permalink URL anywhere else?

I include the post title in the URL for SEO purposes, and it's a good idea too to make sure there's only one canonical URL for each post to prevent diluting SEO across two URLs for the same page.

techanon commented 9 years ago

Did a bit more looking around and fixed a couple permalinks. The update to the identifier comes from the issue that arises when someone goes to the url without the title in it or if the title changes so would the identifier prior to this update. All those variations would produce different comment embeds depending on the url instead of a single one for that blog post. I'm not sure of the extent that SEO would be affected outside of the proposed changes. All of the native links in the blog app would be the full url, while the identifier for the embedded comment will use the title-less one for consistency.

techanon commented 9 years ago

Fixed the points you brought up and updated an incorrect href to the fullurl value.

techanon commented 9 years ago

I've been looking around to make sure the URLs are updated and I haven't found any more needing fixed. Can this be merged or is there there anything else in this PR that I should fix up?

jbroadway commented 9 years ago

Sorry, I really gotta get on these quicker!