New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Routing: Implementing routing stages #5264

Closed
wants to merge 15 commits into
base: staging
from

Conversation

Projects
None yet
9 participants
@Hackwar
Member

Hackwar commented Nov 30, 2014

The issue

The current system only allows to modify the URL before it is send to the component router. This means that for example the language filter plugin has to add the SEF prefix before the component router is even started and also has to delete the language value from the URL before that time. That means, that that data is not available for the component router. This is a more general problem that our routing system is very inflexible.

The proposal

This PR implements a 3-stage processing for the URLs: preprocessing, main stage and postprocessing. This allows the language filter plugin to add the SEF prefix in the preprocessing stage and to delete the language query parameter in the postprocessing stage, making that data available to the component router in between. At the same time, this introduces deprecation notes for all the methods that should be handled as rules in this 3-stage process.

Backwards Compatibility

This PR should be backwards compatible. There are 4 methods that have been changed, the methods to attach rules and the methods to process these rules. Since the new, second parameter is optional, existing code should not create any issues. Existing rules are still stored in their respective arrays and only the pre- and postprocessing steps add new keys to that array, which would otherwise be ignored. That said, no old software should be affected by these changes. The position of the main stage for parse and build, which represents the rules that we have so far, does not change either, which should prevent any changes in our intermediate results.

Personal thoughts

All the code that is in the methods around these process*Rules methods should go into a rule and be pushed into the specific stage. The problem is that that change WILL change the way our routers behave towards third party software. That is most likely a break in backwards compatibility, which would force us to postpone that step until Joomla 4.0, which frustrates me immensely...
Another thing that bugs me personally is, that we call the main stage '' in the method parameters, which looks bad. However that makes the nicest implementation from my perspective.

How to test

Testing here means that nothing changes between applying these changes. So please first check that your site works fine, then apply the change and see that it still works fine.

This PR needs a review by a maintainer.

This was made possible through the generous donation of the people mentioned in the following link via an Indiegogo campaign: http://joomlager.de/crowdfunding/5-contributors

@Bakual

This comment has been minimized.

Contributor

Bakual commented Dec 1, 2014

14 errors in unit tests and some minor codestyle issues.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 1, 2014

Sorry, the majority of those errors was a typo, a missing +. Now everything should be fine.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 1, 2014

BTW: The PHP 5.6 tests throw a notice before the test is started. Something is wrong there... (Unrelated to this PR)

@smanzi

This comment has been minimized.

smanzi commented Dec 1, 2014

Hello Hannes, thanks for delivering this!

I've yet to test it and even to look at the code, but just reading your description, and especially your frustration for having to drag behind the weight of the B/C anchor, made me think something:

Wouldn't it be possible to introduce in com_config "experimental" switches for testing new (possibly B/C breaking) upcoming technologies? Those switches should be accompanied by humongous warnings stating that they are meant only for testing purposes, that most 3rd party extensions will not work when they are turned on and that there is no firm commitment that the functionality will be implemented in upcoming official releases.

This will allow 3rd party extensions developer to prepare for "the future" and core developer to receive an early feedback...

Just my 2 (probably O.T.) cents...

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 1, 2014

Hi @smanzi, that would indeed be a possibility, but I would strongly advise against it. There are 4 reasons against this:

  1. Joomla already has too many options and actually needs to cut down on those. Users will be confused for sure with further options. They are already unable to cope with the FTP settings and we have way to many people that have that set, even though they don't need it and they don't have an idea why they did it.
  2. At the same time, it means that we have to maintain 2 separate code blocks and have to find ways to elegantly wove this into our system. Should the new system have the same class names as the old one? Which one gets put into the default position and which solution overrides the other system? You will have such discussions for each and every change, in addition to our current discussions.
  3. Third party developers will have a nightmare to support all this. Considering the routing, a third party developer might say that he wants to use the new routing system, since it is far less code to write and its easier to understand. That means a user that wants to use that extension, needs to enable the new routing system. Now if for some reason, the old and new system are incompatible and the user wants to use an extension that only supports the old routing, he is screwed. That means that every third party developer has to cater for both situations, old and new, actually increasing the work instead of reducing it.
  4. Our project is notoriously incapable of supporting 2 branches of a code at the same time. A good example is/was JRegistry and JParameter. JParameter was one of the corner stones of Joomla 1.5 and was made obsolete in 1.6 by JForm and using JRegistry. Only after 1.6 was released did someone notice that JParameter has not been touched for 2 years and did not work anymore.

Based on these points, I would not introduce such options as a general measure. Especially the first point is important to me. Compare Joomla and how much options Joomla provides simply when writing an article compared to Wordpress. Yes, the two systems are very different, but having several magnitudes more options than a system that aims at the same market, does not throw the best light on J.

