[wp-trac] [WordPress Trac] #36292: Rewrites: Next Generation

[WordPress Trac]

#36292: Rewrites: Next Generation
-----------------------------+-----------------------------
 Reporter:  rmccue           |       Owner:  rmccue
     Type:  feature request  |      Status:  assigned
 Priority:  normal           |   Milestone:  Future Release
Component:  Rewrite Rules    |     Version:
 Severity:  normal           |  Resolution:
 Keywords:                   |     Focuses:
-----------------------------+-----------------------------

Comment (by giuseppe.mazzapica):

 Replying to [comment:11 MikeSchinkel]:
 > Replying to [comment:10 giuseppe.mazzapica]:
 > > Replying to [comment:9 MikeSchinkel]:
 > > > I think both of the solutions suggested here might be missing
 something.
 > > >
 > > > If hooks are used as @giuseppe.mazzapica then unit testing will be
 more difficult if not close to impossible, and that is Part 2 of @rmccue's
 requirements ''(which I definitely agree with.)''
 > >
 > > Why? Can you better explain this? If there will be a callback or an
 object that receives an array of rules and do the parsing, you can easily
 unit test that callback or object by passing to it an arbitrary set of
 rules.
 >
 > Actually, you can test but you cannot really '''''unit''''' test.
 >
 > You ''can'' do is ''"integration"'' testing though, and that's what your
 test suites in Cortex are doing.
 >
 > [https://www.toptal.com/qa/how-to-write-testable-code-and-why-it-matters
 Here is an article] that explains the difference.

 Believe me, I know exactly what unit tests are. I wrote a library (http
 ://brain-wp.github.io/BrainMonkey/) for the very sole purpose of
 '''unit''' testing WordPress plugins.

 You'll be surprised that my plugins are the very few open source WordPress
 plugins on the entire web that are tested '''without''' loading WordPress.
 Every WordPress class and function and hook is mocked.

 No, they are '''not''' integration tests.

 Moreover, you might find interesting ''my'' answer on WPSE
 http://wordpress.stackexchange.com/questions/164121/testing-hooks-
 callback/164138#164138 where I talk just about unit testing & WordPress.

 > Of course you can't really unit test ''anything'' that has a hook in it,
 which indicts the majority of the WordPress extension API, and I am fully
 aware of that.

 Using my Brain Monkey, I actually can.

 > As best I can tell from what you proposed the callback is not called
 until ''after'' the regex matches, so unless I misunderstand your
 suggestion the matches would still be context free.
 >
 > Maybe your definition of ''"context"'' is different than I intended when
 I used it? The context I meant was that state of the system configuration
 and/or data currently in the system.  For example, if we want Categories
 in the URL root and we have a `sports` category slug then that URL would
 route to the Category Archive instead of routing to a Page with a URL slug
 of `sports`.  ''(And with the approach I proposed we'd need to make sure
 users were warned when path segments became ambigous.)''

 In my first comment here
 (https://core.trac.wordpress.org/ticket/36292?replyto=11#comment:4) I
 proposed to provide context when adding hooks (via the introduction of a
 new hook `add_rewrite_rules` that pass an instance of request object)
 '''and''' to pass context route callbacks.

 Reason is that storing rules in DB, like WP does now, conditional rule
 addition is not possible at all. If the flushing system will be refactored
 (as I hope) a cache layer should probably implemented, and conditional
 addition will break cache.

 So, when a route is ''slightly'' different based on context, is probably
 convenient just ad it and then handle differences while performing route
 action, which is
 easy and straightforward if route action is a callback that receives
 context as argument.

 When differences are bigger, you can use the context to conditional adding
 rules, assuming that flushing system is refactored.


 > P.S. BTW, I set up a test with Cortex and one thing I noticed was quite
 a lot of overhead in the autoloading and the creation of all the objects
 prior to getting to the point of calling `Router->match()`. Further
 `Router->parseRoutes` does a lot of work on each page load that could be
 cached. That includes `RouteParser->parse()` doing a `preg_split()` and
 `RouteParser->parsePlaceholders() `doing a `preg_match_all()` for each
 route for each page load.

 Yes, Cortex approach is '''not''' optimized. A system based purely on
 FastRoute would be much (much) faster.

 The counter-optimization of Cortex has different reasons. Among others,
 operating outside of WordPress core and wanting to have 100% core
 compatibility, I have to include some additional overhead.

 Moreover, FastRoute have no system to differentiate routes based on domain
 and have no way do define "priority" for routes nor to define route
 "groups".

 FastRoute is committed to speed, so it reduces features to avoid overhead,
 but I decided that some overhead is an acceptable trade off to have those
 features.

 Even because I did some very basic profiles comparing Cortex with
 WordPress native implementation and noted that, in real world use cases,
 there is close to zero performance loss and even in worst case scenario
 the performance loss is so trivial that is negligible compared to the
 whole WordPress environment bootstrap process.

 (In the "Clever Rules" implementation wehre I used the URL chunking
 approach performance loss was much bigger).

 So, by now, the flexibility and general benefits I get from Cortex exceed
 by far the small performance issue.

 Finally, consider that Cortex is not stable yet, and one of the reasons is
 that there is no cache layer yet and so route parsing / matching happens
 on every request which, of course, is suboptimal.

 However, I use it in production in quite big sites (sorry, I can't share
 it publicly, but if you are intersted I could privately) and the
 performance impact is unnoticeable, lost in the much bigger "trouble" of
 WordPress bootstrap.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/36292#comment:12>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform