[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 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.

 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.

 It is possible that we should use hooks for routing -- because that is how
 practically everything else in WordPress is extended -- and forgo the
 benefits of unit testing and be satisfied instead with integration testing
 in exchange for easier extensibility.  Personally I am on the fence about
 it and could go either way.

 > The "chunck" step, will create an array for every rule, and arrays are
 notoriously memory consuming. That's the less.

 The existing system uses arrays, and unless we generate code with `switch`
 statements -- not going to happen for a user-administerable blogging
 software -- there is not many others way to handle it routing patterns
 besides arrays.  Also, when you wrote ''"will create an array for every
 rule"'' it implies to me that you may simply not understand what I
 proposed?

 > This is why I proposed a system that adds rules with a callback: using a
 callback is very easy to pass context to it and implement rule logic based
 on context. In fact, I proposed that rule callbacks should receive an
 instance of the yet to come WP_HTTP_Request (that @rmccue mentioned in
 OP). That object would be enough to provide all the context is needed.

 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.)''

 > Consider, first of all, that FastRoute performs a singular
 `preg_match_all` call to find a match among all rules.

 Are you '''absolutely certain, without-a-doubt''' that the above
 statements of yours that I quoted is correct?

 I assert your statement is not correct and that FastRoute chucks in groups
 of ~10 rules meaning it may need to make many more than a single
 `preg_match_all()` call in order to match a URL.  Would you like me to
 prove my assertion?

 ----
 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.

 So even if FastRoute only performed a singular `preg_match_all` call the
 two extra regex you do for each rule negates your assertions there is only
 one.

 Since I did not profile your code I have no certainty on the amount of
 overhead  all your setup adds but it felt like a lot of overhead as
 implemented as It took what seemed like forever to trace through it using
 XDEBUG just to get to `Router->match()`.

 I also have not yet profiled my proposed approach for performance either.
 FWIW.

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