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

Add Redirects Admin Feature #746

Closed
eSilverStrike opened this issue Jan 26, 2017 · 27 comments
Closed

Add Redirects Admin Feature #746

eSilverStrike opened this issue Jan 26, 2017 · 27 comments
Assignees
Labels
Feature Issues that describe new features.
Milestone

Comments

@eSilverStrike
Copy link
Member

The URL Redirect is a great feature but there is no easy way through the Geeklog Admin interface to setup 301 Permanent Redirects. If someone sets up a new URL Rewrite for articles, etc.. they most likely will want all the old links found on external sites to send a proper redirect to the new link.

Plus the odd time I have had the need to setup a redirect when I needed to change an article or staticpage id which in turn obviously changed the url to access the item. Redirect should work or content generated by Geeklog and normal files found on a webserver like php, html, and images.

@eSilverStrike eSilverStrike added the Feature Issues that describe new features. label Jan 26, 2017
@eSilverStrike eSilverStrike added this to the 2.2.0 milestone Jan 26, 2017
@mystralkk
Copy link
Member

If I add a column storing an HTTP status code to the schema of $_TABLES['routes'] and modify URL routing UI, then will this issue be solved? If so, should the default value for an HTTP status code be 301, 302 (Found), or 303 (See other)?

@eSilverStrike
Copy link
Member Author

It should work I would think....

The only problem I see is or URL Routing to work we also need to enable URL Rewrite. For someone who may not have config option URL Rewrite enabled for their site they can't do something like this correct?

Method: Get
Rule: /staticpages/index.php?page=old-sp-id
Route: /staticpages/index.php?page=new-sp-id
Priority: 100
Status Code: 301

I don't have the code in front of me at the moment so I can't confirm.

@mystralkk
Copy link
Member

Implemented with change set ff8cc62. As you pointed out, this feature is available only when URL Routing is enabled. At first I thought of adding Redirects Admin Feature independently of URL Routing, but I thought it over.

@eSilverStrike
Copy link
Member Author

eSilverStrike commented Nov 23, 2017

I was testing #819 and ran into some issues with routing. I have the default routes enabled from the install with url_routing set to enabled without index.php.

On the homepage, links to topics contain the index.php and when clicked just return to the home page.

Also when clicking on article or staticpage link on the homepage like

/article/general

when i visit the article page, the url in the browser shows

/article.php?story=general

@eSilverStrike eSilverStrike reopened this Nov 23, 2017
@mystralkk
Copy link
Member

Also when clicking on article or staticpage link on the homepage like
/article/general

when i visit the article page, the url in the browser shows

/article.php?story=general

This can be solved by editing the default rule from

/article/@sid --> /article.php?story=@sid

to

/article/@sid --> /article.php/@sid

@eSilverStrike
Copy link
Member Author

That solution doesn't seem to work for staticpages though. Also shouldn't it work like before in 2.1.3 where it is based on the original url and not the url rewritten one?

I also had to modify the line 102 in public_html/index.php to

Router::dispatch();

or the Router class was not found. Where does the router class get required now anyways? (I did a quick search and did not find it)

@mystralkk
Copy link
Member

That solution doesn't seem to work for staticpages though. Also shouldn't it work like before in 2.1.3 where it is based on the original url and not the url rewritten one?

Staticpages need other routing rules. As for routing articles, you can't choose both /article.php?story=@sid and /article.php/@sid . Which do you think is better?

I also had to modify the line 102 in public_html/index.php to
Router::dispatch();
or the Router class was not found.

This is my mistake. When I implemented this feature, I wrongly added the Geeklog namespace.

Where does the router class get required now anyways? (I did a quick search and did not find it)

Since I introduced composer with Geeklog 2.1.2, the classes I create and some of existing classes are loaded automatically by way of Geeklog\Autoload class (system/classes/Autoload.php), as libraries installed by composer are.

@eSilverStrike
Copy link
Member Author

Yeah I had checked the Autoload class and didn't see it and had assumed it was loaded with composer.

I had left the namespace as is since I wasn't sure you were changing it or not.

I think I'll update the description and help a bit for the routing manager (with a link to the config docs). It's not clear to new users when visiting the manager what config options, etc.. needs to be changed to make the routing work. It's also not clear right away that routing only works for what can be rewritten (stories, topics, static pages, the article directory, and links).

