KohanaPHP » Kohana v3.x

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);
    }
         ...
}

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

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.

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'));

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

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

No, you should use

$this->request->route()

$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.

// 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?

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

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().

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?

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

@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.

@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?

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.

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

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;
                }
            }

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.

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

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/

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

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

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()

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)

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?

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

@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:

 1 Route::set('articles', 'article(/<action>/<id>)')
 2   ->defaults(array(
 3     'controller' => 'articles',
 4     'action' => 'show',
 5     'id' => NULL,
 6   ));
 7 
 8 // What uri should this give us?
 9 Route::get('articles')->url(array('action' => 'show', 'id' => 5));

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

Ok added: http://dev.kohanaframework.org/issues/4116