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

Inconsistent syntax for creating links #2426

Closed
samdark opened this Issue Feb 14, 2014 · 30 comments

Comments

Projects
None yet
@samdark
Member

samdark commented Feb 14, 2014

// helpers
Html::a($text, ['/site/view', 'id' => 100]);
Html::url(['/site/view', 'id' => 100]);

// controller
$this->redirect(['/site/view', 'id' => 100]);
$this->createAbsoluteUrl('/site/view', ['id' => 100]);
$this->createUrl('/site/view', ['id' => 100]);

// url manager
Yii::$app->getUrlManager()->createUrl('/site/view', ['id' => 100]);
Yii::$app->getUrlManager()->createAbsoluteUrl('/site/view', ['id' => 100]);

So we have 2 signatures for URLs. It's either ($route, [$params]) or ([$route, $params]). I think we'd better choose one and use it consistently.

@slavcodev

This comment has been minimized.

Show comment
Hide comment
@slavcodev

slavcodev Feb 14, 2014

Contributor

I agree, and vote for using array to route
echo Html::url(['/site/view', 'id' => 100]);

and string to relative url
echo Html::url('/images/photo.jpg'); instead of echo Yii::app()->baseUrl . '/images/photo.jpg'

Contributor

slavcodev commented Feb 14, 2014

I agree, and vote for using array to route
echo Html::url(['/site/view', 'id' => 100]);

and string to relative url
echo Html::url('/images/photo.jpg'); instead of echo Yii::app()->baseUrl . '/images/photo.jpg'

@Mirocow

This comment has been minimized.

Show comment
Hide comment
@Mirocow

Mirocow Feb 14, 2014

i agree with this issue

Mirocow commented Feb 14, 2014

i agree with this issue

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 14, 2014

Contributor

i am for the second too.

Contributor

Ragazzo commented Feb 14, 2014

i am for the second too.

@mgrechanik

This comment has been minimized.

Show comment
Hide comment
@mgrechanik

mgrechanik Feb 14, 2014

Contributor

The current situation is all right with me.
We just need to remember that there are two formats:

  1. "classic" for UrlManager().
  2. "advanced" for Html::url() and all methods that rely upon Html::url() such as controller->redirect()

@slavcodev

and string to relative url ... instead of ...

Html::url() already works like that

Contributor

mgrechanik commented Feb 14, 2014

The current situation is all right with me.
We just need to remember that there are two formats:

  1. "classic" for UrlManager().
  2. "advanced" for Html::url() and all methods that rely upon Html::url() such as controller->redirect()

@slavcodev

and string to relative url ... instead of ...

Html::url() already works like that

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 14, 2014

Member

@mgrechanik what about controller's redirect vs createUrl?

Member

samdark commented Feb 14, 2014

@mgrechanik what about controller's redirect vs createUrl?

@mgrechanik

This comment has been minimized.

Show comment
Hide comment
@mgrechanik

mgrechanik Feb 14, 2014

Contributor

controller's redirect could redirect to any urls when controller's createUrl is used for a different purpose (creating some very specific set of urls).
So if the signatures here are different it is not very bad.

Contributor

mgrechanik commented Feb 14, 2014

controller's redirect could redirect to any urls when controller's createUrl is used for a different purpose (creating some very specific set of urls).
So if the signatures here are different it is not very bad.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 14, 2014

Member

The question is now how bad it is, the question is how to make it better.

Member

samdark commented Feb 14, 2014

The question is now how bad it is, the question is how to make it better.

@mgrechanik

This comment has been minimized.

Show comment
Hide comment
@mgrechanik

mgrechanik Feb 14, 2014

Contributor

Changing controller's method's signatures to ([$route, $params]) (as people above proposed) won't make it better at all because controller->crealeUrl will differ too much from UrlManager->crealeUrl.

As I see the question is mainly about redirect method, is it not?
What if teach it to accept both formats?

    public function redirect($url, $params = [], $statusCode = 302)
    {
        $url = (empty($params) || !is_string($url)) ? Html::url($url) : $this->createUrl($url, $params);
        //var_dump($url);
        return Yii::$app->getResponse()->redirect($url, $statusCode);
    }   
Contributor

mgrechanik commented Feb 14, 2014

Changing controller's method's signatures to ([$route, $params]) (as people above proposed) won't make it better at all because controller->crealeUrl will differ too much from UrlManager->crealeUrl.

As I see the question is mainly about redirect method, is it not?
What if teach it to accept both formats?

    public function redirect($url, $params = [], $statusCode = 302)
    {
        $url = (empty($params) || !is_string($url)) ? Html::url($url) : $this->createUrl($url, $params);
        //var_dump($url);
        return Yii::$app->getResponse()->redirect($url, $statusCode);
    }   
@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 14, 2014

Member

@mgrechanik well, it's about unifying ALL signatures. Leaving some with different format doesn't make sense.

Member

