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

#2630: `yii\heplers\Html::url` moved to new `yii\helpers\Url::create` #2676

Merged
merged 21 commits into from Mar 10, 2014

Conversation

Projects
None yet
4 participants
@samdark
Copy link
Member

samdark commented Mar 9, 2014

No description provided.

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 9, 2014

@Ragazzo

This comment has been minimized.

Copy link
Contributor

Ragazzo commented Mar 9, 2014

also need to change BasePage in yii2-codeception extension

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 9, 2014

How exactly? Its getUrl method isn't equivalent of Url::create.

@Ragazzo

This comment has been minimized.

Copy link
Contributor

Ragazzo commented Mar 9, 2014

you said that you will replace all url-manager code with Url helper class

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 9, 2014

I did not.

@Ragazzo

This comment has been minimized.

Copy link
Contributor

Ragazzo commented Mar 9, 2014

re-read that thread, so the only reason to add this helper is Url::create one method? great)

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 9, 2014

It's not the only reason but I did no other changes so far.

samdark added some commits Mar 9, 2014

@cebe cebe added this to the 2.0 Beta milestone Mar 9, 2014

@qiangxue

This comment has been minimized.

Copy link
Member

qiangxue commented Mar 9, 2014

I would suggest the following methods. I'm trying to choose short names, since these are shortcut methods, and also to keep consistency with the method naming in other helpers:

  • route($route = null, $absolute = false): replaces Controller::createUrl() and Controller::createAbsoluteUrl(). $absolute can
    also be a string (either http or https). When $route is not set, returns the current URL.
  • asset($params = null, $absolute = false): replaces Html::url(). When $params is not set, returns the base URL.
  • base($absolute = false): returns the base URL.
  • current($absolute = false): returns the current URL.
  • canonical()
  • home($absolute = false)
  • remember($url = null, $name = null): remembers a URL in session using $name. If $name is not set, default it to User::returnUrlParam.
  • previous($name = null): returns the named URL previously stored in session via remember().

We can drop Controller::createUrl(), Controller::createAbsoluteUrl() and Html::url().

@Ragazzo

This comment has been minimized.

Copy link
Contributor

Ragazzo commented Mar 9, 2014

@samdark said that route is confusing, need better name.

@cebe

This comment has been minimized.

Copy link
Member

cebe commented Mar 9, 2014

Both, asset and route are not really good names imo as it is not intuitive what they do. Also what should $params be in case of asset? what is $route in route() method? an array or a string? It should not be named route if it can take more than a route.

@qiangxue

This comment has been minimized.

Copy link
Member

qiangxue commented Mar 9, 2014

Here's the rationale behind the above naming. URLs used in an application can generally be classified into three types:

  • a URL to a controller action
  • a URL to a resource/asset file
  • a URL to some external resources

