cfpb / sheer

A tool for loading arbitrary content into Elasticsearch and serving that content on the web.
Creative Commons Zero v1.0 Universal
29 stars 23 forks source link

Permalinks should depend on url, not type field in looksups.json #87

Closed anselmbradford closed 7 years ago

anselmbradford commented 9 years ago

If in _settings/lookups.json there is more than one entry that uses the same type value, the permalinks will link to the path for the last entry. This behavior seems wrong, it seems like the permalinks should only care about the url value, not the type value.

For example, given the following lookup info:

"post": {
    "url":       "/blog/<id>/",
    "type":      "posts",
    "permalink": true
  },
  "event": {
    "url":       "/events/<id>/",
    "type":      "posts",
    "permalink": true
  }

The permalinks in, e.g., http://localhost:7000/blog/ will redirect to an http://localhost:7000/events/<id>/ URL, since that shares the same posts value in type.

rosskarchner commented 9 years ago

I'm not sure I get what you're saying-- the permalink is how an item "knows" about it's canonical URL, and they type is part of that lookup

It seems like, for event, 'permalink' should be false.

anselmbradford commented 9 years ago

Okay, maybe I'm misunderstanding the behavior here.

In regard to permalinks this is a summary of my understanding of how it works: Say you have a post that has the database ID of 801 or whatever, and you have a setup where the post can be accessed through its ID in the URL, like http://example.com/blog/801. If backend content is migrated to a new system the ID might change but the content would stay the same. So I thought the permalink was to prettify the URL for SEO purposes and make it based on the content so it was less likely to become a dead URL if the data was migrated somewhere. So for a "A new post" blog post the URL might be http://example.com/blog/a-new-post.

For canonical URLs, this is my understanding: For duplicate content there should be one URL designated as the original. This is done with the canonical URL link element in the head of the page at a particular URL, which if the content is repeated at several different URLs, the canonical link in the head all point to the same URL.

However, where I'm confused is the permalink seems different to me than a canonical URL. What I want to do is have the permalink of "A new post" at the /blog/ URL be http://example.com/blog/a-new-post and the permalink of the same post at the /events/ URL be http://example.com/events/a-new-post. It's fine to say the canonical URL for a particular post is e.g. http://example.com/blog/a-new-post, but I still want the two different URLs available when querying the posts at different URLs.

Where this is coming up is while working on cfgov-refresh, we have https://github.com/cfpb/cfgov-refresh/blob/sprint-21/_settings/lookups.json#L27-L31, which is making all posts listed on the blog at /blog/ go to an /events/ URL when clicked, so inside the loop that populates (pseudocode) for post in posts, the permalink they are picking up is at the /events/ URL when it should be the /blog/ URL.

Scotchester commented 9 years ago

@anselmbradford Shouldn't the type in the event lookup be event?

anselmbradford commented 9 years ago

@Scotchester Yes, it should be, but we don't have the backend events type available yet, so the FEWDs wanted to just feed the blog posts into the /events/ URL so we could design the landing page events list in the meantime.

Scotchester commented 9 years ago

Ah, right.

@rosskarchner can speak better to this, but I think that the point of the type field might that the <id> is really the slug, and those are not unique outside of a given WordPress post type. (And not required by WordPress to be unique at all for hierarchical post types, like Pages.)

So I think what would have to happen is that instead of populating the permalink property by using the type setting here, it'd have to check the first level in the URL of the page you're visiting and translate the permalink accordingly.

Does that make any sense? Disclaimer: I don't have a world-class understanding of Sheer :)

anselmbradford commented 9 years ago

So I think what would have to happen is that instead of populating the permalink property by using the type setting here, it'd have to check the first level in the URL of the page you're visiting and translate the permalink accordingly.

You're saying this is something that Sheer would need to be updated to do?

rosskarchner commented 9 years ago

Hmmm-- my thinking when I wrote that, is that an item has one and only one permalink, which is the same as a "canonical URL".

Once possible adjustment is to abolish the special handling for permalinks, and instead just have named URL patterns, scoped by type.

The JSON might look like:

"post": {
    "url":       "/blog/<id>/",
    "type":      "posts",
    "name": "permalink"
  },
  "event": {
    "url":       "/events/<id>/",
    "type":      "posts",
    "name": "events"
  }

And, in template-land, you can reference a particular link pattern like 'post.links.permalink' or 'post.links.events'

It's interesting to ponder what might be the relationship between the array keys and the link name. Or maybe a behavior like, a URL pattern without a name is treated like the default.

anselmbradford commented 9 years ago

a URL pattern without a name is treated like the default.

Yeah, my initial knee-jerk thought with the JSON you showed is whether you'd need the name entry at all or if you could just pull that from the URL, though that gets messy if you have URLs like /events/archive/<id>/. Also, did you mean to have "name": "permalink" in the post section in the JSON, or should that be "name": "blog"?

rosskarchner commented 9 years ago

the name would just a label (could be anything) for referencing a particular URL pattern.

The JSON you posted above has two different URL patterns for 'post' objects, but (under the current version of Sheer) only one of those can be referred to at render-time (the one with permalink=true).

You were proposing that we eliminate the 'permalink' flag, and the system could automatically pick a pattern, if one made sense in the current URL context (say, /events/). To me, that seems a shade too smart, though I could probably get past it.

My proposal goes in the other direction (more "dumb", or at least "explicit"), it eliminates any logic to automatically determine which URL you want, and instead asks template authors to refer to a URL pattern by name.

rosskarchner commented 9 years ago

With the part about leaving "name" out of a URL definition, I was thinking that would basically imply a name of "default" (or "permalink" or "canonical", or whatever we decide the default name is)