eSilverStrike added a commit that referenced this issue Nov 24, 2017
For feature  #746. Router Manager now links to help file and updated the description a bit.
@mystralkk
Copy link
Member

Also when clicking on article or staticpage link on the homepage like
/article/general
when i visit the article page, the url in the browser shows
/article.php?story=general

If we want to get the content of "/article.php?story=general" by clicking on the link to "/article/general" WITHOUT changing the display of the browser's address bar, we can do so by fetching the content of the page inside the Router::dispatch method and outputting it. Is this behavior desirable?

@eSilverStrike
Copy link
Member Author

eSilverStrike commented Nov 24, 2017

I was thinking that was what it did before (ie Geeklog 2.1.3) but now that I think about it some more, it did not...

I guess we need to figure this out and once implemented I then can update the help to better explain things with the new Status Codes.

So URL Rewrite has always worked by instead of generating links for URLs to articles, topics, static pages, the article directory, and links like

/article.php?story=general

when enabled it would convert these links generated by the site to

/article.php/general

If someone happened to visit the first link (/article.php?story=general) then it would stay the same (url in the browser) but the canonical would reflect the correct rewritten url. I think this probably should stay the same. (what do you think?)

For regular URL Routing using your place holders like @sid, @page, etc... these should work similar to URL rewritten and i guess just return a status code of 200.

For URL Routing that uses a 301 redirect. Originally I was thinking that the Admin could setup multiple redirects from urls like:

/article.php?story=general
/article.php/general

and have them redirect to

/article/general (or what ever the Admin decides he wants)

or they could even substitute the article id with @sid. This way if the user decided in the future to go from not using url routing to using it he could setup the proper permanent redirects from with in Geeklog. This same logic would apply to all items that use url routing.

Another example is the Admin maybe had to change the story id or staticpage id. He then may want to create one or more redirects from for example:

/article.php?story=general
/article.php/general
/article/general

to the new id

/article/new-article-id (or whatever the url for articles is he is using)

Does this make sense? Do you have a better idea? Am I missing anything?

@mystralkk
Copy link
Member

I believe you summarize the feature completely. URL routing feature should:

  1. convert a traditional URL like "/article.php?story=general" into a nice-looking one such as "/article/general" with HTTP status code 200

  2. redirect a visitor to a already-non-existent URL like "/article.php?story=old_id" to a new URL like "/article.php?story=new_id" or "/article/new_id" (depending on $_CONF['url_rewrite'] and $_CONF['url_routing']) with HTTP status code 301 or 302

I wonder if we should allow users to specify 401 (Unauthorized) as HTTP status code for pages requiring Basic access authentication or Digest access authentication and/or 404 (Not Found) for non-existent items.

@eSilverStrike
Copy link
Member Author

and this could include the use of your placeholders like @sid or not depending on what the user wants to do.

How about the possibility of users setting up redirecting url rewritten urls like "/article.php/article-id" to url routed articles "/article/article-id" for those users who made the switch from url rewrite to url routing?

If url routing is enabled the url rewrite page "/article.php/article-id" does display the correct canonical but users may want to still add a redirect.

I wonder if we should allow users to specify 401 (Unauthorized) as HTTP status code for pages requiring Basic access authentication or Digest access authentication and/or 404 (Not Found) for non-existent items.

Do you think a 404 is needed? If the page doesn't exist then a 404 should already be created.

@mystralkk
Copy link
Member

How about the possibility of users setting up redirecting url rewritten urls like "/article.php/article-id" to url routed articles "/article/article-id" for those users who made the switch from url rewrite to url routing?
If url routing is enabled the url rewrite page "/article.php/article-id" does display the correct canonical but users may want to still add a redirect.

I am not sure how I should deal with this. Only better documents will help.

Do you think a 404 is needed? If the page doesn't exist then a 404 should already be created.

No, a 404 is not necessary.

@eSilverStrike
Copy link
Member Author

Okay I was testing #819 and found an issue the Topic Page Navigation does not work for either URL Rewrite or URL Routing. (it includes the index.php when not suppose to and always displays page nav for the homepage even when on another topic)

Topic urls are also not generated correctly for URL Rewrite (but URL Routing ones are fine)

The topic route I have setup is
/topic/@topic
/index.php?topic=@topic
with status code of 200. (I think I might add this record to the sample data loaded by the install)

@mystralkk
Copy link
Member

Changeset c6aa2ba fixed this issue.

@eSilverStrike
Copy link
Member Author

Settings: URL Rewrite = Enabled. URL Routing = Enabled without "index.php")

Just tested again and topic urls are generated correctly but all topic links seem to just return the home page. Topic Page Navigation links also contain "index.php" and don't reflect the topic I am in.

@eSilverStrike
Copy link
Member Author

eSilverStrike commented Apr 21, 2018

@mystralkk

Okay I think I have partially fixed the topic url issues (I have not yet committed the changes)

The main problem happens when the TOPIC_PLACEHOLDER gets added to the url when url_routing is enabled. This was causing issues when you have routes setup for topics. I have since added 2 routes for topics to the routing table:

rule:/topic/@topic route:/index.php?topic=@topic
rule:/topic/@topic/@page route:/index.php?topic=@topic&page=@page

This (among a few other changes) has fixed the issue for topics and the homepage along with paging. For example this is how the General News topic urls now look:

Topic URLS Normal
http://192.168.0.200/index.php?topic=General
http://192.168.0.200/index.php?topic=General&page=2

Topic URL Rewrite
http://192.168.0.200/index.php/topic/General
http://192.168.0.200/index.php/topic/General/2

Topic URL Routing (with index.php)
http://192.168.0.200/index.php/topic/General
http://192.168.0.200/index.php/topic/General/2

Topic URL Routing (without index.php)
http://192.168.0.200/topic/General
http://192.168.0.200/topic/General/2

The problem happens with my fix is when you remove the routes. With url routing still enbled the new routes for the topics becomes:

Topic URL Routing (without index.php) AND without any routes defined
http://192.168.0.200/index.php/General
http://192.168.0.200/index.php/General/2

They still work fine but are not the same as the url_rewrite urls since with url_routing still enabled the TOPIC_PLACEHOLDER does not get added back in. All of the others (articles, staticpages) if you remove the routes but url_routing is still enabled defaults back to how the url_rewrite urls are.

To fix this issue we need to be able to tell if there are topic routes available when url_routing is enabled so we know to add in the topic place holder when we build topic urls (like in index.php and with the function TOPIC_getUrl). Is there a way we can request this info from the router class?

Do you want me to commit my changes so you have a better idea of what I mean?

The other option is we just commit my changes and leave it as is. We then would have to document that topic urls do not look the same if url_rewrite is enabled along with url_routing but without any defined topic routes.

eSilverStrike added a commit that referenced this issue Apr 22, 2018
@eSilverStrike
Copy link
Member Author

eSilverStrike commented Apr 22, 2018

@mystralkk Okay I committed the code mentioned above. You will have to run the devel-db-update.php in the install so the topic records and routing table changes get added.

Here are the 2 remaining issues I see at this time with url routing:

  1. The topic routing still does not fall back to the same url as url_rewrite if routing is enabled but no topic routes are defined. Should we just leave the topic routing as is for now? (I would have to update the docs to note the issue.

  2. Setting up a 301 redirect like you mentioned On Nov 24th

redirect a visitor to a already-non-existent URL like "/article.php?story=old_id" to a new URL like "/article.php?story=new_id" or "/article/new_id" (depending on $_CONF['url_rewrite'] and $_CONF['url_routing']) with HTTP status code 301 or 302.

I have not been able to create a 301 redirect from a non existing article to one that exists. Is this possible yet? (If it is not and you do not have time too add it in, we can lock the url routing status code to just 200 for Geeklog 2.2.0)

eSilverStrike added a commit that referenced this issue Apr 30, 2018
…. Added new route for Staticpages Print page

For feature #746

Also fixed insert of new route failing
eSilverStrike added a commit that referenced this issue Apr 30, 2018
@eSilverStrike eSilverStrike modified the milestones: 2.2.0, 2.2.1 Jun 26, 2018
@eSilverStrike eSilverStrike modified the milestones: 2.2.1, 2.2.2 Sep 27, 2019
@eSilverStrike
Copy link
Member Author

Moving to Geeklog v2.2.2 milestone.

mystralkk added a commit that referenced this issue Jan 11, 2022
@mystralkk
Copy link
Member

@eSilverStrike , I think I have fixed the two issues you mentioned on 23 Apr 2018 with change set 9b80653.

@eSilverStrike
Copy link
Member Author

eSilverStrike commented Jan 11, 2022

I will come back to this issue after I finish working on the issue I am fixing now for a more in-depth look.

I haven't pulled the latest changeset yet.

@eSilverStrike
Copy link
Member Author

eSilverStrike commented Jan 11, 2022

@mystralkk regarding the 2 issues you said should be fixed. From what I can tell they are not.

I unfortunately found another issue with the routing which I just spent most of the last 2 hours on fixing. I will commit the changes after the comment. It deals with articles that have multiple pages. Initially article with multiple pages only worked with URL Rewrite Mode and none of the URL Routing Modes (I guess I missed this issue before when I tested).

The fix for this includes adding a new record in the routing table that must always come after the article print route record. This is due to the fact that both the print mode variable and the article page count variable share the same location in the URL so if the variable is fount to be numeric it is assumed to be a page number else it will be considered a "Mode".

I also updated the Config Documentation /docs/english/config.html#url-routing which explains this requirement. (I only partly did the Japanese version)

eSilverStrike added a commit that referenced this issue Jan 11, 2022
Partial fix for issue #746
Fixes when URL Routing is enabled and articles have multiple pages.
@eSilverStrike
Copy link
Member Author

eSilverStrike commented Jan 11, 2022

@mystralkk Testing URL Routing while having backwards compatibility for URL Rewrite is such a pain and can get real confusing 😀.

Anyways, Regarding the 2 remaining issues from Apr 22, 2018 comment. I don't think they are fixed yet unless I am missing something.

Issue 1
Initially I think we had it working where the URL Routing records was disabled or missing, URL Routing pages (like for articles, topics, static pages, etc...) would just fall back to how URL Rewrite does it. At least that is how I read that point 1 in the comment. Right now it looks like if the routing record is removed say for the static page, the updated links to the page gets created properly for example

/staticpages/index.php/staticpage-id

but when they are clicked on it goes to a 404 page (article does the same thing and the topic page actually just soft redirects to the homepage).

So these links should work and actually take them to whatever content it is for.

To keep everything straight I make sure that the docs/english/config.html#url-routing is always updated to how it should all work. This is currently what is written about point 1)

Note: If you use URL Routing but remove or disable one of the predefined routes, that route will fallback to how URL Rewrite will display the URL. The only time this is not true is with Topics. If their routes are removed or disabled the URL Rewrite will be "https://www.example.com/@topic" and for paging "https://www.example.com/@topic/@page". The "topic" text place holder will not be present (see above URL Rewrite for examples). This is a current limitation of Geeklog and the router but shouldn't cause an issue as you could change the route to reflect the URL Rewrite url if needed.

Issue 2
Point 2 doesn't seem to work either. I created a rule for an article that does not exist and pointed it to an article that exists

Rule: /article/article-id-that-doesnt-exist
Rule: /article.php?story=article-id-that-exists
Status Code: 301

and when I went to http://example.com/article/article-id-that-doesnt-exist I didn't get redirected to the proper page (article-id-that-exists).

@mystralkk
Copy link
Member

As for issue 2, did you set the priority high enough? Otherwise, the route "/article/article-id-that-doesnt-exist" matches the default rule "/article/@sid", which will cause 404 error.

@eSilverStrike
Copy link
Member Author

eSilverStrike commented Jan 12, 2022

You are right my mistake. I had it below the default story rule. I didn't even think of that... I will add a reminder to the docs.

@eSilverStrike
Copy link
Member Author

eSilverStrike commented Jan 12, 2022

Okay I pushed a fix for issue #1 and I tested it with URL Rewrite, and the 2 URL Routing modes for Staticpages, Articles, and topics when their routes are enabled and disabled.

This fix allows for fallback to URL Rewrite if URL Routing is enabled but no routes are found.

In the URL class I updated setArgNames. If URL Routing is enabled and the router string parse doesn't find any arguments (this assumes there was none to be found in the routes table) it automatically attempts to find them using the urlRewrite method (so order of variables in the URL). I don't think doing it this way will cause any problems (can you think of any @mystralkk ?)

If you are okay with the fix I think this feature can be closed.

Forgot one thing... I didn't update the Japanese docs with all the latest English changes for this since it already had some things translated. I didn't know if it was easier for you to add to it or just start with a fresh translation.

@mystralkk
Copy link
Member

I am okay with your fix. I'll take care of the Japanese docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issues that describe new features.
Projects
None yet
Development

No branches or pull requests

2 participants