Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Update system/libraries/Form_validation.php #1933

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
5 participants

CodeIgniter documentation states that rules in configuration can be specified by naming according to class/function.

This update maintains the current functionality using the uri->ruir_string() and adds new functionality consistent with the documentation by using the router->class and router->method. I do not check for the $controller_method to be empty as the router->class and router->method should always return a value.

Associating a Controller Function with a Rule Group
"...is to name it according to the controller class/function you intend to use it with..."

Update system/libraries/Form_validation.php
CodeIgniter documentation states that rules in configuration can be specified by naming according to class/function.

This update maintains the current functionality using the uri->ruir_string() and adds new functionality consistent with the documentation by using the router->class and router->method. I do not check for the $controller_method to be empty as the router->class and router->method should always return a value.

[Associating a Controller Function with a Rule Group](http://codeigniter.com/user_guide/libraries/form_validation.html#savingtoconfig)
"...is to name it according to the controller class/function you intend to use it with..."
Contributor

narfbg commented Oct 26, 2012

That's the purpose of using ruri_string(), doesn't it work?

If my controller->method takes an argument the the ruri string has controller/method/argument and it doesn't match the rules...

I'm also not sure how it will be affected if I'm using routes, but I know my method will return the correct Controller/Method even if the Controller is routed differently.

For Example:

http://pp-dev.srv-vm-web-0002.gsnwgl.local/test/test_ruri/param

Produces:

string '/test/test_ruri/param' (length=21)

I'm not sure how clear that is, I was pretty tired when I left this... What I was trying to say is that the RURI string also contains the parameters passed to the controller method in the url.

lanort commented Jan 9, 2013

I can confirm this behavoir:
I routed /admin/operator/edit/(:num) to /admin/operator_edit($id)
If I browse to "/admin/operator/edit/9" the run function searches for "admin/operator_edit/9" instead of "admin/operator_edit".

Patch:
Replace the following line (line 300) in system/libraries/Form_validation.php

$uri = ($group == '') ? trim($this->CI->uri->ruri_string(), '/') : $group;

with

$uri = ($group == '') ? $this->CI->router->fetch_class().'/'.$this->CI->router->fetch_method() : $group;

Changing the functionality in the way lanort suggests does make the functionality match the documentation.

I think the real question is should the functionality be made consistent with the documentation, or should it maintain backwards compatibility?

Contributor

cryode commented Jan 12, 2013

I've never heard someone complain about this feature not working properly, so this fix seems good to me. It maintains the direct URI match (which should maintain a degree of backwards compatibility), as well as verifying the controller/method group as defined in the user guide.

@cryode cryode commented on an outdated diff Jan 12, 2013

system/libraries/Form_validation.php
@@ -419,10 +419,17 @@ public function run($group = '')
// Is there a validation rule for the particular URI being accessed?
$uri = ($group === '') ? trim($this->CI->uri->ruri_string(), '/') : $group;
+ // Is there a validation rule for the particular Controller/Method being accessed?
+ $controller_method = ($group == '') ? "{$this->CI->router->class}/{$this->CI->router->method}" : $group;
@cryode

cryode Jan 12, 2013

Contributor

The $group == '' check might be redundant, since if the $group param exists, it will already be defined to $uri above, which is checked before $controller_method anyway.

Contributor

cryode commented Jan 25, 2013

@realslacker In response to your question, I think only a handful of people utilize the route-matched ruleset of the library, so backwards compatibility isn't a huge issue here (especially since 3.0 is a major release, so required changes are expected). I think using the fetch_class() and fetch_method() methods are a better choice than pulling the properties directly. Can you update this and make sure there are no conflicts so it can be merged? Make sure to include a change log entry, and update the upgrade guide page, also.

@realslacker realslacker reopened this Jan 29, 2013

@cryode This is my first patch submission on CodeIgniter, so please let me know if I missed anything or if there are any other steps I need to take. Thanks!

Contributor

cryode commented Feb 1, 2013

The diff shows that your change log file has "2607 additions, 2606 deletions". Your IDE might've changed the spacing of the file, because there should only be one new line there.

The user guide update could use better wording, IMO. I can put something together if you'd like. Otherwise that's how you submit patches, so nice job.

Contributor

narfbg commented Feb 1, 2013

Furthermore, the fix description should be in changelog.rst, not in the upgrade notes.

realslacker added some commits Feb 4, 2013

fix documentation consistent with comments
Roll back change to changelog.rst to fix IDE issue, update change log
with bug details and remove same detail from upgrade guide.

Yeah, I really don't know what I'm doing. I'm going to kill this pull request and start over.

@realslacker realslacker closed this Feb 4, 2013

@realslacker realslacker deleted the realslacker:patch-3 branch Feb 4, 2013

Problem still in 2.1.4.

When I put a controller Account with a method login in /application/controllers/api/ folder, I can't use api/account/login as rules group identifier. But account/login works. I think it's a bug because the controller I am visiting is http://localhost/api/account/login.

Yeah, I got frustrated trying to satisfy the documentation requirements so
the fix didn't get merged.
On Nov 22, 2013 1:12 AM, "ÔìÂÖ×Ó¹¤³Ìʦ" notifications@github.com wrote:

Problem still in 2.1.4.

When I put a controller Account with a method login in
/application/controllers/api/ folder, I can't use api/account/login as
rules group identifier. But account/login works. I think it's a bug
because the controller I am visiting is http://localhost/api/account/login
.

¡ª
Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/pull/1933#issuecomment-29052808
.

Thanks for your reply @realslacker. I just changed my local code for my project like this:

$this->CI->uri->ruri_string() // before
$this->CI->uri->uri_string() // after

Then it works well.

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