@smanzi

This comment has been minimized.

smanzi commented Dec 1, 2014

@Hackwar To be honest I agree only with your points 2 (strongly agree) and 4 (possibly agree).

Solutions for the other points:

  • the switches could be totally hidden (no UI for setting them, just tinker with configuration.php or a separate "experimental.php" config file)
  • In no way 3rd party developers should deliver officially supported (by them, I mean) code that assumes any of such experimental switches is turned on.

On the other hand I concur that smoothly integrating two "lines of code execution" in the core can be quite a mess and will most likely imply altering the "current line of code".

I see this as something that should be reserved for exceptional cases, like probably a "new router" is, not something to be used for trivial alternatives.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 1, 2014

The router is by 80% aimed at third party developers, since it provides them a way to write a router in 5 lines of code and they don't get a serious headache just because of that aspect of their component. There would be little gained if third party devs did not adopt this code as quickly as possible.

@smanzi

This comment has been minimized.

smanzi commented Dec 1, 2014

So... how do you see the future? An "early" 4.0 (will allow breaking B/C) or the frustration of having at hand fantastic code ... that can't be used?

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 1, 2014

First of all, there are ways to get what we want right now without breaking B/C. And then I don't know what you mean with an "early" 4.0. Nothing is promised for 4.0. If the PLT decides to start work on 4.0 after 3.4 is released, so be it. 4.0 could mean that we have this router and some other minor details and that is it. Or it means something completely different.

@smanzi

This comment has been minimized.

smanzi commented Dec 1, 2014

by "early 4.0" I was meaning exactly what you said: "this router and some other minor details". Well, maybe not just minor ones: I don't have examples at hand right now, but I would say everything that we are eager to do but can't because of the B/C commitment

@smanzi

This comment has been minimized.

smanzi commented Dec 1, 2014

but... can you elaborate on the concept of "there are ways to get what we want right now without breaking B/C"?

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 1, 2014

These stages things for example are a way to rewrite the current router to a rule based concept which is backwards compatible in a lot of ways. There are also questions how far our backwards compatibility goes. If we for example said that non-SEF URLs are not covered by that backwards compatibility claim, then we could do different things while still be part of 3.x.

@smanzi

This comment has been minimized.

smanzi commented Dec 1, 2014

@Hackwar I want to test this: should I apply #5105 too?

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 1, 2014

You can test this without #5105. 5105 however needs testing, too. 😉

@smanzi

This comment has been minimized.

smanzi commented Dec 1, 2014

got it! 😜

@smanzi

This comment has been minimized.

smanzi commented Dec 1, 2014

@test success

  • Applying this patch doesn't seems to have any negative impact with the tested component (com_content, several views)
  • SEF URLs exactly as before (which is a mixed blessing, IMHO....)
  • NON SEF URLs same as before

Question: should this be tested with SEF engines (like e.g. sh404) for compatibility or is this guaranteed to be B/C in this context?

Personal note: ehmm... are you aware many of those who crowdfunded your initiative are expecting you to get rid of the ArticleID from SEF URLs, aren't you? I Understand this is not acceptable in the core because of B/C, but do you plan to release a system plugin (or anything else) to achieve that?

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 1, 2014

Regarding sh404sef: Old code will be backwards compatible, but if you are using code written for whatever Joomla version this one will be released in, you might get conflicts. If sh404sef does not use the core router and instead deploys its own stuff, they would be disabling these stages effectively.

Regarding your personal note: This is a place where I plan to add an option to disable or enable certain features of the routing system, which would include the IDs. So it will be part of Joomla and it will be in in a backwards compatible way.

@smanzi

This comment has been minimized.

smanzi commented Dec 1, 2014

Perfect! 😄

@mbabker

This comment has been minimized.

Member

mbabker commented Dec 1, 2014

I found an older version of sh404SEF in my filesystem and quickly glanced through its code to find its router integration. From what I could tell, they're extending JRouterSite but none of the methods or vars changed in this URL are in their extended class, so this shouldn't break compatibility with their code from that aspect. @shumisha if you'd like to test this and confirm that things will continue to work well, it would be greatly appreciated.

In general with regards to backward compatibility, we can't control what happens if an extension is extending a library class and we change the internals of a method to extend what it is able to do. If the base API continues to behave the same pre- and post- change, then to me it is a B/C change. Unfortunately, once code is rewritten to make use of those newer features, that would cause an arguable B/C break if the extended class isn't updated as well. It's a risk we take any time we change the internals of a class or method to add new features.

@weeblr-dev

This comment has been minimized.

weeblr-dev commented Dec 1, 2014

Hi all,
Thanks Michael for the head up. I don't have time right now to get to the details of this, hopefully tomorrow. I can make the following very quick comments:

1 - strictly about sh404SEF, I do extend JRouterSite, but to be honest that's not really needed and I can probably put proxies in place if some methods I need/use were changed, which I doubt. sh404SEf is really API-compliant in that matter and only uses the additional rules it attaches to the site router.

2 - Foreseeable issues may more likely be if the routing system changes its behavior. LanguageFilter plugin is a good example. If Hannes suggestion to change it's behavior to NOT set/unset the language code were applied, that is broken between a pre-processing and a post-processing stage, then the data received by whatever routing rules are added will be different from what it is today. LanguageFilter routing has been causing much issues, and all SEF extensions (except sh404SEF) simply disable it for multilingual sites (and replicate its features elsewhere).
I have found some logic to keep it enabled and still be able to properly parse urls, but again I would expect that to break if the data provided to me is changed.

As Michael outlined, I think a B/C break would happen if changes were made to the language filter plugin routing rules to take advantage of this. Otherwise, we should be good and I'm sure I can accommodate that PR, maybe without having to do anything actually. If the LF plugin is modified in such a way however, then I think that should not happen before 4.0

I'll get back to this post in the coming days with some test results.

@Hackwar I think something that could be quite useful would be to keep and make accessible by all parsing or building rules the original input data (ie the original $uri) before it's been modified by any of the attached rules or the component itself. One reason for the difficulties with the language filter plugin is what you outlined, ie the language information is lost on the other parser/builder after the LF plugin has handled it.
So instead/on top relying on the LF to only remove the query string (when building) at the postpreocessing stage, it'd be very nice to simply store the original data in a place where any parsing rule can access it.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 1, 2014

Hi @weeblr-dev, I've been working on the language filter plugin in PR #5140. Please look there, too.

Regarding storing the original data somewhere: I'm a bit hesitant regarding this. I personally decided against it, since I think it is not only a clean way to clean up after you, but also a way to override something. It allows me to prevent the default code from working on this stuff by removing it from the query. But if there is a majority to implement something like this, I would not fight for dear life. 😉

@smanzi

This comment has been minimized.

smanzi commented Dec 3, 2014

Hannes, I've been testing with your PRs #5140, #5264 and #5276 in a multilingual environment and I have some results I would like to share with you hoping that you find them useful.
I'm posting the results here as this seems to be "the mother of all PRs": sorry if I'm wrong.

The test have been performed using:

2 languages:

  • English en-GB en (default)
  • Italiano it-IT it

3 menu structures

  • Main Menu 1 item: home (Featured Articles, default home for all)
  • Italiano 1 item: pagina-principale (Featured Articles, home for it)
  • English 1 item: home-page (Featured Articles, home for en)

3 categories

  • uncategorized (all) ID=2
  • test (en) ID=8
  • prova (it) ID=9

2 articles (both flagged as featured)

  • foobar (en, in test, ID=1)
  • casino (it, in prova, ID=2)

menu items, categories, and articles have been associated between themselves

"Remove URL Language Code" (for default language) is set to: No

Multilingual Status is:

multilingualstatus

From the above structure I had expected the following URLs to be valid and generated:

/en/
/en/8-test/1-fubar
/it/
/it/9-prova/2-casino

What I get is instead:

/en/
/en/home-page/8-test/1-fubar
/it/
/it/pagina-principale/9-prova/2-casino

i.e.: the aliases of the home pages are not stripped away

Language switcher does his duty, no problem.

The "expected URLs" do works if entered manually, but the bad news are that also "chimeric" URLs do work, like e.g.:

/it/home-page/8-test/2-casino
/en/pagina-principale/9-prova/1-fubar
/it/pagina-principale/8-test/2-casino
etc...

While others do give a 404, like:

/it/home-page/8-test/1-fubar
/en/pagina-principale/9-prova/2-casino

The discriminating factor seems to be that the language code must match the language of the item requested (disregarding categories and menu items in between)

P.S: Of course the above has been performed with the latest 'staging' at the time of this writing (head at b858931), integrated with the 3 above PRs.
I've been unable to integrate #5285 and did not tried integrating #5294 & #5295

@johanjanssens

This comment has been minimized.

johanjanssens commented on 0e6ca11 Dec 3, 2014

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

This comment has been minimized.

Owner

Hackwar replied Dec 3, 2014

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.

This comment has been minimized.

johanjanssens replied Dec 3, 2014

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.

This comment has been minimized.

Owner