The goal here is to provide a way to simplify URL creation for the first two categories (we don't have control of the last category). The name route and asset describe the above categorization.

@qiangxue

This comment has been minimized.

Copy link
Member

qiangxue commented Mar 9, 2014

Also what should $params be in case of asset? what is $route in route() method? an array or a string?

In route($route, $absolute = false), $route can be either a string (e.g. index) or an array (['index', 'lang' => 'en'])

In asset($params, $absolute = false), $params (probably should be renamed as $asset) can also be a string or an array. If a string, it represents a relative URL (unlike that in route()); if an array, it's similar to that in route().

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 9, 2014

route is a bad name since route (site/index) is not URL and it was often confused with URL in 1.1 causing trouble.

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 9, 2014

Also I think we should use $params in case where URL parameters are passed i.e. route and asset.

@qiangxue

This comment has been minimized.

Copy link
Member

qiangxue commented Mar 9, 2014

We can name them as Url::toRoute($route, $absolute = false) and Url::toAsset($asset, $absolute = false), or ofRoute() and ofAsset().

I'm using the name $route here because site/index is a route and ['site/index', 'lang' => 'en'] can be considered as a parameterized route. If using $params, I would wonder where I should specify the route.

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 9, 2014

toRoute and toAsset are better names.

$route for param name is OK as well.

asset($params should be asset($relativeUrl, right?

@qiangxue

This comment has been minimized.

Copy link
Member

qiangxue commented Mar 9, 2014

Fine with me for asset($relativeUrl. Note that asset() should implement the same feature as Html::url(). That is, it can handle both relative URL and [$route, ...params..].

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 9, 2014

Just noticed that asset could accept alias as well (if we aren't going to remove it) so $relativeUrl doesn't make sense anymore to me.

Why should it accept [$route, ...params..]?

@qiangxue

This comment has been minimized.

Copy link
Member

qiangxue commented Mar 9, 2014

Just make it practical since Html::url() is being used by functions such as Html::form() which can take both relative URLs and routes. For relative URLs, they could be using aliases.

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 9, 2014

Question was about Url::toAsset and [$route, ...params..], not about Url::toRoute that is used in forms.

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 10, 2014

Done with adjustments.

public static function to($url = null, $absolute = false)
{
if (is_array($url) && isset($url[0]) || $url === '') {
return static::to($url, $absolute);

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

Infinite recursion when $url === ''.

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

The implementation of to() should just copy that of Html::url() with adjustment regarding $absolute.

This comment has been minimized.

Copy link
@samdark

samdark Mar 10, 2014

Author Member

I've just reused code instead of copying. Recursion is already fixed.

* @return string the normalized URL
* @throws InvalidParamException if the parameter is invalid.
*/
public static function toRoute($route, $absolute = false)

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

To completely replace Controller::createAbsoluteUrl(), $absolute needs to enhanced:

  • If $absolute is a string, it is treated as the scheme (e.g. http or https). An absolute URL will be returned.
  • Otherwise, treat it as a boolean indicating whether absolute url is needed.

This comment has been minimized.

Copy link
@samdark

samdark Mar 10, 2014

Author Member

Maybe rename it to scheme and make it null by default then?

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

Fine with me.

This comment has been minimized.

Copy link
@samdark

samdark Mar 10, 2014

Author Member

So null will mean use relative URL while when specifying prefix it will use absolute one with this prefix.

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

Yes. prefix should be called URI scheme, to be more precise.

return $url;
} else {
$prefix = $absolute ? Yii::$app->request->getHostInfo() : '';
return $prefix . Yii::$app->getRequest()->getBaseUrl() . '/' . $url;

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

Usage of request should be consistent, either ->request or ->getRequest(), but not both.

} else {
$url = Yii::getAlias($url);
if ($url !== '' && ($url[0] === '/' || $url[0] === '#' || strpos($url, '://') || !strncmp($url, './', 2))) {
return $url;

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

Need to consider $absolute here.

@cebe

This comment has been minimized.

Copy link
Member

cebe commented Mar 10, 2014

Tests are failing cause by the changes, need to check whats going on there. In general URL creation should not need an application request if not absolutely necessary.

samdark added some commits Mar 10, 2014

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 10, 2014

Build is now fixed. API was adjusted again.

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 10, 2014

Currently nothing except Html::url was removed. Url helper is reusing controller and url manager. As I can see it's possible to actually move implementation details to helper and remove methods from controller and probably url manager.

*
* In case there is no controller, [[\yii\web\UrlManager::createUrl()]] will be used.
*
* @param string $schema URI schema to use. If specified absolute URL with the schema specified is returned.

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

I thought it again. I think we should support the "current scheme". That is, when I'm creating an absolute URL, I want it to use the current scheme, but I don't want to explicitly specify if it is http or https.

This comment has been minimized.

Copy link
@samdark

samdark Mar 10, 2014

Author Member

Hmm. That's either changing signature to ..., $absolute = false, $schema = null or introducing a special value for $schema. Which way do you prefer?

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

I prefer using a single parameter $absolute

  • false: default.
  • true: absolute URL with the current scheme
  • 'http': absolute URL with 'http'
  • 'https': absolute URL with 'https'

This comment has been minimized.

Copy link
@qiangxue

qiangxue Mar 10, 2014

Member

In most cases, you would only use boolean value.
Only in some special scenario, you may want to change the scheme.

This comment has been minimized.

Copy link
@samdark

samdark Mar 10, 2014

Author Member

There's a special case where you may want setting$absolute to false while $schema to something. It's getting absolute URL from alias.

This comment has been minimized.

Copy link
@cebe

cebe Mar 10, 2014

Member

why would you set it to false when you expect a schema? You should set it then, the function should be able to resolve alias and replace schema then.

This comment has been minimized.

Copy link
@samdark

samdark Mar 10, 2014

Author Member

Right. Changing it...

@qiangxue

This comment has been minimized.

Copy link
Member

qiangxue commented Mar 10, 2014

As I can see it's possible to actually move implementation details to helper and remove methods from controller and probably url manager.

We can/should remove the method from controller, but not from the url manager. The url manager is the place where the actual implementation is done. Url is mainly a wrapper.

@cebe

This comment has been minimized.

Copy link
Member

cebe commented Mar 10, 2014

Some tests for all the different cases would be useful.

@samdark

This comment has been minimized.

Copy link
Member Author

samdark commented Mar 10, 2014

Working on additional tests.

@qiangxue

This comment has been minimized.

Copy link
Member

qiangxue commented Mar 10, 2014

The implementation looks good to me now. I have no more comments.

samdark added a commit that referenced this pull request Mar 10, 2014

Merge pull request #2676 from yiisoft/url-helper
#2630: `yii\heplers\Html::url` moved to new `yii\helpers\Url::create`

@samdark samdark merged commit 348dfa7 into master Mar 10, 2014

@samdark samdark deleted the url-helper branch Mar 10, 2014

bethrezen added a commit to bethrezen/yii2-widgets that referenced this pull request Mar 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.