Skip to content

Commit

Permalink
Implementing routing stages pre-, main and post-stage
Browse files Browse the repository at this point in the history
  • Loading branch information
@Hackwar
Hackwar committed Nov 30, 2014
1 parent ff5257b commit 0e6ca11
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 48 deletions.
76 changes: 58 additions & 18 deletions libraries/cms/router/router.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,12 @@ class JRouter
* @since 1.5
*/
protected $rules = array(
'buildpreprocess' => array(),
'build' => array(),
'parse' => array()
'buildpostprocess' => array(),
'parsepreprocess' => array(),
'parse' => array(),
'parsepostprocess' => array()
);

/**
Expand All @@ -75,8 +79,12 @@ class JRouter
* @deprecated 4.0 Will convert to $rules
*/
protected $_rules = array(

This comment has been minimized.

Sign in to view

Copy link
@johanjanssens

johanjanssens Dec 3, 2014

Not sure if you need to add additional arrays here for each. You could do a

protected $rules = array(
    'build => array(
        'before' => array()
        'after' => array();
    ),
    parse'  => array(
        'before' => array()
        'after' => array();
    ),
)

This comment has been minimized.

Sign in to view

Copy link
@Hackwar

Hackwar Dec 3, 2014

Author Owner

As I wrote in the other comment, we not only have before and after, but the build process itself, too. Besides that, the current router expect build and parse to be filled with callbacks and not with additional arrays that again hold the callbacks. Thus the proposed structure, as nice as it sounds, would break backwards compatibility, which is why I choose the current structure.

This comment has been minimized.

Sign in to view

Copy link
@johanjanssens

johanjanssens Dec 3, 2014

As per other comment. I would recommend to reconsider the approach of having 3 different types of callbacks. It makes things quite complex to understand.
'buildpreprocess' => array(),
'build' => array(),
'parse' => array()
'buildpostprocess' => array(),
'parsepreprocess' => array(),
'parse' => array(),
'parsepostprocess' => array()
);

/**
Expand Down Expand Up @@ -175,8 +183,12 @@ public static function getInstance($client, $options = array())
*/
public function parse(&$uri)
{
// Do the preprocess stage of the URL build process
$vars = $this->processParseRules($uri, 'preprocess');

// Process the parsed variables based on custom defined rules
$vars = $this->_processParseRules($uri);
// This is the main parse stage
$vars += $this->_processParseRules($uri);

// Parse RAW URL
if ($this->_mode == JROUTER_MODE_RAW)
Expand All @@ -189,6 +201,9 @@ public function parse(&$uri)
{
$vars += $this->_parseSefRoute($uri);
}

// Do the postprocess stage of the URL build process
$vars = $this->processParseRules($uri, 'postprocess');

return array_merge($this->getVars(), $vars);
}
Expand All @@ -213,8 +228,12 @@ public function build($url)

// Create the URI object
$uri = $this->createURI($url);

// Do the preprocess stage of the URL build process
$this->processBuildRules($uri, 'preprocess');

// Process the uri information based on custom defined rules
// Process the uri information based on custom defined rules.
// This is the main build stage
$this->_processBuildRules($uri);

// Build RAW URL
Expand All @@ -228,6 +247,9 @@ public function build($url)
{
$this->_buildSefRoute($uri);
}

// Do the postprocess stage of the URL build process
$this->processBuildRules($uri, 'postprocess');

$this->cache[$key] = clone $uri;

Expand Down Expand Up @@ -338,28 +360,36 @@ public function getVars()
* Attach a build rule
*
* @param callback $callback The function to be called
* @param string $stage The stage of the build process that
* this should be added to. Possible values:
* 'preprocess', '' for the main build process,
* 'postprocess'
*
* @return void
*
* @since 1.5
*/
public function attachBuildRule($callback)
public function attachBuildRule($callback, $stage = '')
{
$this->_rules['build'][] = $callback;
$this->_rules['build' . $stage][] = $callback;
}

/**
* Attach a parse rule
*
* @param callback $callback The function to be called.
* @param string $stage The stage of the parse process that
* this should be added to. Possible values:
* 'preprocess', '' for the main parse process,
* 'postprocess'
*
* @return void
*
* @since 1.5
*/
public function attachParseRule($callback)
public function attachParseRule($callback, $stage = '')
{
$this->_rules['parse'][] = $callback;
$this->_rules['parse' . $stage][] = $callback;
}

/**
Expand All @@ -370,7 +400,7 @@ public function attachParseRule($callback)
* @return boolean
*
* @since 1.5
* @deprecated 4.0 Use parseRawRoute() instead
* @deprecated 4.0 Attach your logic as rule to the main parse stage
*/
protected function _parseRawRoute(&$uri)
{
Expand All @@ -385,6 +415,7 @@ protected function _parseRawRoute(&$uri)
* @return array Array of variables
*
* @since 3.2
* @deprecated 4.0 Attach your logic as rule to the main parse stage
*/
protected function parseRawRoute(&$uri)
{
Expand All @@ -399,7 +430,7 @@ protected function parseRawRoute(&$uri)
* @return string Internal URI
*
* @since 1.5
* @deprecated 4.0 Use parseSefRoute() instead
* @deprecated 4.0 Attach your logic as rule to the main parse stage
*/
protected function _parseSefRoute(&$uri)
{
Expand All @@ -414,6 +445,7 @@ protected function _parseSefRoute(&$uri)
* @return array Array of variables
*
* @since 3.2
* @deprecated 4.0 Attach your logic as rule to the main parse stage
*/
protected function parseSefRoute(&$uri)
{
Expand All @@ -428,7 +460,7 @@ protected function parseSefRoute(&$uri)
* @return string Raw Route
*
* @since 1.5
* @deprecated 4.0 Use buildRawRoute() instead
* @deprecated 4.0 Attach your logic as rule to the main build stage
*/
protected function _buildRawRoute(&$uri)
{
Expand All @@ -443,6 +475,7 @@ protected function _buildRawRoute(&$uri)
* @return string Raw Route
*
* @since 3.2
* @deprecated 4.0 Attach your logic as rule to the main build stage
*/
protected function buildRawRoute(&$uri)
{
Expand All @@ -456,7 +489,7 @@ protected function buildRawRoute(&$uri)
* @return string The SEF route
*
* @since 1.5
* @deprecated 4.0 Use buildSefRoute() instead
* @deprecated 4.0 Attach your logic as rule to the main build stage
*/
protected function _buildSefRoute(&$uri)
{
Expand All @@ -471,6 +504,7 @@ protected function _buildSefRoute(&$uri)
* @return string The SEF route
*
* @since 3.2
* @deprecated 4.0 Attach your logic as rule to the main build stage
*/
protected function buildSefRoute(&$uri)
{
Expand All @@ -494,17 +528,20 @@ protected function _processParseRules(&$uri)
/**
* Process the parsed router variables based on custom defined rules
*
* @param JUri &$uri The URI to parse
* @param JUri &$uri The URI to parse
* @param string $stage The stage that should be processed.
* Possible values: 'preprocess', 'postprocess'
* and '' for the main parse stage
*
* @return array The array of processed URI variables
*
* @since 3.2
*/
protected function processParseRules(&$uri)
protected function processParseRules(&$uri, $stage = '')
{
$vars = array();

foreach ($this->_rules['parse'] as $rule)
foreach ($this->_rules['parse' . $stage] as $rule)
{
$vars += call_user_func_array($rule, array(&$this, &$uri));
}
Expand All @@ -530,15 +567,18 @@ protected function _processBuildRules(&$uri)
/**
* Process the build uri query data based on custom defined rules
*
* @param JUri &$uri The URI
* @param JUri &$uri The URI
* @param string $stage The stage that should be processed.
* Possible values: 'preprocess', 'postprocess'
* and '' for the main build stage
*
* @return void
*
* @since 3.2
*/
protected function processBuildRules(&$uri)
protected function processBuildRules(&$uri, $stage = '')
{
foreach ($this->_rules['build'] as $rule)
foreach ($this->_rules['build' . $stage] as $rule)
{
call_user_func_array($rule, array(&$this, &$uri));
}
Expand Down
80 changes: 50 additions & 30 deletions libraries/cms/router/site.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ public function build($url)
* @return array
*
* @since 3.2
* @deprecated 4.0 Attach your logic as rule to the main parse stage
*/
protected function parseRawRoute(&$uri)
{
Expand Down Expand Up @@ -235,6 +236,7 @@ protected function parseRawRoute(&$uri)
* @return string Internal URI
*
* @since 3.2
* @deprecated 4.0 Attach your logic as rule to the main parse stage
*/
protected function parseSefRoute(&$uri)
{
Expand Down Expand Up @@ -415,6 +417,7 @@ protected function parseSefRoute(&$uri)
* @return string Raw Route
*
* @since 3.2
* @deprecated 4.0 Attach your logic as rule to the main build stage
*/
protected function buildRawRoute(&$uri)
{
Expand All @@ -441,7 +444,7 @@ protected function buildRawRoute(&$uri)
* @return void
*
* @since 1.5
* @deprecated 4.0 Use buildSefRoute() instead
* @deprecated 4.0 Attach your logic as rule to the main build stage
* @codeCoverageIgnore
*/
protected function _buildSefRoute(&$uri)
Expand All @@ -457,6 +460,7 @@ protected function _buildSefRoute(&$uri)
* @return void
*
* @since 3.2
* @deprecated 4.0 Attach your logic as rule to the main build stage
*/
protected function buildSefRoute(&$uri)
{
Expand Down Expand Up @@ -531,23 +535,29 @@ protected function buildSefRoute(&$uri)
* Process the parsed router variables based on custom defined rules
*
* @param JUri &$uri The URI to parse
* @param string $stage The stage that should be processed.
* Possible values: 'preprocess', 'postprocess'
* and '' for the main parse stage
*
* @return array The array of processed URI variables
*
* @since 3.2
*/
protected function processParseRules(&$uri)
protected function processParseRules(&$uri, $stage = '')
{
// Process the attached parse rules
$vars = parent::processParseRules($uri);
$vars = parent::processParseRules($uri, $stage);

// Process the pagination support
if ($this->_mode == JROUTER_MODE_SEF)
if ($stage == '')
{
if ($start = $uri->getVar('start'))
// Process the pagination support
if ($this->_mode == JROUTER_MODE_SEF)
{
$uri->delVar('start');
$vars['limitstart'] = $start;
if ($start = $uri->getVar('start'))
{
$uri->delVar('start');
$vars['limitstart'] = $start;
}
}
}

Expand All @@ -557,45 +567,55 @@ protected function processParseRules(&$uri)
/**
* Process the build uri query data based on custom defined rules
*
* @param JUri &$uri The URI
* @param JUri &$uri The URI
* @param string $stage The stage that should be processed.
* Possible values: 'preprocess', 'postprocess'
* and '' for the main build stage
*
* @return void
*
* @since 3.2
* @deprecated 4.0 The special logic should be implemented as rule
*/
protected function processBuildRules(&$uri)
protected function processBuildRules(&$uri, $stage = '')
{
// Make sure any menu vars are used if no others are specified
if (($this->_mode != JROUTER_MODE_SEF) && $uri->getVar('Itemid') && count($uri->getQuery(true)) == 2)
if ($stage == '')
{
// Get the active menu item
$itemid = $uri->getVar('Itemid');
$item = $this->menu->getItem($itemid);

if ($item)
// Make sure any menu vars are used if no others are specified
if (($this->_mode != JROUTER_MODE_SEF) && $uri->getVar('Itemid') && count($uri->getQuery(true)) == 2)
{
$uri->setQuery($item->query);
}
// Get the active menu item
$itemid = $uri->getVar('Itemid');
$item = $this->menu->getItem($itemid);

$uri->setVar('Itemid', $itemid);
if ($item)
{
$uri->setQuery($item->query);
}

$uri->setVar('Itemid', $itemid);
}
}

// Process the attached build rules
parent::processBuildRules($uri);
parent::processBuildRules($uri, $stage);

// Get the path data
$route = $uri->getPath();

if ($this->_mode == JROUTER_MODE_SEF && $route)
if ($stage == '')
{
if ($limitstart = $uri->getVar('limitstart'))
// Get the path data
$route = $uri->getPath();

if ($this->_mode == JROUTER_MODE_SEF && $route)
{
$uri->setVar('start', (int) $limitstart);
$uri->delVar('limitstart');
if ($limitstart = $uri->getVar('limitstart'))
{
$uri->setVar('start', (int) $limitstart);
$uri->delVar('limitstart');
}
}
}

$uri->setPath($route);
$uri->setPath($route);
}
}

/**
Expand Down

12 comments on commit 0e6ca11

@johanjanssens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hackwar Good work here. Some suggestions to make the the code a bit cleaner.

1. Add three constants to the router

const PROCESS_BEFORE = 'preprocess'
const PROCESS_DURING = ''
const PROCESS_AFTER   = 'postprocess'

2. Change attachBuildRule signature

Change the method signature to : public function attachBuildRule($callback, $process = self::PROCESS_DURING); Which is the current behaviour of the method. PROCESS_BEFORE or PROCESS_AFTER can additionally also be used.

3. Change attachParseRule signature

Would change the method signature to : public function attachBuildRule($callback, $process = self::PROCESS_DURING); Which is the current behaviour of the method. PROCESS_BEFORE or PROCESS_AFTER can additionally also be used.

These changes make the API a little bit more readable and self-explanatory.

Hope that helps. Keep up the good work.

@Hackwar
Copy link
Owner Author

@Hackwar Hackwar commented on 0e6ca11 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is, that we would need more than a boolean here. If you can make a boolean a tristate thing... 😉 We have the "buildpreprocess", then the "build" and the "buildpostprocess" stages. For a practical example: The buildpreprocess could make sure that the right language and Itemid (via the component router preprocess method) is set in the query, the build then adds the SEF prefix and runs the component router and puts the right menu path into the URL and the buildpostprocess removes the language query parameter again.

@johanjanssens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the need for a before/after and like the idea. Having 3 places that rules are called is rather complex though. It's also very hard to communicate through an API and interface. Would recommend to reconsider this approach and see if it can be solved with a before/after approach.

@Hackwar
Copy link
Owner Author

@Hackwar Hackwar commented on 0e6ca11 Dec 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is, before and after what? There will be no logic in the application routers besides the rules.
Generating a rule will work the following way: Take the input URL, run it through the preprocessbuild rules, run them through the build rules, run them through the postprocessbuild rules, return the result. That is it. There will be no other code, no transformations of any kind in the router. All the logic will be in the rules and could be replaced if you so wish. A router without any rules will return the same URL that you handed in.

I think that approach is actually pretty easy to understand. I can attach a function to prepare my query and validate it for example. Then I have a way to attach a function so that the query is transformed into something else and then I have a way to attach a function to clean up afterwards.

@johanjanssens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oki, I understand better now what you are trying to do and why things are as they are.

Question :

You referred to the language filter plugin in a previous comment and as an example that uses the 3 stages approach.

Is the language filter plugin the main reason why you implemented the router with a three staged approach ? Or said differently was this change triggered by the solution you choose there, or was it triggered by a more architectural need for flexibility.

I'm just trying to better understand the reasoning behind the change. Thanks!

@Hackwar
Copy link
Owner Author

@Hackwar Hackwar commented on 0e6ca11 Dec 5, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The language filter plugin was a factor in this.

Originally I wanted to have a plugin event when the (component-)router is instantiated and at that point I wanted to add all the rules dynamically and not have any of those hardcoded into the system. With that I mean that the constructor of the router was supposed to trigger a plugin event and wait for any rules to be added and otherwise the router would be stupid/empty.

With that approach, the order in which the plugins are called, would play a vital role in the whole routing thing. Right now I stepped away from the plugin approach, too, and would rather have a bunch of the rules be set by default.

In any case, I tried to solve the issue with the calling order by having a parameter in the attach*Rule() method that either added the new rule to the front or the back of the rules array. That would help a tiny bit with that problem, but not solve it completely.

Then I stumbled over the language plugin, which forced this issue again on me and that is where I decided to have this 3-stages approach. It still is not a fool-proof solution for the calling order issue, but at least it allows you to have groups that are called in a defined order, even though you can't influence the order in the groups themselfs.

@johanjanssens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hannes. A couple of points of final feedback :

Using a correct design pattern

The 3 staged approach your are implementing here is not implemented according to any existing design pattern. For architectural changes to the Joomla core it would advised to choose a proper design pattern. This will make it easier for developer to understand the implementation and communicate how the code works.

In short what you are trying to here is called an Intercepting Filter pattern Each router rule can be implemented as an intercepting filter that can modify the data 'query' being passed.

The problem with an intercepting filter is that it doesn't have priority. This could be solved by augmenting the implementing using a priority queue where each filter is given a priority. Priorities could be named and hardcoded to generate a fix set of phases or stages.

Wrong problem being solved ?

With that said. I'm not convinced your are solving the right problem. You problem definition states that you need to be able to add data to the query before the component router is called, to be able to remove that data after the router has been called to allow a component router to make use of this. Example you give is the SEF prefix.

I'm not convinced this is a problem that needs to be solved. If a router needs to know about the language the system is running in he should use a proper API call to do this. The routers implementation shouldn't be coupled to data that flows through the router.

Component routers should only know about the query information they need to route, all other information if part of the query they should just pass along.

A router should never make decisions based on data being passed in through the query. This is what you are trying to do with the language filter, in essence you are coupling the different layers of the application architecture together through the router.

In Joomla routing works in 3 stages : component -> page -> application Results of each stage are appended to the next which results in following URL http://www.joomla.org/[application]/[page]/[component]

Each stage in this process shouldn't be aware about the previous and the next stage. All each stage does is take relevant data from the query or application context to generate the route, or vice-versa take relevant data from the url to generate the query.

Conclusion

I'm not convinced that your reasoning is sufficient to warrant the change you are proposing in this patch. I'm not convinced component routers should have data injected and removed. Would like to see different ideas discussed before an final implementation is chosen.

@Hackwar
Copy link
Owner Author

@Hackwar Hackwar commented on 0e6ca11 Dec 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Johan, I'm not convinced that you see the whole picture here. Let me try to cover your points:

Considering the missing pattern here: Yes, that might be an issue and I'm open to implementing a better solution. However, please keep in mind that we need to be backwards compatible. I chose this approach because it is something that we already used so far in the router and which developers are familiar with. In theory we could wrap the existing router rules logic in such a filter, but as far as I can see, we can not do this in a backwards compatible way, since the current router is written in a way that would force us to keep both all methods and all calls to methods in the identical call chain. If someone extends the current router and only overrides parseSEFRule() or what's it called, that method still has to be called at the same place with the same arguments, etcetera, etcetera. Backwards compatibility here is a major bitch...

Anyway, for the moment the concept of the rules as described in this PR is a known concept and is backwards compatible, which is my reason for choosing it. BTW: If I understand the description from Oracle correctly, I would know that pattern as a decorator pattern and would actually love to use this for our MVC classes.

Regarding the problem that I'm trying to solve: First of all, the router should only transform the query from one representation to another, regardless if that is parsing or building the query. Right now however, we have at least 3 issues:

  1. A fixed pipeline that only allows to add your own code at one place, with the major part (component router) happening AFTER that part.
  2. Crappy code quality that means that we can get almost anything handed over as a query to process. (see ContentHelperRoute::getArticleRoute needs to be called with catid and language joomla/joomla-cms#5276)
  3. Backwards compatibility.

As long as the shit that is coming from our own code is not guaranteed to be a correct URL, we need some way to clean this up. That is the preprocess stage for. At the same time, if we want to do this properly with the language lookup for the Itemid for example, we need to have the current language still available for the component router, which we currently don't have. Since I really, REALLY, don't want to parse back the segments in the component router to the "correct" language, we need that as part of the query. Since that again needs to be removed from the final URL, we need a stage to clean up our query and for example rename limitstart to start and such. And there is no way to call the "correct" language via an API call, because the language is query related. If I'm on an english page and want to generate the link to its french translation, I can not ask Joomla for the current language. That has to be part of the query, otherwise Joomla will happily report to me "I'm currently an english site".

Right now I'm still convinced that this is a good approach and that it fullfills my criterias of "known construct to J devs", "backwards compatible" and "lightweight". For Joomla 4.0, we can go a different route, but for 3.x, this seems like the only reasonable way to solve this.

@johanjanssens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Hannes,

Thanks for the clarifications, very much appreciated; It's indeed not easy to see the whole picture while not being in the middle of the code as you are. Trying my best.

About the chosen approach

I do agree 100% with you that backwards compatibility is key. No argument what so ever from me there. I'm just always trying to ask myself what the most OO and pattern based approach for a problem is. Reading your explanation I can see that there is not much wiggle room here.

I would however not mark this implementation as 3.x only. We both know that 4.0 is something that is high up in the air. Best to try and make this implementation as rock solid as possible under the assumption it will be there for a long time to come.

About the intercepting filter pattern

The pattern is often used to do pre and post filtering in an MVC controller context yes. Pre filter the request and post filter the response. This is also how it's implement in Nooku. There is a difference between a decorator and an intercepting filter though, and intercepting filter is not a decorator but it can be implemented into an object at runtime using one. A good read on the topic http://msdn.microsoft.com/en-us/library/ff647251.aspx

I have updated my initial comment with an approach that allows for tri-states and hides a bit more the complexity while being more declarative. Have a look and let me know what you think.

@johanjanssens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final comment. I would consider deprecating the

const JROUTER_MODE_RAW = 0;
const JROUTER_MODE_SEF = 1;

for 4.0 too. If i understand your approach correctly they won't be needed anymore anyway, and it would be good thing to mark them both deprecated.

@Hackwar
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @johanjanssens,
I will implement your proposal. I agree that the consts should be deprecated, too.

@johanjanssens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hannes! Happy wit that.

About the consts, they where ever only intended for 1.5 as a BC mechanism between non SEF and SEF. We always intended RAW to be removed in 2.0. Would be good if we can finally get rid of them.

Please sign in to comment.