Convert front-facing files to use the front controller

Note: We are moving the topics of this forum and it will be deleted at some point

Publish your own request for comments/change or patches for the next version of phpBB. Discuss the contributions and proposals of others. Upcoming releases are 3.2/Rhea and 3.3.
User avatar
imkingdavid
Registered User
Posts: 1050
Joined: Thu Jul 30, 2009 12:06 pm

Convert front-facing files to use the front controller

Post by imkingdavid » Tue Apr 23, 2013 3:42 pm

Eventually the idea is to convert all front-facing files to use the recently merged controller system. It will require some major refactoring of a lot of basic functionality. We should try to leave code as is where possible (simply copy/paste), but some of it will have to be rewritten to work in the controller.

Because this change will be fairly massive (all of the user-accessible files are going to be touched to some extent), it would not make sense to wait until all of the changes have been made before merging. All that will do is result in an incredibly large diff that no one will review, and therefore it will never be merged. Instead, I would propose that we merge in chunks or even merge each controller separately, to allow for readable diffs and give time to test/review individual parts without worrying about other parts. Because the major controller infrastructure is already present, multiple controllers can be made at the same time as each other. The only issue I can foresee is linking from a finished controller to an unfinished controller or vice versa, but we can figure out how to best handle that later.

As we go, of course, we need to be sure to maintain and add new tests so that a greater amount of the codebase will be covered by the time it is merged than is currently covered. I'm hoping that by converting the files to controllers, a lot of the functionality can be put into methods that can then be unit tested so we don't have to always use functional tests, but we'll see.

We will need to include regression route definitions to continue to route old URLs to new ones (such routes will of course only work when URL rewriting is enabled because, otherwise, routes will need app.php in the URL [see next paragraph]).

One dependency that must be sorted out prior to even beginning is the paths issue (in short, it's the difference between URLs that look like /app.php?controller=topic/1#p124 and URLs that look like /app.php/topic/1#p124 or (using URL rewriting) /topic/1#p124. If we do not sort this out first, we will be breaking URLs twice (when it eventually does get sorted out), which is not acceptable.

I do have an example of a front controller file in my (as of yet unmerged) Post Revision Tracking pull request: here for anyone who wishes to see how it all works.

Before coding actually really begins, I would like to go ahead and propose and discuss URL routes now so we don't have to do so later. As an example, in my post revision tracking PR, I set up the route to be /post/{post_id}/revisions. We could, for instance, allow /post/{post_id} to redirect to something like /topic/{topic_id}#{post_id}.

One thing I would like to mention is that I plan on implementing a URL "slug" (not sure on the proper terminology, but that's what I've heard before) for some of the controllers (e.g. topic, forum, etc.). Basically, it's a completely optional string of text that can be placed in the URL for "pretty URLs", and is entirely ignored by the server. For instance, with a route definition of /topic/{topic_id}-{slug} let's say we have a URL like this: /topic/102-phpbb-3-1-release-date. When the user sees it, they are given some information about what is actually behind the link before they click it. When they end up clicking it, the server uses the topic ID to serve the user the appropriate topic, and completely disregards the slug. As I said, this would be primarily for making the URLs "pretty". Slugs would be generated based on the topic title, converted to all lowercase, and all special characters would be converted to dashes. An example of this can be seen in a currently out of date extension I have been working on here and here. Of course, you would be able to leave off the slug and use the route /topic/{topic_id} to get to the same topic. This is, of course, up for discussion.

Here is a non-exhaustive list of proposed routes from current URLs. I will add more, and I am open to suggestions on how to do it better if you have any input. Note that the query string will still be used for things like pagination and sort options where applicable.
index.php
  • index.php => /
posting.php
  • posting.php?mode=post&f={forum_id} => /forum/{forum_id}/new
  • posting.php?mode=reply&t={topic_id} => /topic/{topic_id}/reply
  • posting.php?mode=edit&p={post_id} => /post/{post_id}/edit
viewtopic.php
  • viewtopic.php?t={topic_id} => /topic/{topic_id} (adding #p{post_id} will still go directly to that post)
  • viewtopic.php?p={post_id} => /post/{post_id} (redirects to) /topic/{topic_id}#p{post_id}
memberlist.php
  • memberlist.php => /members
  • memberlist.php?mode=viewprofile&u={user_id} => /user/{user_id}
  • memberlist.php?mode=viewprofile&un={username} => /user/username
  • memberlist.php?mode=group&g={group_id} => /group/{group_id}
Xcp.php
I am not sure how control panels and their modules will be handled at this point because I have not thought it through yet.
Ideally for private messages, for example, we could have something like this:
  • ucp.php?i=pm (default) => /messages
  • ucp.php?i=pm&folder={folder_id} => /messages/folder/{folder_id} (note: folder_id in this case can be numeric or one of 'inbox', 'outbox' and 'sentbox')
  • ucp.php?i=pm&mode=compose => /messages/new
  • ucp.php?i=pm&mode=view&p={message_id} => /message/{message_id}
  • ucp.php?i=pm&mode=compose&action=reply&p={message_id} => /message/{message_id}/reply
  • ucp.php?i=pm&mode=compose&action=edit&p={message_id} => /message/{message_id}/edit
viewonline.php
  • viewonline.php => /members/online
I do custom MODs. PM for a quote!
View My: MODs | Portfolio
Please do NOT contact for support via PM or email.
Remember, the enemy's gate is down.

User avatar
keith10456
Registered User
Posts: 523
Joined: Sat Apr 22, 2006 10:29 pm
Contact:

Re: Convert front-facing files to use the front controller

Post by keith10456 » Tue Apr 23, 2013 5:06 pm

Kindly explain the benefit of this? This is an honest question (not trying to be a jerk). I'm not fully understanding :?
Last edited by keith10456 on Tue Apr 23, 2013 5:08 pm, edited 2 times in total.

User avatar
EXreaction
Registered User
Posts: 1555
Joined: Sat Sep 10, 2005 2:15 am

Re: Convert front-facing files to use the front controller

Post by EXreaction » Tue Apr 23, 2013 5:07 pm

I like the idea. A few comments I have on everything:

Slugs can be interesting, I've dealt with them on a number of mods/projects. The most important thing I think will be to have this a setting which can be turned on or off, the reason for this being that slugs are really only usable for English boards. Limiting the slug to (a-z0-9) makes building slugs easy and means there will not be any problems with the URL with the slug in it, but that means international boards would have a disaster of a slug if one at all. Slugs *can* be extended to UTF8 for the most part, but then there may be serious problems with the resulting URL and it can become an ugly mess depending on what software it is entered into.

viewonline -> /members/online

User avatar
imkingdavid
Registered User
Posts: 1050
Joined: Thu Jul 30, 2009 12:06 pm

Re: Convert front-facing files to use the front controller

Post by imkingdavid » Tue Apr 23, 2013 5:54 pm

keith10456 wrote:Kindly explain the benefit of this? This is an honest question (not trying to be a jerk). I'm not fully understanding :?
In general it is a good idea to route all traffic through one point. This makes it easier to control and format ouput, handle errors, etc. In this case, all traffic would be routed through the app.php file. By doing so, you are also decreasing code duplication. It also improves the testability of the code because controllers are classes and methods, whereas currently the majority of the code is global and cannot be easily isolated and tested as a unit.

And thanks Nathan, that is better. I changed it. :)
I do custom MODs. PM for a quote!
View My: MODs | Portfolio
Please do NOT contact for support via PM or email.
Remember, the enemy's gate is down.

User avatar
imkingdavid
Registered User
Posts: 1050
Joined: Thu Jul 30, 2009 12:06 pm

Re: Convert front-facing files to use the front controller

Post by imkingdavid » Tue Apr 23, 2013 7:09 pm

EXreaction wrote:Slugs can be interesting, I've dealt with them on a number of mods/projects. The most important thing I think will be to have this a setting which can be turned on or off, the reason for this being that slugs are really only usable for English boards. Limiting the slug to (a-z0-9) makes building slugs easy and means there will not be any problems with the URL with the slug in it, but that means international boards would have a disaster of a slug if one at all. Slugs *can* be extended to UTF8 for the most part, but then there may be serious problems with the resulting URL and it can become an ugly mess depending on what software it is entered into.
In the code to which I linked, my valid_slug() method does require a-z0-9 and dashes for a match. I do realize now that the generate_slug() method does not remove non a-z0-9 characters, so I will need to change that. But overall, yes, I agree that the slug should be an optional feature. In the end, it's not a huge deal, because the slug is not stored anywhere (it's generated at run time based on the topic or forum title), and even if the feature is turned off, people can manually add a slug to the URL if they want. I realize that that means that URLs can have false slugs that do not actually exemplify the content behind the link, but that's not our problem.
I do custom MODs. PM for a quote!
View My: MODs | Portfolio
Please do NOT contact for support via PM or email.
Remember, the enemy's gate is down.


