URI page router

[foxly]

A page router is a set of classes that analyzes a URI supplied by the web server, and determines what (if any) page to return to a user. It's the heart of any web application and one of the most difficult parts to build ...because it must be reliable, secure, and extremely fast.

In BuddyPress, the functions in the /bp-core/bp-core-catchuri.php file (combined with various code strewn about the plugin) serve as the page router.

As part of building BP-Media, our team performed a detailed audit on BP's page router code. Although when *tested by humans* the code appears to work properly, when we instrumented and traced it, we found some serious problems:

1) It can't handle URI's with repeated tokens "example.com/foo/bar/foo"
2) It runs excessive numbers of queries
3) It's very CPU intensive
4) It has no caching
5) Its impossible to unit test
6) Nobody understands how it works

So we've written you a new router class, with the following benefits:

1) It correctly handles any URI
2) It uses 1 query per page load instead of 5 (best case) or 20 (typical) or hundreds (site with dense page hierarchy and lots of blogs)
3) Uses 40% less CPU cycles per run
4) Includes full unit tests (once you add the Unit Test suite we've built for you, attached in another SVN ticket)
5) It supports caching (once we discuss how you'd like to implement it)
6) It includes an advanced error handling system that fully integrates with the unit tests (we'll post the utilities libraries to let you print the error traces to your browser in another ticket)
7) It has great documentation.

This patch is current as of an hour ago, and cleanly applies to the 1.6 trunk.

Enjoy!

/^F

[foxly]

bp-core-catchuri.php patch

[DJPaul]

Can you check the patch? There's a lot of stuff marked in red that looks like it's meant to be marked in green? Thanks

[boonebgorges]

Thanks, Foxly. I don't think anyone on our team has much love for bp_core_set_uri_globals(), and we'll be happy to replace it with something better.

As DJPaul notes, this patch doesn't appear to be generated correctly, so that I can't make it apply. I tried modifying the path names and swapping the - signs with the + signs, but it still won't go. Could you regenerate it please?

Also, would you be willing to share the automated tests you've written for these methods? Or are they available somewhere in the bp-media codebase where you can point us?

For future reference, this patch should fix #3554

[foxly]

Updated catchuri file

[foxly]

Here's the complete bp-core-catchuri.php file - just replace the existing file with this, and you're good to go.

We are absolutely willing to share the AUT's we've written for these methods. It's at about 50% coverage right now. I wouldn't be comfortable releasing this code to production until it's at 100% coverage.

Now that we know you want to use the code, we'll finish up the tests and documentation quickly - ETA about 2 weeks.

=====================

We're currently putting the finishing touches on Version 2.0 of the Unit Test Runner System we built for BuddyPress. Its awesome. Once I have it finished in the next couple of weeks, it should be able to support "test scenarios" ...for example:

a) Test BP with WP running in single-site mode
b) Test BP with WP running in "network" mode
c) Test BP in "network" mode with "root profiles"

Did we mention we have a version of the test runner that can be run in the user's browser at the admin back-end? ​http://code.google.com/p/buddypress-media/wiki/DOCS_bpm_unit_test_top

Its great for remotely debugging vexing problems on users' servers.

So give us a bit of time to get all this stuff packed up nicely, re-branded, and documented for you, and then get ready to enjoy full-on automated testing.

PS: If you guys move to GIT, our tests also run on ​http://travis-ci.org/, so you can automatically check if any new commit causes a unit test to fail (and bounce that commit!)

\^F

[boonebgorges]

Thanks, Foxly.

I've spent a bit of time with the patch. Before I level criticism :) let me say that the general architecture of this router class is far, far more sane than the hodge-podge currently in BP. I think it represents a very nice foundation for moving forward.

In order to get it running on my setup, I had to correct a couple of problems:

• The logic that accounted for WP running in a subdirectory (example.com/wp/) was missing. It's possible that it was incorrectly ported because the original parser is a little redundant in this respect, and has unclear docs.
• $wp_query->is_singular had to be set to true in order for most pages not to 404
• Cleared up a number of smallish PHP notices