Hackwar replied Dec 3, 2014

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.

This comment has been minimized.

johanjanssens replied Dec 5, 2014

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!

This comment has been minimized.

Owner

Hackwar replied Dec 6, 2014

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

This comment has been minimized.

johanjanssens replied Dec 12, 2014

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.

This comment has been minimized.

johanjanssens replied Dec 12, 2014

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.

This comment has been minimized.

Owner

Hackwar replied Dec 12, 2014

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

This comment has been minimized.

johanjanssens replied Dec 12, 2014

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.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 8, 2014

fixed

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 12, 2014

Ok, I implemented the changes that Johan proposed. Could we then get a review by a maintainer now and merge this stuff? 😉

@johanjanssens

This comment has been minimized.

Contributor

johanjanssens commented Dec 16, 2014

@Hackwar Thanks looking good.

One final remark. I'm not sure about the additional class properties that have been added without the underscore. I understand that in a future version the properties should not be underscored but adding them without using them makes little sense ?

Proposal

What about using the correct properties and removing the underscored ones and then dynamically assigning them again by reference when the object is instantiated ?

Example : $this->_vars =& $this->vars;

This should work, I think.

The benefit of this is that you don't need to add unused properties to the class definition. Keeping things nice and tidy. Removing the legacy support also becomes easier later.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 17, 2014

Hi @johanjanssens, those properties are not added by me. That is stuff from 1.6 times, I think. In any case, the by-reference-trick will only work as long as no one assigns a new array to that property. I agree that we should get rid of that old crap, but I fear that it wont be possible before 4.0. Feel free to duke this out with the PLT. 😉

@johanjanssens

This comment has been minimized.

Contributor

johanjanssens commented Dec 25, 2014

@Hackwar You are right sorry, forgot to check the blame. This really smells bad. But if it was there, let's leave it there.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 29, 2014

I updated the unittests to test properly for the new routing stages.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jan 5, 2015

I had to change the order in which the methods are called in the router to solve a logical error that I made. So far the order was:

  • Preprocess build rules
  • Process build rules
  • Call preprocess() of the component router
  • Call build() of the component router
  • Postprocess build rules

This would not allow the language system and the Itemid-lookup to work correctly and since the Itemid lookup should be pulled into the router, all plugins depending on the Itemid to be present in the buildRules step would fail subsequently, so I had to reorder this to the following:

  • Preprocess build rules
  • Call preprocess() of the component router
  • Process build rules
  • Call build() of the component router
  • Postprocess build rules

It should be noted that the first order (preprocess() after buildRules) has been shipped with 3.3.6. So this would technically be a break in backwards compatibility if we change the order now. However, this requires that code in the preprocess() component router step has to rely on data from the buildRules step to even create an issue. This again means, that someone had to implement this so far undocumented and in the core unused feature. I would take the chance here and change this, especially since otherwise none of my routing changes can be merged before Joomla 4.0.

@Bakual

This comment has been minimized.

Contributor

Bakual commented Jan 5, 2015

Imho we can take that B/C risk since I doubt anyone already used that new method. It would mean that they built an extension with a minimum requirement of J3.3.6 but they didn't really gain anything out of it.
Also there is a really slim chance if someone is using it, that the order actually matters.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jan 5, 2015

Could we then merge this stuff?

@infograf768

This comment has been minimized.

Member

infograf768 commented Jan 7, 2015

@test
I did not get any issue on my multilingual test site.

@mbabker mbabker closed this Jan 9, 2015

mbabker added a commit that referenced this pull request Jan 9, 2015

@mbabker mbabker added this to the Joomla! 3.4.0 milestone Jan 9, 2015

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

This comment has been minimized.

@johanjanssens

johanjanssens Jan 11, 2015

Contributor

@Hackwar For code readability sake it would change this to $vars += $this->_processParseRules($uri, self::PROCESS_DURING);

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

This comment has been minimized.

@johanjanssens

johanjanssens Jan 11, 2015

Contributor

@Hackwar For code readability sake it would change this to $vars += $this->_processBuildRules($uri, self::PROCESS_DURING);

@johanjanssens

This comment has been minimized.

Contributor

johanjanssens commented Jan 12, 2015

@Hackwar The changes order for the build rules makes total sense and it more logical. Should the same change be made for the parse rules too to keep the implementation consistent ?

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jan 12, 2015

There is no preprocess step for the parsing for component routers, so there is nothing that we could change there. 😉

@johanjanssens

This comment has been minimized.

Contributor

johanjanssens commented Jan 13, 2015

True. All good then.

@Hackwar Hackwar deleted the Hackwar:router_stages branch Jan 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment