Adding Url helper class #2630

Closed
qiangxue opened this Issue Mar 5, 2014 · 39 comments

Comments

Projects
None yet
7 participants
@qiangxue
Member

qiangxue commented Mar 5, 2014

We currently have several URL-related functions scattering in different classes. How about introducing a Url helper class to contain these functions and provide more commonly ones? What functions should be provided in this class? What about the followings?

  • action(): like Controller::createUrl()
  • asset(): prefix a relative URL with base URL, e.g. Url::asset('images/logo.gif')
  • How to deal with Html::url() which does both of the above?
@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 5, 2014

Member

Do you want to move methods from other places to Url or leave these as is?

Member

samdark commented Mar 5, 2014

Do you want to move methods from other places to Url or leave these as is?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 5, 2014

Member

UrlManager::createUrl() and Controller::createUrl() should be kept (one is to create url from absolute route, the other supports relative route).

All other URL creation methods should be moved to Url helper, including Html::url().

Member

qiangxue commented Mar 5, 2014

UrlManager::createUrl() and Controller::createUrl() should be kept (one is to create url from absolute route, the other supports relative route).

All other URL creation methods should be moved to Url helper, including Html::url().

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 5, 2014

Member

Sounds good. Especially considering the fact that logically URL has nothing to do with Html helper.

Member

samdark commented Mar 5, 2014

Sounds good. Especially considering the fact that logically URL has nothing to do with Html helper.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 5, 2014

Member

We could have Url::to() to replace Html::url().
These methods are mainly commonly used shortcut methods.

Member

qiangxue commented Mar 5, 2014

We could have Url::to() to replace Html::url().
These methods are mainly commonly used shortcut methods.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Mar 5, 2014

Member

what is Url::to()? I would expect Url::create().

Member

cebe commented Mar 5, 2014

what is Url::to()? I would expect Url::create().

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

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 5, 2014

Contributor

why moving this to helper if it is depending on url-manager settings? what exactly methods do you want to move? createUrl/createAbsoluteUrl are fin in url-manager and in controller, what are other methods?

Contributor

Ragazzo commented Mar 5, 2014

why moving this to helper if it is depending on url-manager settings? what exactly methods do you want to move? createUrl/createAbsoluteUrl are fin in url-manager and in controller, what are other methods?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 5, 2014

Member

I'd also expect Url::create.

Member

samdark commented Mar 5, 2014

I'd also expect Url::create.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 5, 2014

Contributor

for what? it anyway will be a calling url-manager, because of differetn url-routes settings.

Contributor

Ragazzo commented Mar 5, 2014

for what? it anyway will be a calling url-manager, because of differetn url-routes settings.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 5, 2014

Member

@Ragazzo The purpose of this helper class is to provide a set of shortcuts to commonly used methods. It's fine it has dependency on other components.

Member

qiangxue commented Mar 5, 2014

@Ragazzo The purpose of this helper class is to provide a set of shortcuts to commonly used methods. It's fine it has dependency on other components.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 6, 2014

Contributor

but i dont see any good things in it, why it is better then using url-manager? i think it can confuse user, because now he has already 3 start points: controller, url-manager, Html::url(), and it will be added one more. Not good as for me, also it is better when user explicitly see where and how url are generated rather then browsing through many wrappers.

Contributor

Ragazzo commented Mar 6, 2014

but i dont see any good things in it, why it is better then using url-manager? i think it can confuse user, because now he has already 3 start points: controller, url-manager, Html::url(), and it will be added one more. Not good as for me, also it is better when user explicitly see where and how url are generated rather then browsing through many wrappers.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 6, 2014

Member

It won't be one more. Html::url() will be replaced with Url::create.

Member

samdark commented Mar 6, 2014

It won't be one more. Html::url() will be replaced with Url::create.

@drenty

This comment has been minimized.

Show comment
Hide comment
@drenty

drenty Mar 6, 2014

Contributor

I think a shortcut to do this :

Yii::$app->user->setReturnUrl(Yii::$app->getRequest()->getUrl());

would be cool if it doesn't exist already.

Contributor

drenty commented Mar 6, 2014

I think a shortcut to do this :

Yii::$app->user->setReturnUrl(Yii::$app->getRequest()->getUrl());

would be cool if it doesn't exist already.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 6, 2014

Member

@drenty: perhaps Url::remember($url = null) ?

Member

qiangxue commented Mar 6, 2014

@drenty: perhaps Url::remember($url = null) ?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 6, 2014

Member

@qiangxue no, that's too generic.

Member

samdark commented Mar 6, 2014

@qiangxue no, that's too generic.

@drenty

This comment has been minimized.

Show comment
Hide comment
@drenty

drenty Mar 7, 2014

Contributor

@qiangxue @samdark Url::mark($url = null)?

Contributor

drenty commented Mar 7, 2014

@qiangxue @samdark Url::mark($url = null)?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 7, 2014

Member

Too generic as well. Url::setReturn or Url::setBack should be OK.

Member

samdark commented Mar 7, 2014

Too generic as well. Url::setReturn or Url::setBack should be OK.

@lucianobaraglia

This comment has been minimized.

Show comment
Hide comment
@lucianobaraglia

lucianobaraglia Mar 7, 2014

Contributor

@samdark if I first see Url::setReturn or Url::setBack I would think it's about setting a boolean...
Would prefer Url::setReturnUrl or Url::setBackUrl even if it sounds repetitive...

Contributor

lucianobaraglia commented Mar 7, 2014

@samdark if I first see Url::setReturn or Url::setBack I would think it's about setting a boolean...
Would prefer Url::setReturnUrl or Url::setBackUrl even if it sounds repetitive...

@dyegonery

This comment has been minimized.

Show comment
Hide comment
@dyegonery

dyegonery Mar 7, 2014

I agree with Url::setReturnUrl

I agree with Url::setReturnUrl

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 9, 2014

Member

@qiangxue I can handle it if you haven't started yet.

Member

samdark commented Mar 9, 2014

@qiangxue I can handle it if you haven't started yet.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 9, 2014

Member

Sure, go ahead.

On Sunday, March 9, 2014, Alexander Makarov notifications@github.com
wrote:

@qiangxue https://github.com/qiangxue I can handle it if you haven't
started yet.


Reply to this email directly or view it on GitHubhttps://github.com/yiisoft/yii2/issues/2630#issuecomment-37124970
.

Member

qiangxue commented Mar 9, 2014

Sure, go ahead.

On Sunday, March 9, 2014, Alexander Makarov notifications@github.com
wrote:

@qiangxue https://github.com/qiangxue I can handle it if you haven't
started yet.


Reply to this email directly or view it on GitHubhttps://github.com/yiisoft/yii2/issues/2630#issuecomment-37124970
.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 9, 2014

Member

Should we move the following to the helper?

  1. yii\web\controller\createUrl($params) to Url::action($params).
  2. yii\web\controller\createAbsoluteUrl($params) to Url::absoluteAction($params) or Url::action($params, true).
  3. yii\web\controller\getCanonicalUrl to Url::canonical.
Member

samdark commented Mar 9, 2014

Should we move the following to the helper?

  1. yii\web\controller\createUrl($params) to Url::action($params).
  2. yii\web\controller\createAbsoluteUrl($params) to Url::absoluteAction($params) or Url::action($params, true).
  3. yii\web\controller\getCanonicalUrl to Url::canonical.
@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 9, 2014

Contributor

i think action should be route.

Contributor

Ragazzo commented Mar 9, 2014

i think action should be route.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 9, 2014

Member

It doesn't return route, it returns URL pointing to an action.

Member

samdark commented Mar 9, 2014

It doesn't return route, it returns URL pointing to an action.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 9, 2014

Contributor

route sounds beter, Url::absoluteAction as for not good, since route can have only controller or module/controller without action.

Contributor

Ragazzo commented Mar 9, 2014

route sounds beter, Url::absoluteAction as for not good, since route can have only controller or module/controller without action.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 9, 2014

Member

Route is site/index or post/view. It's not URL so it will definitely be confusing that a method named route actually returns URL.

Member

samdark commented Mar 9, 2014

Route is site/index or post/view. It's not URL so it will definitely be confusing that a method named route actually returns URL.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 9, 2014

Contributor

so absoluteAction does not sounds confusing to you?

Contributor

Ragazzo commented Mar 9, 2014

so absoluteAction does not sounds confusing to you?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 9, 2014

Member

route sounds much more confusing. Do you have a name better than action?

Member

samdark commented Mar 9, 2014

route sounds much more confusing. Do you have a name better than action?

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 9, 2014

Contributor
  1. Url::create();
  2. Url::createAbsolute();
  3. Url::createCanonical().
Contributor

Ragazzo commented Mar 9, 2014

  1. Url::create();
  2. Url::createAbsolute();
  3. Url::createCanonical().
@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 9, 2014

Member

So you suggest dropping yii\web\controller\createUrl($params) and yii\web\controller\createAbsoluteUrl($params) without adding any alternative?

Member

samdark commented Mar 9, 2014

So you suggest dropping yii\web\controller\createUrl($params) and yii\web\controller\createAbsoluteUrl($params) without adding any alternative?

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 9, 2014

Contributor

why dropping? make them use Url helper, i thought it was your initial idea, no?

Contributor

Ragazzo commented Mar 9, 2014

why dropping? make them use Url helper, i thought it was your initial idea, no?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 9, 2014

Member

We should keep yii\web\controller\createUrl($params) and
yii\web\controller\createAbsoluteUrl($params) .

Make 'Url' a wrapper of them, not the other way around.

On Sunday, March 9, 2014, Mark notifications@github.com wrote:

why dropping? make them use Url helper, i thought it was your initial
idea, no?


Reply to this email directly or view it on GitHubhttps://github.com/yiisoft/yii2/issues/2630#issuecomment-37126929
.

Member

qiangxue commented Mar 9, 2014

We should keep yii\web\controller\createUrl($params) and
yii\web\controller\createAbsoluteUrl($params) .

Make 'Url' a wrapper of them, not the other way around.

On Sunday, March 9, 2014, Mark notifications@github.com wrote:

why dropping? make them use Url helper, i thought it was your initial
idea, no?


Reply to this email directly or view it on GitHubhttps://github.com/yiisoft/yii2/issues/2630#issuecomment-37126929
.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 9, 2014

Contributor

why then you are adding new helper? anyway i provided as i see names of those methods and how they can be used with controller, other is for you ofcourse))

Contributor

Ragazzo commented Mar 9, 2014

why then you are adding new helper? anyway i provided as i see names of those methods and how they can be used with controller, other is for you ofcourse))

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 9, 2014

Member

@Ragazzo currently create is old Html::url that is not the same as yii\web\controller\createUrl($params).

Member

samdark commented Mar 9, 2014

@Ragazzo currently create is old Html::url that is not the same as yii\web\controller\createUrl($params).

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 9, 2014

Contributor

yes, i know, i mean can we more or less make it similar and use this helper ? anyway as i said, i am not for dropping them. what about names, are they ok?

Contributor

Ragazzo commented Mar 9, 2014

yes, i know, i mean can we more or less make it similar and use this helper ? anyway as i said, i am not for dropping them. what about names, are they ok?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 9, 2014

Member

Overall done. Resulted in the following methods:

Url::create($params);
Url::createAbsolute($params);
Url::base('files/source.zip');
Url::rememberReturn();
Url::getCanonical();
Url::getHome();
Member

samdark commented Mar 9, 2014

Overall done. Resulted in the following methods:

Url::create($params);
Url::createAbsolute($params);
Url::base('files/source.zip');
Url::rememberReturn();
Url::getCanonical();
Url::getHome();
@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Mar 9, 2014

Member

Url::base('files/source.zip');

We may also make $url param optional to allow usage like this in view files:

<img src="<?= Url::base() ?>/images/myimage.png" />

Looks good to me overall.

Member

cebe commented Mar 9, 2014

Url::base('files/source.zip');

We may also make $url param optional to allow usage like this in view files:

<img src="<?= Url::base() ?>/images/myimage.png" />

Looks good to me overall.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 9, 2014

Member

@cebe done.

Member

samdark commented Mar 9, 2014

@cebe done.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Mar 9, 2014

Member

Hm... maybe naming of getCanonical and getHome should be adjusted to be consistent with base:

Url::base()
Url::canonical()
Url::home()

or

Url::getBase()
Url::getCanonical()
Url::getHome()

I'd prefer the first case.

Member

cebe commented Mar 9, 2014

Hm... maybe naming of getCanonical and getHome should be adjusted to be consistent with base:

Url::base()
Url::canonical()
Url::home()

or

Url::getBase()
Url::getCanonical()
Url::getHome()

I'd prefer the first case.

samdark added a commit that referenced this issue 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 closed this Mar 10, 2014

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 11, 2014

Member

👍

Member

qiangxue commented Mar 11, 2014

👍

@ivokund ivokund referenced this issue in Nodge/yii2-eauth Mar 11, 2014

Merged

Fixes Yii2 refactor #2630 #15

@drenty drenty referenced this issue in kartik-v/yii2-markdown Mar 12, 2014

Closed

New URL helper breaks yii2-markdown #12

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