User avatar
EXreaction
Registered User
Posts: 1555
Joined: Sat Sep 10, 2005 2:15 am

Re: Convert front-facing files to use the front controller

Post by EXreaction » Wed Apr 24, 2013 3:15 pm

imkingdavid wrote:I realize that that means that URLs can have false slugs that do not actually exemplify the content behind the link, but that's not our problem.
This can cause issues with search robots. This may be helpful for that (would need to research how many search engines support this or if other options exist):
http://googlewebmastercentral.blogspot. ... nical.html

Oleg
Posts: 1150
Joined: Tue Feb 23, 2010 2:38 am
Contact:

Re: Convert front-facing files to use the front controller

Post by Oleg » Fri Apr 26, 2013 11:35 pm

imkingdavid wrote:
keith10456 wrote:Kindly explain the benefit of this? This is an honest question (not trying to be a jerk). I'm not fully understanding :?
In general it is a good idea to route all traffic through one point.
If this is the entire benefit, there are a lot of other features our users would rather have.

User avatar
imkingdavid
Registered User
Posts: 1050
Joined: Thu Jul 30, 2009 12:06 pm

Re: Convert front-facing files to use the front controller

Post by imkingdavid » Sat Apr 27, 2013 3:34 pm

Oleg wrote:
imkingdavid wrote:
keith10456 wrote:Kindly explain the benefit of this? This is an honest question (not trying to be a jerk). I'm not fully understanding :?
In general it is a good idea to route all traffic through one point.
If this is the entire benefit, there are a lot of other features our users would rather have.
Good thing that isn't the entire benefit (bold emphasis added):
This makes it easier to control and format ouput, handle errors, etc. In this case, all traffic would be routed through the app.php file. By doing so, you are also decreasing code duplication. It also improves the testability of the code because controllers are classes and methods, whereas currently the majority of the code is global and cannot be easily isolated and tested as a unit.
EDIT: And by that logic, this will never get done. However, I feel that it is an important change that should be made sooner rather than later.
I do custom MODs. PM for a quote!
View My: MODs | Portfolio
Please do NOT contact for support via PM or email.
Remember, the enemy's gate is down.

User avatar
VSE
Extension Customisations
Extension Customisations
Posts: 670
Joined: Mon Mar 08, 2010 9:18 am

Re: Convert front-facing files to use the front controller

Post by VSE » Sat Apr 27, 2013 5:50 pm

imkingdavid wrote:One dependency that must be sorted out prior to even beginning is the paths issue (in short, it's the difference between URLs that look like /app.php?controller=topic/1#p124 and URLs that look like /app.php/topic/1#p124 or (using URL rewriting) /topic/1#p124. If we do not sort this out first, we will be breaking URLs twice (when it eventually does get sorted out), which is not acceptable.
FYI: In its current state, the meta_refresh function fails to handle controller file URLS right now... because it likes to filter out any slashes that appear among the URL params... So for now, a url like /app.php?controller=topic/1#p124 can not be handled by the meta_refresh function. It would redirect you instead to /app.php?controller=topic. It may probably also fail to handle /app.php/topic/1#p124 formatted URLs too.
Has an irascible disposition.

Post Reply