The first of these errors (not running properly in a subdirectory) points toward the importance of having automated tests for this new class. For all of the bad things you might say about the existing bp_core_set_uri_globals() - and there are plenty of bad things to say! - it had been battle tested (if not unit tested) to account for lots and lots of different scenarios. It's imperative to make sure that they've carried over. I'm looking forward to getting my hands dirty writing some tests to cover these cases, but I'll wait until you guys have shared yours, so that we don't duplicate effort.

There are some details that we'll need to hash out. Code format (naming conventions, comment format, etc) should be brought into line with WP/BP standards. I'll want to talk more about the error/status reporting system: I would like to avoid, if possible, introducing our own bespoke system when we have something more general like WP_Error available, though you've suggested that your format is part of the unit testing framework, so we'll have to discuss how to move forward with it.

BP 1.6 is far overdue. I want to get it out, and adding these changes to the release has the potential to extend the cycle by a lot. So I'm moving it to Future Release with a 1.7-early tag. It's one of the two or three things I'll be most interested in tackling for the next dev cycle, and I'll keep playing with it in the meantime.

4140.02.patch is a proper patch against the current trunk (r5970), with some of the fixes above. I'm going to spend more time with it in the upcoming weeks or two, to test some of the benchmark claims that Foxly put in the original ticket :) Again, thanks to the BP Media team, and thanks in advance to all for your feedback and help in refinement.

[foxly]

Find the spec sheet for WP/BP coding standards?

^F

[boonebgorges]

Sure thing: ​https://codex.wordpress.org/WordPress_Coding_Standards

[foxly]

We disagree with "Yoda" if statements, because that technique has little value anymore - all modern IDE's (netbeans will underline the error in yellow).

But Yoda statements make code much harder to mentally parse/skim, because your brain has to flip the words so it makes sense.

if('red' == $car && 'running_ok' == engineStatus() | | $driving = engineStatus( ))

That's just not easy to read.

You'll also notice we use "headings" to break up sections of large functions. This makes them much easier to skim when you're looking for something.

Overall, just notice how much easier it is to go through our code than many other files in BP.

I will look into the WP global error message object. The last time I checked it was not up to the task, is why I built our own system.

^F

[boonebgorges]

You'll also notice we use "headings" to break up sections of large functions. This makes them much easier to skim when you're looking for something.

+1. I like this.

That's just not easy to read.
Overall, just notice how much easier it is to go through our code than many other files in BP.

It's understandable that you'd find your own code easier to go through :) In any case, it's all pretty subjective. If we want to start a conversation about departing from (or modifying) WP coding standards, let's not do it in this ticket. It's a minor issue and I don't want to detract from substantive discussion of your patch.

[fanquake]

You know what I see missing from this.. instantiating the BP router as a global singleton and secondly, methods for other plugins to use these router functions. They need to be able to register/unregister, and be able to check for instances of slug collisions and such things.

This could be the beginning of a BuddyPress Plugin (Component) API, for third party plugins.

[boonebgorges]

I'm moving this into the 1.7 milestone for further discussion.

I would love to clean up our router, but not without automated tests. I would also like to examine the possibility that if we're going to put a bunch of work into rewriting the URL parser, we may as well write it using proper WP rewrites, which will perform far better (though the refactoring process will be much more complex, I'd imagine).

[Foxly]

Its scheduled. Its the next thing up after we finish our system logging classes.

^F

[johnjamesjacoby]

Replying to fanquake:

This could be the beginning of a BuddyPress Plugin (Component) API, for third party plugins.

Would rather improvements go into the BP_Component class where needed. The intention there was for it to be usable anywhere.

[DJPaul]

Going to be looking at routing in general in a future release, probably 1.8, so I'm adjusting the milestone.

[DJPaul]

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see ​https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: ​https://make.wordpress.org/chat/