Cleaned up discussion idWithSlug handling

[askvortsov1]

Fixes #1360

Changes proposed in this pull request:

• Moved all discussion idWithSlug generation logic to backend.

Reviewers should focus on:

• Should we still send the raw slug itself to the frontend?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

[franzliedke]

Because we now auto-format our JS code with Prettier, this branch now has conflicts with master. Sorry about that. 😉

Please take the steps outlined in the forum to resolve the conflicts.

[askvortsov1]

Keeping this logic on the backend could be useful in preparation for transliteration changes.

[tankerkiller125]

This works on my machine and it looks good. Will for sure help with Slug customization for communities that need it.

[dsevillamartin]

Seems like a good idea.

[askvortsov1]

There's one question I have before merging: If we use this as a base for transliteration drivers in discussion slugs, perhaps it should be called something different in the API ('fullSlug', perhaps?) As @franzliedke pointed out in #2049, the REST API is something that we are somewhat commited to support.

[tankerkiller125]

@askvortsov1 I'm happy with fullSlug

[clarkwinkelmann]

Here's a bit of nitpicking from me.

Otherwise like the idea 👍

You want to rename idWithSlug into fullSlug ? I think idWithSlug makes more sense here. It clearly says what it is, and also sounds like it would be a generated value.

[askvortsov1]

You want to rename idWithSlug into fullSlug ? I think idWithSlug makes more sense here. It clearly says what it is, and also sounds like it would be a generated value.

@clarkwinkelmann For this particular instance, idWithSlug makes more sense. But if we want to return one value for transliteration, naming it 'fullSlug' might be better, especially if a transliteration driver uses just the ID

[clarkwinkelmann]

I'm okay with the current version of this PR.

Regarding fullSlug, I feel like this implies the ID is not part of the content of the attribute. And then, the full slug is just the slug? Because the slug isn't the real canonical ID of a discussion, I would find odd to use an attribute named like that to build links. Having id in the name clearly implies we can use this in an URL and Flarum will be able to match it with the right discussion even if the slug changes down the road.

[askvortsov1]

@clarkwinkelmann But in the same way, if the transliteration strategy does not include a slug at all, wouldn't idWithSlug also be incorrect?

[tankerkiller125]

Another answer to the name of the object could be fullPath since technically we are setting the path.

[clarkwinkelmann]

@askvortsov1 I think this needs discussing in a new issue. Right now routing is fully ID based, and the ID must always be the first part of the URL. Slug is only there for SEO and is irrelevant to routing.

If you want to propose a change in how discussion routing works, a new issue would be best I think.

If we're only discussing names without a change in how it works, maybe one could be called the "minimal" URL (just id) and the other the "canonical" or "friendly" URL (id + slug) ? Maybe friendlyId ?

[askvortsov1]

Sorry, I don't think I'm wording this well. I recognize that this idWithSlug is only there for SEO, but if we want to have it vary depending on the transliteration scheme, imo we should call it something generic enough that its not tied to any given implementation. I apologize for all the bikeshedding here, I just think that when we choose a name that goes in core's public REST API, we should support it. As per https://en.wikipedia.org/wiki/Clean_URL, perhaps 'cleanId' (or 'friendlyId' as you suggest) could be the way to go here.

[clarkwinkelmann]

IMO, the word "id" needs to be in it, because the URL structure is /resources/<id>. I fear "path" might give the wrong impression that it could contain the full path and not just the last ID part.

So yeah, maybe one of friendlyId, cleanId, or maybe seoId ?

Even though the Wikipedia page makes it very clear what "clean URL" mean, I feel like it sounds like cleanId would be a shorter version of id, but in our case it's quite the opposite.

[askvortsov1]

@flarum/core Do we have a preference for friendlyId or seoId?

[tankerkiller125]

@askvortsov1 between those two I vote seoId

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[askvortsov1]

Superceded by #2456