Bug Report #3690

Request::uri() should return the actual URI

Added by Kiall Mac Innes about 3 years ago. Updated almost 3 years ago.

Status:ClosedStart date:06/27/2010
Priority:NormalDue date:
Assignee:Jeremy Bush% Done:

100%

Category:Core
Target version:v3.2.0
Resolution:fixed Points:

Description

The default parameters (Route::defaults) of a route are there to provide fallbacks when those segments are not included in the requested URI. They should not be included when generating a URI using Route::uri, as this results in duplicate content and less predictable behavior when the route defaults are changed and old URIs are cached.

See #3015


Related issues

Related to Kohana v3.x - Bug Report #3015: Reverse routing should not include the default parameters Closed 06/27/2010
Related to Kohana v3.x - Feature Request #3976: Request::uri() query parameters Closed 05/07/2011

Associated revisions

Revision e466e11e
Added by Jeremy Bush almost 3 years ago

Change Route::uri() to return the exact uri passed into it. Use $this->request->route() from now on for routable uri support. Fixes #3690

Revision e9b8e360
Added by Sam de Freyssinet almost 3 years ago

Refs #3976, #3690. All Request_Client_External handlers process Request query parameters correctly again

Revision b39fb8bc
Added by Sam de Freyssinet almost 3 years ago

Refs #3976, #3690. Tidying up Request_Client_Curl to optimize performance and ensure correct settings cannot be overridden

History

#1 Updated by Tomek Rychlik about 3 years ago

This is my proposal to solve this:

In Kohana_Route:

Add new field to store matched default parameters:

class Kohana_Route
{
        ...
        protected $_matched_defaults = array();

      public function get_matched_defaults()
    {
        return $this->_matched_defaults;
    }
        ...

    public function matches($uri)
    {
        ...
        foreach ($this->_defaults as $key => $value)
        {
                ...
            if ( ! isset($params[$key]) OR $params[$key] === '')
            {
                // Set default values for any key that was not matched
                $params[$key] = $value;

                // Store default parameter
                $this->_matched_defaults[$key] = $value;
            }
                ...
        }
    ...
}

In Kohana_Request:

New URI method

class Kohana_Request
        ...
    public function uri(array $params = NULL)
    {
        if ($this->_external)
            return $this->_uri;

        if (is_null($params))
            $params = array();

        $matched_defaults = $this->_route->get_matched_defaults();

        if ( ! array_key_exists('directory', $params) AND ! isset($matched_defaults['directory']))
        {
            // Add the current directory
            $params['directory'] = $this->_directory;
        }

        if ( ! array_key_exists('controller', $params) AND ! isset($matched_defaults['controller']))
        {
            // Add the current controller
            $params['controller'] = $this->_controller;
        }

        if ( ! array_key_exists('action', $params) AND ! isset($matched_defaults['action']))
        {
            // Add the current action
            $params['action'] = $this->_action;
        }

        foreach($this->_params as $key => $value)
        {
            if ( ! array_key_exists($key, $params) AND ! isset($matched_defaults[$key]))
            {
                $params[$key] = $value;
            }
        }

        return $this->_route->uri($params);
    }
         ...
}

#2 Updated by Jeremy Bush about 3 years ago

This is specifically related to Request::uri() now. The correct behavior for this is to just return $this->_uri.

#3 Updated by Tomek Rychlik about 3 years ago

Jeremy Bush wrote:

This is specifically related to Request::uri() now.

I know, i agree.

The correct behavior for this is to just return $this->_uri

???

$this->uri just returns current uri.

Im talking about reverse routing with current request parameters -> Request->uri()
And i want to say that it has 2 bugs:

1) We cannot simply reset parameter to default value: $this->request->uri('action' => NULL), now, we must enter this value publicly: $this->request->uri('action' => 'our_default_action')

2) This method insert default values! (duplicate content).

My soultion solve this 2 problems. I thought you agreed with me in #3015.

#4 Updated by Jeremy Bush almost 3 years ago

  • Status changed from New to Assigned
  • Assignee set to Jeremy Bush

I think Request::uri() should return what's in the url bar. It should not do any kind of reverse routing. If you need that, you should use Route::uri().

#5 Updated by Tomek Rychlik almost 3 years ago

Jeremy, now Request::uri() can change request params in new generated url, do you think it is wrong?
I think it is great, very helpful, but it has one bug (this issue).

If you still think its wrong (as you said "I think Request::uri() should return what's in the url bar")
how do you i.e. change one parameter in current url, with others stay the same?

I.e.:

Current URI: /param1/paramA/paramb

to

NEW URI: /param1/paramB/paramb

i will use

echo Request::uri(array('uppercase_param' => 'paramB'));

#6 Updated by Jeremy Bush almost 3 years ago

If you want to do that, you can use $this->request->route->uri().

#7 Updated by Jeremy Bush almost 3 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100
  • Resolution set to fixed

Fixed and tested by e466e11

#8 Updated by Dmitry T. almost 3 years ago

  • Status changed from Closed to Feedback

Please do not do that!

You're making doing the pagination impossible. If you want do the pagination - you should pass ALL route params to it. Isn't it crazy? Consider this:

Route::set('some', 'blog/<category_id>/<category_name>/<other_param_1>/<other_param_2>(/<page>)')
  ->defaults(array(
    'controller' => 'articles',
    'action' => 'show',
  ));

// Link to page 2 (old version)
echo Route::current()->url(array('page' => 2)); // That's it! Very simple and clear

//New version
echo Route::get('some')->url(array(
  'category_id' => Request::current()->param('category_id'),
  'category_name' => Request::current()->param('category_idname
  'other_param_1' => Request::current()->param('other_param_1'),
  'other_param_1' => Request::current()->param('other_param_2'),
  'page' => 2,
));

Are you serious?

#9 Updated by Dmitry T. almost 3 years ago

Sorry, I mean Request::current()...

#10 Updated by Jeremy Bush almost 3 years ago

No, you should use

$this->request->route()
not Route::get(). If this really makes pagination "hard", then pagination should provide some kind of helper. This didn't change much that you couldn't already do without
$this->request->route()

All the old method did was pre-pass in the controller/action/directory, which was completely wrong anyway, because the uri very likely didn't match the request's uri.

#11 Updated by Dmitry T. almost 3 years ago

// Old version
echo Route::current()->url(array('page' => 2));

//New version
echo Request::current()->route()->url(array(
  'category_id' => Request::current()->param('category_id'),
  'category_name' => Request::current()->param('category_idname
  'other_param_1' => Request::current()->param('other_param_1'),
  'other_param_1' => Request::current()->param('other_param_2'),
  'page' => 2,
));

Not much shorter/easier, is it?

Jeremy Bush wrote:

All the old method did was pre-pass in the controller/action/directory, which was completely wrong anyway

Why wrong? Just the opposite. Not only controller/action/directory, but ALL current route parameters. Now we have to pass all these parameters manually producing much more useless code.

Do I get something wrong?

#12 Updated by Chris Bandy almost 3 years ago

Use Route->uri($params) on the Route that your controller passes to the view, I think.

#13 Updated by Jeremy Bush almost 3 years ago

Do I get something wrong?

Yes, when I do Request::factory('foo') and $this->request->uri() gives me 'foo/bar', that's wrong. There should be no objection to this, you can do EXACTLY what it used to do by using $this->request->route().

#14 Updated by Dmitry T. almost 3 years ago

Let me show one more example:

Route::set('articles', 'article/<id>(/<action>)')
  ->defaults(array(
    'controller' => 'articles',
    'action' => 'show',
  ));

// Old way
// Edit article
Request::current()->url(array('action' => 'edit'));

// Delete
Request::current()->url(array('action' => 'delete'));

// Publish
Request::current()->url(array('action' => 'publish'));

// Etc etc...

// New way
Request::current()->route()->url(array(
  'id' => Request::current()->param('id'),
  'action' => 'edit',
));

Request::current()->route()->url(array(
  'id' => Request::current()->param('id'),
  'action' => 'delete',
));

Request::current()->route()->url(array(
  'id' => Request::current()->param('id'),
  'action' => 'publish',
));

// Etc

You see? In second case we have to pass the id parameter explicitly. But the thing is that we're taking the parameter from the route, which could be done automatically. Isn't the goal of a framework - automatisation and simplification of the coding proccess?

#15 Updated by Dmitry T. almost 3 years ago

Jeremy Bush wrote:

Yes, when I do Request::factory('foo') and $this->request->uri() gives me 'foo/bar', that's wrong

Of course that's wrong. Because "bar" should not be in the url (if that's the default parameter and we don't pass it). But it does not mean that if I want to change only one parameter - I should pass all the defaults every time.

By the way, in KO3.0 all works perfectly and there were no any unwanted parameters in the uri

#16 Updated by Yahasana . almost 3 years ago

@Dmitry, my recommendation is stopping argue for that and stop upgrade to 3.1+, which would like to make everything look like perfect in conception.

#17 Updated by Dmitry T. almost 3 years ago

@Yahasana, your message has nothing to do with the discussion of this topic. Also, docs say that "Kohana is driven by community discussion", and I'm just trying to make Kohana better. Do I have the right to do so?

#18 Updated by Jeremy Bush almost 3 years ago

Your concerns are noted. However I and other devs firmly believe this is the proper behavior. I was slightly incorrect in my assessment earlier. You'll need to do

$this->request->route()->uri($this->request->param())

To mimic the old behavior. Or you can extend route and copy paste the old uri() method into a new method. No, it's not as "nice" as the old method, but the new behavior is the proper behavior for that specific method.

But it does not mean that if I want to change only one parameter

That's not what Request::uri() should do. The old behavior was improper. Request::uri() and Request::url() should be read only. You should be working with the route objects directly for changing parameters and related tasks. It's certainly possible to transplant the old behavior somewhere else, but it won't be part of this issue.

#19 Updated by Dmitry T. almost 3 years ago

OK, then maybe to add some parameter that will be filled with route parameters, such as Route::current()->url(...)? Or maybe some other solution? If you agree, I can create the appropriate ticket.

Thank you for the explanations

#20 Updated by Jeremy Bush almost 3 years ago

  • Status changed from Feedback to Closed

You can make a new issue for 3.3 for some kind of method somewhere that will do what this method used to do.

#21 Updated by Tomek Rychlik almost 3 years ago

Jeremy thanks for the explanations, I agree with you, it is more complicated but more logical.

But this bug report is about that reverse routing should not include to URI the default parameters when passed by $params. And unfortunately it still adds it.

$this->request->route()->uri($this->request->param())

This works ok only when $this->request->param() does not have any default values!

For example:

Route::set('articles', 'article/<id>(/<action>)')
  ->defaults(array(
    'controller' => 'articles',
    'action' => 'show',
  ));

echo Route::url('articles', array('id' => '100');  // 'article/100' OK!
echo $this->request->route()->uri($this->request->param()) // 'article/show/100' WRONG! show is default and its generates duplicate content! I was expecting 'article/100'

There is now way to tell to Route->uri($this->request->param()) that some of the params was matched by default and it should use it! 

The solution to this seems to be easy.
Just update Route->uri() to check if $params values are default or not! For example:

            while (preg_match('#'.Route::REGEX_KEY.'#', $replace, $match))
            {
                list($key, $param) = $match;

                if (isset($params[$param]) AND ! (array_key_exists($param, $this->_defaults) AND ($this->_defaults[$param] == $params[$param]))))
                {
                    // Replace the key with the parameter value
                    $replace = str_replace($key, $params[$param], $replace);
                }
                else
                {
                    // This group has missing parameters
                    $replace = '';
                    break;
                }
            }

#22 Updated by Jeremy Bush almost 3 years ago

Tomek Rychlik wrote:

Jeremy thanks for the explanations, I agree with you, it is more complicated but more logical.

But this bug report is about that reverse routing should not include to URI the default parameters when passed by $params. And unfortunately it still adds it.

It's actually not. The title should be changed on this issue. The reason it's the same is that we changed this for Route::uri() in 3.1, but didn't update Request::uri(). Kiall then copied that issue here for 3.2.

#23 Updated by Jeremy Bush almost 3 years ago

  • Subject changed from Reverse routing should not include the default parameters to Request::uri() should return the actual URI

#24 Updated by Dmitry T. almost 3 years ago

So you want to say in 3.2 default parameters is not included?

#25 Updated by Tomek Rychlik almost 3 years ago

Jeremy Bush:

Ok but you should now create new issue "Route::uri() should NOT use parameter from $params argument if value is the same as defalut"

Dmitry:

No! Look at my first example. Im taking only about parameters from method arguments! In article example:

$this->request->route()->uri(array('article' => 'show', 'id' => 5)); 

Parameter 'show' SHOULD be ignored becouse it is defined in Route as default! Now Route has bug and it will produce: /article/show/5 instead of /article/5/

#26 Updated by Jeremy Bush almost 3 years ago

What does that have to do with this issue, which has to do with Request::uri() ?

#27 Updated by Tomek Rychlik almost 3 years ago

My issue now only apply to Route::uri() and it should be move to new bug report

#28 Updated by Tomek Rychlik almost 3 years ago

Dmitry T. wrote:

OK, then maybe to add some parameter that will be filled with route parameters, such as Route::current()->url(...)? Or maybe some other solution? If you agree, I can create the appropriate ticket.

Thank you for the explanations

By the way, I agree, now to change parameter in current uri we have to do:

$this->request->route()->uri(array('page' => 2) + $this->request->param())

insted of (like in KO 3.1)

$this->request->uri(array('page' => 2))

Please upadte this, maybe add new method in Request or condition statemant in Request::uri()

#29 Updated by Dmitry T. almost 3 years ago

Tomek Rychlik wrote:

Ok but you should now create new issue "Route::uri() should NOT use parameter from $params argument if value is the same as defalut"

I'd say "Route::uri() should NOT include unspecified parameters in the uri", because we still may need explicitly set parameters which equal to default ones (in very very rare cases though)

#30 Updated by Tomek Rychlik almost 3 years ago

So, you think posibility to set explicity default parameters (very rare) is more important than duplicate content (often!)?

Now, to create new uri with current request parameters without default parameters is really very difficult...

What about add second parameter to Route::uri(), something like 'add_defaults = FALSE' to determine if user wants default parameters in URI?

#31 Updated by Jeremy Bush almost 3 years ago

Please file a new issue for this. None of this relates to Request::uri().

#32 Updated by Dmitry T. almost 3 years ago

@Jeremy, I am very sorry, this is my last message in this thread. I promise :)

Tomek Rychlik wrote:

Ok but you should now create new issue "Route::uri() should NOT use parameter from $params argument if value is the same as defalut"

Wait, if so, then Route::uri() would work incorrectle if we have some default parameters as well as non-default?
Example:

Route::set('articles', 'article(/<action>/<id>)')
  ->defaults(array(
    'controller' => 'articles',
    'action' => 'show',
    'id' => NULL,
  ));

// What uri should this give us?
Route::get('articles')->url(array('action' => 'show', 'id' => 5));

P.S. If you want to continue the discussion, please post on the forum

Also available in: Atom PDF