Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
- Bug yiisoft#13350: Fixed bug with incorrect caching of `yii\web\UrlRule::createUrl()` results in `yii\web\UrlManager`.
- Bug yiisoft#14094: Fixed bug when single `yii\web\UrlManager::createUrl()` call my result multiple calls of `yii\web\UrlRule::createUrl()` for the same rule.
- Enh yiisoft#11288: Added support for caching of `yii\web\UrlRule::createUrl()` results in `yii\web\UrlManager` for rules with defaults.
  • Loading branch information
rob006 authored and samdark committed May 10, 2017
1 parent 5ed6910 commit daa8b67
Show file tree
Hide file tree
Showing 12 changed files with 497 additions and 39 deletions.
3 changes: 3 additions & 0 deletions framework/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Yii Framework 2 Change Log
- Bug #13306: Wildcard in `reloadableScripts` in `yii.js` allows 0 characters (arogachev)
- Bug #13340: Fixed `yii\db\Connection::useMaster()` - exception within callback completely disables slaves (Vovan-VE)
- Bug #13343: Fixed `yii\i18n\Formatter::asTime()` to process time-only values without time zone conversion (bizley)
- Bug #13350: Fixed bug with incorrect caching of `yii\web\UrlRule::createUrl()` results in `yii\web\UrlManager` (rob006)
- Bug #13362: Fixed return value of `yii\caching\MemCache::setValues()` (masterklavi)
- Bug #13379: Fixed `applyFilter()` function in `yii.gridView.js` to work correctly when params in `filterUrl` are indexed (SilverFire, arogachev)
- Bug #13418: Fixed `QueryBuilder::batchInsert()` if `$rows` is `\Generator` (lav45)
Expand Down Expand Up @@ -53,7 +54,9 @@ Yii Framework 2 Change Log
- Bug #14033: Fixed `yii\filters\AccessRule::matchIp()` erroring in case IP is not defined under HHVM (Kolyunya)
- Bug #14052: Fixed processing parse errors on PHP 7 since these are instances of `\ParseError` (samdark)
- Bug #14074: Fixed default value of `yii\console\controllers\FixtureController::$globalFixtures` to contain valid class name (lynicidn)
- Bug #14094: Fixed bug when single `yii\web\UrlManager::createUrl()` call my result multiple calls of `yii\web\UrlRule::createUrl()` for the same rule (rossoneri)
- Enh #8641: Enhanced `yii\console\Request::resolve()` to prevent passing parameters, that begin from digits (silverfire)
- Enh #11288: Added support for caching of `yii\web\UrlRule::createUrl()` results in `yii\web\UrlManager` for rules with defaults (rob006)
- Enh #12528: Added option to disable query logging and profiling in DB command (cebe)
- Enh #13144: Refactored `yii\db\Query::queryScalar()` (Alex-Code)
- Enh #13179: Added `yii\data\Sort::parseSortParam` allowing to customize sort param in descendant class (leandrogehlen)
Expand Down
4 changes: 4 additions & 0 deletions framework/UPGRADE.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ Upgrade from Yii 2.0.11

* The signature of `yii\cache\Cache::getOrSet()` has been adjusted to also accept a callable and not only `Closure`.
If you extend this method, make sure to adjust your code.

* `yii\web\UrlManager` now checks if rules implement `getCreateUrlStatus()` method in order to decide whether to use
internal cache for `createUrl()` calls. Ensure that all your custom rules implement this method in order to fully
benefit from the acceleration provided by this cache.

* `yii\filters\AccessControl` now can be used without `user` component.
In this case `yii\filters\AccessControl::denyAccess()` throws `yii\web\ForbiddenHttpException` and using `AccessRule`
Expand Down
27 changes: 18 additions & 9 deletions framework/rest/UrlRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
use yii\base\InvalidConfigException;
use yii\helpers\Inflector;
use yii\web\CompositeUrlRule;
use yii\web\UrlRule as WebUrlRule;
use yii\web\UrlRuleInterface;

/**
* UrlRule is provided to simplify the creation of URL rules for RESTful API support.
Expand Down Expand Up @@ -187,7 +189,7 @@ protected function createRules()
* @param string $pattern
* @param string $prefix
* @param string $action
* @return \yii\web\UrlRuleInterface
* @return UrlRuleInterface
*/
protected function createRule($pattern, $prefix, $action)
{
Expand All @@ -204,7 +206,7 @@ protected function createRule($pattern, $prefix, $action)
$config['pattern'] = rtrim($prefix . '/' . strtr($pattern, $this->tokens), '/');
$config['route'] = $action;
if (!empty($verbs) && !in_array('GET', $verbs)) {
$config['mode'] = \yii\web\UrlRule::PARSING_ONLY;
$config['mode'] = WebUrlRule::PARSING_ONLY;
}
$config['suffix'] = $this->suffix;

Expand All @@ -220,13 +222,13 @@ public function parseRequest($manager, $request)
foreach ($this->rules as $urlName => $rules) {
if (strpos($pathInfo, $urlName) !== false) {
foreach ($rules as $rule) {
/* @var $rule \yii\web\UrlRule */
/* @var $rule WebUrlRule */
$result = $rule->parseRequest($manager, $request);
if (YII_DEBUG) {
Yii::trace([
'rule' => method_exists($rule, '__toString') ? $rule->__toString() : get_class($rule),
'match' => $result !== false,
'parent' => self::className()
'parent' => self::className(),
], __METHOD__);
}
if ($result !== false) {
Expand All @@ -244,17 +246,24 @@ public function parseRequest($manager, $request)
*/
public function createUrl($manager, $route, $params)
{
$this->createStatus = WebUrlRule::CREATE_STATUS_SUCCESS;
foreach ($this->controller as $urlName => $controller) {
if (strpos($route, $controller) !== false) {
foreach ($this->rules[$urlName] as $rule) {
/* @var $rule \yii\web\UrlRule */
if (($url = $rule->createUrl($manager, $route, $params)) !== false) {
return $url;
}
/* @var $rules UrlRuleInterface[] */
$rules = $this->rules[$urlName];
$url = $this->iterateRules($rules, $manager, $route, $params);
if ($url !== false) {
return $url;
}
} else {
$this->createStatus |= WebUrlRule::CREATE_STATUS_ROUTE_MISMATCH;
}
}

if ($this->createStatus === WebUrlRule::CREATE_STATUS_SUCCESS) {
// create status was not changed - there is no rules configured
$this->createStatus = WebUrlRule::CREATE_STATUS_PARSING_ONLY;
}
return false;
}
}
67 changes: 63 additions & 4 deletions framework/web/CompositeUrlRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ abstract class CompositeUrlRule extends Object implements UrlRuleInterface
* This property is set in [[init()]] by the return value of [[createRules()]].
*/
protected $rules = [];
/**
* @var int|null status of the URL creation after the last [[createUrl()]] call.
* @since 2.0.12
*/
protected $createStatus;


/**
Expand All @@ -46,7 +51,7 @@ public function init()
public function parseRequest($manager, $request)
{
foreach ($this->rules as $rule) {
/* @var $rule \yii\web\UrlRule */
/* @var $rule UrlRule */
$result = $rule->parseRequest($manager, $request);
if (YII_DEBUG) {
Yii::trace([
Expand All @@ -68,13 +73,67 @@ public function parseRequest($manager, $request)
*/
public function createUrl($manager, $route, $params)
{
foreach ($this->rules as $rule) {
/* @var $rule \yii\web\UrlRule */
if (($url = $rule->createUrl($manager, $route, $params)) !== false) {
$this->createStatus = UrlRule::CREATE_STATUS_SUCCESS;
$url = $this->iterateRules($this->rules, $manager, $route, $params);
if ($url !== false) {
return $url;
}

if ($this->createStatus === UrlRule::CREATE_STATUS_SUCCESS) {
// create status was not changed - there is no rules configured
$this->createStatus = UrlRule::CREATE_STATUS_PARSING_ONLY;
}
return false;
}

/**
* Iterates through specified rules and calls [[createUrl()]] for each of them.
*
* @param UrlRuleInterface[] $rules rules to iterate.
* @param UrlManager $manager the URL manager
* @param string $route the route. It should not have slashes at the beginning or the end.
* @param array $params the parameters
* @return bool|string the created URL, or `false` if none of specified rules cannot be used for creating this URL.
* @see createUrl()
* @since 2.0.12
*/
protected function iterateRules($rules, $manager, $route, $params)
{
/* @var $rule UrlRule */
foreach ($rules as $rule) {
$url = $rule->createUrl($manager, $route, $params);
if ($url !== false) {
$this->createStatus = UrlRule::CREATE_STATUS_SUCCESS;
return $url;
}
if (
$this->createStatus === null
|| !method_exists($rule, 'getCreateUrlStatus')
|| $rule->getCreateUrlStatus() === null
) {
$this->createStatus = null;
} else {
$this->createStatus |= $rule->getCreateUrlStatus();
}
}

return false;
}

/**
* Returns status of the URL creation after the last [[createUrl()]] call.
*
* For multiple rules statuses will be combined by bitwise `or` operator
* (e.g. `UrlRule::CREATE_STATUS_PARSING_ONLY | UrlRule::CREATE_STATUS_PARAMS_MISMATCH`).
*
* @return null|int Status of the URL creation after the last [[createUrl()]] call. `null` if rule does not provide
* info about create status.
* @see $createStatus
* @see http://php.net/manual/en/language.operators.bitwise.php
* @since 2.0.12
*/
public function getCreateUrlStatus()
{
return $this->createStatus;
}
}
1 change: 1 addition & 0 deletions framework/web/GroupUrlRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ public function createUrl($manager, $route, $params)
if ($this->routePrefix === '' || strpos($route, $this->routePrefix . '/') === 0) {
return parent::createUrl($manager, $route, $params);
} else {
$this->createStatus = UrlRule::CREATE_STATUS_ROUTE_MISMATCH;
return false;
}
}
Expand Down
42 changes: 31 additions & 11 deletions framework/web/UrlManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public function parseRequest($request)
Yii::trace([
'rule' => method_exists($rule, '__toString') ? $rule->__toString() : get_class($rule),
'match' => $result !== false,
'parent' => null
'parent' => null,
], __METHOD__);
}
if ($result !== false) {
Expand Down Expand Up @@ -371,19 +371,19 @@ public function createUrl($params)
}

$url = $this->getUrlFromCache($cacheKey, $route, $params);

if ($url === false) {
$cacheable = true;
/* @var $rule UrlRule */
foreach ($this->rules as $rule) {
/* @var $rule UrlRule */
if (!empty($rule->defaults) && $rule->mode !== UrlRule::PARSING_ONLY) {
// if there is a rule with default values involved, the matching result may not be cached
$cacheable = false;
if (in_array($rule, $this->_ruleCache[$cacheKey], true)) {
// avoid redundant calls of `UrlRule::createUrl()` for rules checked in `getUrlFromCache()`
// @see https://github.com/yiisoft/yii2/issues/14094
continue;
}
$url = $rule->createUrl($this, $route, $params);
if ($this->canBeCached($rule)) {
$this->setRuleToCache($cacheKey, $rule);
}
if (($url = $rule->createUrl($this, $route, $params)) !== false) {
if ($cacheable) {
$this->setRuleToCache($cacheKey, $rule);
}
if ($url !== false) {
break;
}
}
Expand Down Expand Up @@ -427,6 +427,26 @@ public function createUrl($params)
}
}

/**
* Returns the value indicating whether result of [[createUrl()]] of rule should be cached in internal cache.
*
* @param UrlRuleInterface $rule
* @return bool `true` if result should be cached, `false` if not.
* @since 2.0.12
* @see getUrlFromCache()
* @see setRuleToCache()
* @see UrlRule::getCreateUrlStatus()
*/
protected function canBeCached(UrlRuleInterface $rule)
{
return
// if rule does not provide info about create status, we cache it every time to prevent bugs like #13350
// @see https://github.com/yiisoft/yii2/pull/13350#discussion_r114873476
!method_exists($rule, 'getCreateUrlStatus') || ($status = $rule->getCreateUrlStatus()) === null
|| $status === UrlRule::CREATE_STATUS_SUCCESS
|| $status & UrlRule::CREATE_STATUS_PARAMS_MISMATCH;
}

/**
* Get URL from internal cache if exists
* @param string $cacheKey generated cache key to store data.
Expand Down
49 changes: 49 additions & 0 deletions framework/web/UrlRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,32 @@ class UrlRule extends Object implements UrlRuleInterface
* Set [[mode]] with this value to mark that this rule is for URL creation only
*/
const CREATION_ONLY = 2;
/**
* Represents the successful URL generation by last [[createUrl()]] call.
* @see $createStatus
* @since 2.0.12
*/
const CREATE_STATUS_SUCCESS = 0;
/**
* Represents the unsuccessful URL generation by last [[createUrl()]] call, because rule does not support
* creating URLs.
* @see $createStatus
* @since 2.0.12
*/
const CREATE_STATUS_PARSING_ONLY = 1;
/**
* Represents the unsuccessful URL generation by last [[createUrl()]] call, because of mismatched route.
* @see $createStatus
* @since 2.0.12
*/
const CREATE_STATUS_ROUTE_MISMATCH = 2;
/**
* Represents the unsuccessful URL generation by last [[createUrl()]] call, because of mismatched
* or missing parameters.
* @see $createStatus
* @since 2.0.12
*/
const CREATE_STATUS_PARAMS_MISMATCH = 4;

/**
* @var string the name of this rule. If not set, it will use [[pattern]] as the name.
Expand Down Expand Up @@ -96,6 +122,11 @@ class UrlRule extends Object implements UrlRuleInterface
* @since 2.0.10
*/
public $normalizer;
/**
* @var int|null status of the URL creation after the last [[createUrl()]] call.
* @since 2.0.12
*/
protected $createStatus;

/**
* @var array list of placeholders for matching parameters names. Used in [[parseRequest()]], [[createUrl()]].
Expand Down Expand Up @@ -420,6 +451,7 @@ public function parseRequest($manager, $request)
public function createUrl($manager, $route, $params)
{
if ($this->mode === self::PARSING_ONLY) {
$this->createStatus = self::CREATE_STATUS_PARSING_ONLY;
return false;
}

Expand All @@ -437,6 +469,7 @@ public function createUrl($manager, $route, $params)
}
}
} else {
$this->createStatus = self::CREATE_STATUS_ROUTE_MISMATCH;
return false;
}
}
Expand All @@ -453,6 +486,7 @@ public function createUrl($manager, $route, $params)
if (in_array($name, $this->placeholders) && strcmp($value, '') === 0) {
$params[$name] = '';
} else {
$this->createStatus = self::CREATE_STATUS_PARAMS_MISMATCH;
return false;
}
}
Expand All @@ -462,6 +496,7 @@ public function createUrl($manager, $route, $params)
$tr["<$name>"] = '';
}
} elseif (!isset($this->_paramRules[$name])) {
$this->createStatus = self::CREATE_STATUS_PARAMS_MISMATCH;
return false;
}
}
Expand All @@ -472,6 +507,7 @@ public function createUrl($manager, $route, $params)
$tr["<$name>"] = $this->encodeParams ? urlencode($params[$name]) : $params[$name];
unset($params[$name]);
} elseif (!isset($this->defaults[$name]) || isset($params[$name])) {
$this->createStatus = self::CREATE_STATUS_PARAMS_MISMATCH;
return false;
}
}
Expand All @@ -494,9 +530,22 @@ public function createUrl($manager, $route, $params)
$url .= '?' . $query;
}

$this->createStatus = self::CREATE_STATUS_SUCCESS;
return $url;
}

/**
* Returns status of the URL creation after the last [[createUrl()]] call.
*
* @return null|int Status of the URL creation after the last [[createUrl()]] call. `null` if rule does not provide
* info about create status.
* @see $createStatus
* @since 2.0.12
*/
public function getCreateUrlStatus() {
return $this->createStatus;
}

/**
* Returns list of regex for matching parameter.
* @return array parameter keys and regexp rules.
Expand Down
Loading

0 comments on commit daa8b67

Please sign in to comment.