samdark commented Feb 14, 2014

@mgrechanik well, it's about unifying ALL signatures. Leaving some with different format doesn't make sense.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 14, 2014

Member

I'm fine with unifying the format to be ['route', 'name1' => 'value1', 'name2' => 'value2', ...]. There are many places to be changed.

Member

qiangxue commented Feb 14, 2014

I'm fine with unifying the format to be ['route', 'name1' => 'value1', 'name2' => 'value2', ...]. There are many places to be changed.

@samdark samdark added this to the 2.0 Beta milestone Feb 14, 2014

@samdark samdark self-assigned this Feb 14, 2014

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 15, 2014

Member

What about URL parsing? Should format be changed there as well?

Member

samdark commented Feb 15, 2014

What about URL parsing? Should format be changed there as well?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 15, 2014

Member

Why URL parsing is related with this?

On Saturday, February 15, 2014, Alexander Makarov notifications@github.com
wrote:

What about URL parsing? Should format be changed there as well?


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

Member

qiangxue commented Feb 15, 2014

Why URL parsing is related with this?

On Saturday, February 15, 2014, Alexander Makarov notifications@github.com
wrote:

What about URL parsing? Should format be changed there as well?


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

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 15, 2014

Member

It's not. Confused it with something else...

Member

samdark commented Feb 15, 2014

It's not. Confused it with something else...

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 15, 2014

Member

I'm not sure ['route', 'name1' => 'value1', 'name2' => 'value2', ...] is optimal. For example, forming parameters dynamically could require array_merge:

$params = [];
if ($sortBy) {
  $params['sortBy'] = $sortBy;
}
if ($limit) {
  $params['limit'] = $limit;
}
$this->createUrl(array_merge(['orders/index'], $params));
Member

samdark commented Feb 15, 2014

I'm not sure ['route', 'name1' => 'value1', 'name2' => 'value2', ...] is optimal. For example, forming parameters dynamically could require array_merge:

$params = [];
if ($sortBy) {
  $params['sortBy'] = $sortBy;
}
if ($limit) {
  $params['limit'] = $limit;
}
$this->createUrl(array_merge(['orders/index'], $params));
@zelenin

This comment has been minimized.

Show comment
Hide comment
@zelenin

zelenin Feb 15, 2014

Contributor

maybe

$params = [];
if ($sortBy) {
  $params['sortBy'] = $sortBy;
}
if ($limit) {
  $params['limit'] = $limit;
}
$this->createUrl( 'orders/index', $params );

?

Contributor

zelenin commented Feb 15, 2014

maybe

$params = [];
if ($sortBy) {
  $params['sortBy'] = $sortBy;
}
if ($limit) {
  $params['limit'] = $limit;
}
$this->createUrl( 'orders/index', $params );

?

@mgrechanik

This comment has been minimized.

Show comment
Hide comment
@mgrechanik

mgrechanik Feb 16, 2014

Contributor

@zelenin

maybe ?
$this->createUrl( 'orders/index', $params );

That is how it works now.
And we are discussing the possibility to "unifying the format to be ['route', 'name1' => 'value1', 'name2' => 'value2', ...]".

Contributor

mgrechanik commented Feb 16, 2014

@zelenin

maybe ?
$this->createUrl( 'orders/index', $params );

That is how it works now.
And we are discussing the possibility to "unifying the format to be ['route', 'name1' => 'value1', 'name2' => 'value2', ...]".

@zelenin

This comment has been minimized.

Show comment
Hide comment
@zelenin

zelenin Feb 16, 2014

Contributor

@mgrechanik
yes, I propose a version where route is separated from the params. It's clearer and more readable I think.

Contributor

zelenin commented Feb 16, 2014

@mgrechanik
yes, I propose a version where route is separated from the params. It's clearer and more readable I think.

@RdeWilde

This comment has been minimized.

Show comment
Hide comment
@RdeWilde

RdeWilde Feb 16, 2014

I'd choose the first signature, because this is most clear (as in the signature itself, documentation and autocompletion). It makes clear the route is required, parameters are optional. Also, these two are complete different types of data to me, so seperating those seems logical.

I would unify them all.

For Html::a it would make more sense for the second argument to be the string url (result from the routing), as it could also be a manual URL that doesnt have to be routed.

I'd choose the first signature, because this is most clear (as in the signature itself, documentation and autocompletion). It makes clear the route is required, parameters are optional. Also, these two are complete different types of data to me, so seperating those seems logical.

I would unify them all.

For Html::a it would make more sense for the second argument to be the string url (result from the routing), as it could also be a manual URL that doesnt have to be routed.

@yupe

This comment has been minimized.

Show comment
Hide comment
@yupe

yupe Feb 19, 2014

Contributor

I vote for Html::a('link', [...params])...and same signature for other methods

Contributor

yupe commented Feb 19, 2014

I vote for Html::a('link', [...params])...and same signature for other methods

@Mirocow

This comment has been minimized.

Show comment
Hide comment
@Mirocow

Mirocow Feb 19, 2014

yes i vote to like this format Html::a('link', [...params], [...options])...
and in the params write such ['lik', [...params], [...options]]
and in the img like this Html:img('link', [...params], [...options])

and all except the reference should not be required

Mirocow commented Feb 19, 2014

yes i vote to like this format Html::a('link', [...params], [...options])...
and in the params write such ['lik', [...params], [...options]]
and in the img like this Html:img('link', [...params], [...options])

and all except the reference should not be required

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 20, 2014

Member

I'm thinking about ['route', [$params]] for all cases.

Member

samdark commented Feb 20, 2014

I'm thinking about ['route', [$params]] for all cases.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 20, 2014

Member
// helpers
Html::a($text, ['/site/view', ['id' => 100]]);
Html::url(['/site/view', ['id' => 100]]);

// controller
$this->redirect(['/site/view', ['id' => 100]]);
$this->createAbsoluteUrl(['/site/view', ['id' => 100]]);
$this->createUrl(['/site/view', ['id' => 100]]);

// url manager
Yii::$app->getUrlManager()->createUrl(['/site/view', ['id' => 100]]);
Yii::$app->getUrlManager()->createAbsoluteUrl(['/site/view', ['id' => 100]]);
Member

samdark commented Feb 20, 2014

// helpers
Html::a($text, ['/site/view', ['id' => 100]]);
Html::url(['/site/view', ['id' => 100]]);

// controller
$this->redirect(['/site/view', ['id' => 100]]);
$this->createAbsoluteUrl(['/site/view', ['id' => 100]]);
$this->createUrl(['/site/view', ['id' => 100]]);

// url manager
Yii::$app->getUrlManager()->createUrl(['/site/view', ['id' => 100]]);
Yii::$app->getUrlManager()->createAbsoluteUrl(['/site/view', ['id' => 100]]);
@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Feb 20, 2014

Member

I'm thinking about ['route', [$params]] for all cases.

Why do we need the brackets around params in this case?

Member

cebe commented Feb 20, 2014

I'm thinking about ['route', [$params]] for all cases.

Why do we need the brackets around params in this case?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 20, 2014

Member

In order to be able to pass an array of parameters w/o doing any merging.

Member

samdark commented Feb 20, 2014

In order to be able to pass an array of parameters w/o doing any merging.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 20, 2014

Member

I think this format requires more typing and is more error prone.
It's much more common to create a URL from scratch by writing parameters one by one than modifying an existing parameter array.

Member

qiangxue commented Feb 20, 2014

I think this format requires more typing and is more error prone.
It's much more common to create a URL from scratch by writing parameters one by one than modifying an existing parameter array.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 20, 2014

Member

OK. Implementing ['/site/view', 'id' => 100] variant then.

Member

samdark commented Feb 20, 2014

OK. Implementing ['/site/view', 'id' => 100] variant then.

@samdark samdark closed this in 43c17d9 Feb 21, 2014

samdark added a commit that referenced this issue Feb 21, 2014

Merge pull request #2501 from yiisoft/unify-urls
Fixes #2426: Changed URL creation method signatures to be consistent
@RdeWilde

This comment has been minimized.

Show comment
Hide comment
@RdeWilde

RdeWilde Feb 22, 2014

It requires more typing this way, is difficult to phpdoc and makes no sense, for example if you just have an url with no params your creating an Array object for nothing, not efficient at all.

It requires more typing this way, is difficult to phpdoc and makes no sense, for example if you just have an url with no params your creating an Array object for nothing, not efficient at all.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Feb 22, 2014

Member

@samdark is '/site/view' also a valid URL in your implementation?

Member

cebe commented Feb 22, 2014

@samdark is '/site/view' also a valid URL in your implementation?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Feb 22, 2014

Member

Yes.

Member

samdark commented Feb 22, 2014

Yes.

@RusAlex

This comment has been minimized.

Show comment
Hide comment
@RusAlex

RusAlex Feb 22, 2014

Contributor

On my opinion, current way (yii1.x) is good.
Sometimes i just mess how to write "route to module" , but i never
getting mistakes in route array
['/controller/action','param1'=>'value1',...] .
Current way is very easy to remember.

Thanks

Contributor

RusAlex commented Feb 22, 2014

On my opinion, current way (yii1.x) is good.
Sometimes i just mess how to write "route to module" , but i never
getting mistakes in route array
['/controller/action','param1'=>'value1',...] .
Current way is very easy to remember.

Thanks

qiansen1386 pushed a commit to qiansen1386/yii2 that referenced this issue Mar 9, 2014

qiansen1386 pushed a commit to qiansen1386/yii2 that referenced this issue Mar 9, 2014

Merge pull request #2501 from yiisoft/unify-urls
Fixes #2426: Changed URL creation method signatures to be consistent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment