Skip to content
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

Optimizations for JRoute and JRouter 2. Try #3164

Closed
wants to merge 4 commits into from
Closed

Conversation

@Hackwar
Copy link
Member

@Hackwar Hackwar commented Feb 22, 2014

This is a new PR against staging instead of master. The original one can be found here: #2487
Optimizing JRoute

  • Allow associative arrays instead of string URIs
  • Allow routers to set the URI to HTTPS from the inside
  • some performance optimizations

Optimizing JRouter

  • optimizing code in JRouter::build()
  • introducing a runtime cache to only process a URI once per runtime
@dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Feb 23, 2014

Code is up and running here http://www.casaecampo.gr. Nginx, php 5.4, Joomla! 3.2.2 Stable [ Ember ], Multilanguage, sef enabled

All good!

@chivitli
Copy link
Contributor

@chivitli chivitli commented Feb 26, 2014

Multilingual test which was previously reported to fail now passed in my testing

{
$prefix = $uri->toString(array('host', 'port'));
$uri2 = JURI::getInstance();

This comment has been minimized.

@b2z

b2z Feb 28, 2014
Member

JURI should be changed to JUri ;)

@@ -215,6 +230,8 @@ public function build($url)
{
$this->_buildSefRoute($uri);
}

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

This comment has been minimized.

@dongilbert

dongilbert Feb 28, 2014
Contributor

clone is a language construct, and as such, does not require parenthesis. I know there's nothing specifically in the Code Style Guide addressing this, so it's not a merge stopper. However, the style guide does state that language constructs like include and require shouldn't have parenthesis. http://joomla.github.io/coding-standards/?coding-standards/chapters/php.md

@dongilbert
Copy link
Contributor

@dongilbert dongilbert commented Feb 28, 2014

👍

@betweenbrain
Copy link
Contributor

@betweenbrain betweenbrain commented Feb 28, 2014

As reported on the tracker, tests good here 👍

@@ -80,6 +80,14 @@ class JRouter
'build' => array(),
'parse' => array()
);

This comment has been minimized.

@betweenbrain

betweenbrain Feb 28, 2014
Contributor

Seems to be an arrant tab indentation on this line. Can you remove it?

{
return clone($this->cache[$key]);
}

This comment has been minimized.

@betweenbrain

betweenbrain Feb 28, 2014
Contributor

Same indentation issue here (two)

@@ -215,6 +230,8 @@ public function build($url)
{
$this->_buildSefRoute($uri);
}

This comment has been minimized.

@betweenbrain

betweenbrain Feb 28, 2014
Contributor

ditto (tabs)

{
$uri = new JUri('index.php');
$uri->setQuery($url);
return $uri;

This comment has been minimized.

@betweenbrain

betweenbrain Feb 28, 2014
Contributor

Please consider an empty line before the return statement

@JoomliC
Copy link
Contributor

@JoomliC JoomliC commented Feb 28, 2014

@test successful with core, and with my own component and modules too.

@Hackwar
Copy link
Member Author

@Hackwar Hackwar commented Mar 1, 2014

fixed the codestyle issues

* Caching of processed URIs
*
* @var array
* @since 3.5

This comment has been minimized.

@oc666

oc666 Mar 1, 2014
Contributor

Since 3.3?

This comment has been minimized.

@Hackwar

Hackwar Mar 2, 2014
Author Member

From a time long long gone, where 3.1 was supposed to be the last STS and 3.5 the next release and an LTS. ;-)

@dbhurley
Copy link
Contributor

@dbhurley dbhurley commented Mar 2, 2014

👍

@dongilbert
Copy link
Contributor

@dongilbert dongilbert commented Mar 3, 2014

Thanks Hannes

mbabker added a commit that referenced this pull request Mar 3, 2014
@Bakual
Copy link
Contributor

@Bakual Bakual commented Mar 3, 2014

Closing as merged into 3.3-dev branch.

@Bakual Bakual closed this Mar 3, 2014
Bakual added a commit to Bakual/joomla-cms that referenced this pull request May 12, 2014
@Hackwar Hackwar deleted the Hackwar:jroute2 branch Jan 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet