front controller path/routing adjustment

[asdfzdfj]

the new filter ui changes also introduces a couple more filtering parameters in the form of controller path params, for threads/entries listing it looks fine and relatively staightforward, but for microblog posts listing, what used to be a simple /microblog is now something like /all/hot/∞/all/all/microblog, which hinders the user experience on url readability and memorability compared to the previous scheme, for those who still care

this changes tries to adjust the path/routing of these front controller especially for the microblog part, to make it more simpler and could omit some default filter parameters if they aren't set, similar to the previous path scheme

note that this patch main focus is just the path routing, there's probably more to adjust/polish in front controller and related areas but those would likely be a separate follow up patch

---

some of the front path routing that I have in mind, based on on previous scheme:

• /{subscription}/{content}/{sortBy}/{time}/{type}/{federation} -- /sub/microblog/newest/1d
• /{subscription}/{sortBy}/{time}/{type}/{federation} -- /mod/commented/1w (*)
• /{content}/{sortBy}/{time}/{type}/{federation} -- /microblog/newest
• /{sortBy}/{time}/{type}/{federation} -- /top/6h (*)
• /m/{magazineName}/{content}/{sortBy}/{time}/{type}/{federation} -- /m/playground/microblog/active
• /m/{magazineName}/{sortBy}/{time}/{type}/{federation} -- /m/random/newest (*)

path components sortBy and after shouldn't showed up if using default filter values

paths marked with (*) could be later used for combined views default/short entry point when the time comes? right now they should be alias for when content param is threads i.e. threads/entries front view

type and federation may be able to be pulled out into query params, esp. type where it currently only make sense in the context of threads listing, as we don't do such categorization for microblog posts

[e-five256]

I agree with you that type should be pulled out because it's exclusive to threads so it being in the url for microblogs and not doing anything is confusing

[asdfzdfj]

already sends this to e-five but I figured it should be here too:

---

as for how things are currently:

• managed to get some front_* routes that should match all the url patterns I described, and thanks to route param requirements e-five added it should be able to match unambiguously
• currently live on my instance and so far, I'm quite happy about it (duh), could use some help to check that I didn't break stuff anywhere else and that the url navigated/generated made sense as a user (frankly I count this entire thing as more of a ux problem)

something that could use help/input:

• the new front controller introduces one front controller function/endpoint EntryFrontController::front that does the main job, and it also introduces EntryFrontController::front_redirect which seems to exist for compat reasons? so now I'm not sure on how to best map routes to controller endpoint
  • like should all of them mapped to main front but with default params acting as controller function partials or something
  • or only the full route (with all params) should get the main front endpoint, and then shim the rest with front_redirect endpoint
• there's also the route naming itself
  • from what I've seen the route name often encodes some context into it and there exist some naming convention in it, but with the new front controller names that I've given currently doesn't do that job too well
  • a more practical one would be to migrate the other front routes (post, magazine) into this front controller (incl. path generation, the route name context thing), but somehow this feels like more of an answer to "unifying front controller" question rather than "new front controller paths look like shit" question

[asdfzdfj]

current state as of writing, if that means anything:

• trying the "all front route use main front controller" method, will see how this goes but seems to work fine
• made a specialized front_options_url for main front filtering options, where it can "upgrade" the front route used in url generation, otherwise when generating url that uses {subscription} or {content} in front route that doesn't have them defined already will result in it being appended as query params (e.g. /microblog/newest?subscription=sub rather than /sub/microblog/newest)
• pulling out {type} is trickier than expected: deleting {type} from path params seem to make it not working consistenly, and looks like the type in defaults would also need to be removed otherwise pager link generation would lost the ?type=... somehow, do both and type filtering seem to break completely, and thus {type} path param is kept for the time being. more test is needed

[asdfzdfj]

ok, giving another go at plucking out {type} parameter, and looks like it went better than expected? things seem to be working just like before but type is a query param now, could use some help verifying that it's not a one off on my instance

as to what was done this time:

• remove {type} from route params and defaults
• add #[MapQueryParameter] to $type in front controllers params (perhaps this is what was missing in my previous attempt to get it working)

[e-five256]

Took this for a spin and it looks good to me, do you still have any outstanding work?

I was going to say if the redirect stuff made things more difficult, we could consider removing it, but it seems it's sort of been reworked into allowing short form URLs, so maybe useful to keep around? (e.g. /home/threads/hot/1d can be specified as /hot/1d which is how it used to be). I think there will definitely be cases of 404s for both <1.5.3 and >1.5.3 instances but as long as we try to communicate it I'm sure people can figure out the new URLs

[asdfzdfj]

Took this for a spin and it looks good to me, do you still have any outstanding work?

I don't think I have any, the main problem of path has been adjusted to look somewhat like the previous ones, {type} has been plucked out, keeping {federation} for the time being as I could see it being a common filter option

I think this should be ready for review/inclusion

I was going to say if the redirect stuff made things more difficult, we could consider removing it, but it seems it's sort of been reworked into allowing short form URLs, so maybe useful to keep around? (e.g. /home/threads/hot/1d can be specified as /hot/1d which is how it used to be). I think there will definitely be cases of 404s for both <1.5.3 and >1.5.3 instances but as long as we try to communicate it I'm sure people can figure out the new URLs

I didn't rework the redirect stuff so much that I just split them in two controllers that also encodes/indicates the context it should be used in (rather than having to note that $name is meant to be magazine name and then do a branching on that), also I've seen them used in shimming old routes so I think I'll keep it for that for now

[e-five256]

One very minor thing, maybe not worth holding this up, but it looks like ?type= is kept in the header nav bar when set and selecting microblog.

For example on /m/testing/threads/hot/1y?type=links if I hover over Microblog I get the link /m/testing/microblog/hot/1y?type=links

I was able to remove it with

diff --git a/templates/layout/_header_nav.html.twig b/templates/layout/_header_nav.html.twig
index 2e1a5441..2e1bfc4b 100644
--- a/templates/layout/_header_nav.html.twig
+++ b/templates/layout/_header_nav.html.twig
@@ -40,13 +40,13 @@
         <a href="
             {% if magazine is defined and magazine %}
                 {% if is_route_name_starts_with('front') %}
-                    {{ options_url('content', 'microblog', 'front_magazine', {'name': magazine.name,'p': null}) }}
+                    {{ options_url('content', 'microblog', 'front_magazine', {'name': magazine.name,'p': null,'type':null}) }}
                 {% else %}
                     {{ navbar_posts_url(magazine) }}
                 {% endif %}
             {% else %}
                 {% if is_route_name_starts_with('front') %}
-                    {{ options_url('content', 'microblog', 'front', {'p': null}) }}
+                    {{ options_url('content', 'microblog', 'front', {'p': null,'type':null}) }}
                 {% else %}
                     {{ navbar_posts_url(null) }}
                 {% endif %}

Or maybe there's a better way in optionsUrl / frontOptionsUrl that would be preferred.

[asdfzdfj]

One very minor thing, maybe not worth holding this up, but it looks like ?type= is kept in the header nav bar when set and selecting microblog.

good point, added the suggested patch, that does bother me a little

to be honest, I kinda glossed over the navbar part, though that's likely because I also have another plan to try and push the condition in templating front link (the block you edited) down to the navbar_*_url twig functions so I didn't look or touch it much in here, and planned to make it a different/followed up patch of sorts