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

Remove empty per_page query string from first page URL. #2166

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

cryode commented Jan 18, 2013

In response to #2124. The Pagination library adds the per_page query string to the base_url, but the first page is empty, resulting in a URL like example.com/welcome?per_page=. This fix assigns the base_url to the first_url option, if a custom one does not already exist.

I'm not sure if this fix will affect the reuse_query_string feature, since I've never used it. Pretty sure it won't, but if anyone can test and verify, that'd be swell.

Signed-off-by: Eric Roberts eric@cryode.com

Remove empty per_page query string from first page URL.
Signed-off-by: Eric Roberts <eric@cryode.com>
Contributor

cryode commented Jan 18, 2013

Crap, hold off on merging this, I have another addition to the Pagination class I forgot about, that I can include here.

Contributor

narfbg commented Jan 18, 2013

Unless it's related to #2124 it's probably better to submit it as a separate PR. However, I will hold this one until it's confirmed that it doesn't mess up reuse_query_string or any other feature.

Contributor

cryode commented Jan 18, 2013

It's not related. I'll PR it after, then. Will try to do some testing with this, too, figure out if this affects anything else that I can find.

Contributor

cryode commented Jan 19, 2013

So while trying to double check that this PR won't cause issues, I found something weird. Somewhere around line 396 is this line:

$this->cur_page = (int) $CI->uri->rsegment($this->uri_segment);

Note the use of rsegment() instead of segment(). I think someone had a brain fart when they were working on this, because that isn't gonna fly.

Edit: There's an rsegment() near line 384, also.

Contributor

narfbg commented Jan 21, 2013

Why wouldn't it "fly"? AFAIK, it's intentional and tries to work around weird routing configurations.

Contributor

cryode commented Jan 21, 2013

If you don't put the offset / page number in your rerouted rule, it won't be found. The normal URI should always have the page number in it -- after all, that's the one that's seen by the user, indexed by search engines, etc. Why you'd use the rerouted URI, I have no idea.

$route['blog/page/:num'] = 'blog'; // broken
Contributor

cryode commented Jan 21, 2013

Oh, and I did find an issue that's semi related to this change that needs fixing. Has to do with how the first page URLs are rendered -- they are inconsistent, and there's a bug with my code that's related. I'll try to have it fixed up tonight.

Contributor

narfbg commented Jan 22, 2013

Because the default segment number is 3, which in the rsegments array is the first parameter passed to the controller method.

Contributor

cryode commented Jan 22, 2013

Yeah, if the parameter is routed in the first place. Look at my example -- the rerouted URI would be blog/index. That's it.

When a user sets the uri_segment option in order for the pagination library to know what page it's on, they will always use the URI that they can see in their browser. That's how it currently sits in 2.1.3, and every version before that.

So if there is a valid reason for using rsegment(), it needs to be reworked, because using it by itself is broken.

Contributor

narfbg commented Jan 22, 2013

Not just if - unless you do any special routing or if you're under a directory, rsegment is the same as segment. Plus, routing blog/page/:num to blog is bogus on its own.

Contributor

cryode commented Jan 22, 2013

You're giving the exact reasons why this doesn't work, lol.

Whether the route is "bogus" or not is not the point. It was an example to show how this is broken. There are a lot of times people might want to use the URI class instead of method parameters, especially if they only need the index() method of their controller. Not to mention that using rsegment() will use a URI that the user is not expecting and is not documented. If you see the URL example.com/blog/archive/page/15, what segment are you going to assume you need the pagination lib to use?

I don't know what more you want from me to explain that this is stupid...

Contributor

narfbg commented Jan 22, 2013

Have you actually tried and confirmed that anything related to this discussion isn't working? Cause I can repeat your first sentence with a tiny correction. :)

First of all, you can't give a broken example if you want to show that something is broken.

And now that you mention the URI class - check out the docs, "routed segments" are well documented and they could help you (and I'll quote you here) especially if they only need the index method of their controller, lol.
Consider that the requested URI path is '/home', here's how it works:

  • segments:
    1 - home
    2 - NULL
    3 - NULL
  • rsegments:
    1 - home
    2 - index
    3 - NULL

How in any twisted kind of logic would using the first instead of the second would help you? It just doesn't make sense. Furthermore, the example.com/blog/archive/page/15 will map correctly using routed segments and it will not so by using regular URI segments.
By default at least, and this is probably what you're missing in the whole picture - the default value for the uri_segment option is 3. And the third rsegment will always be the first parameter to your controller's method, no matter how creative you can be on the routing part. Consider it black magic if you will, but it actually saves the user from the need to expect and therefore configure.

Contributor

cryode commented Jan 22, 2013

No, I just complain about broken shit without validating anything. Give me a break.

First, I didn't give you a broken example of anything. That is a 100% valid route. I have a single index() method of my Blog controller. I want to route my paginated URLs to it (which should look something like example.com/blog/page/5. I don't need to use the param option because I can use the URI class to fetch the segment to generate my query offset. So I keep the route simple. I have this exact scenario on my localhost for testing this PR and I am watching the page numbers not work correctly. If I change both instances to segment(), the pages work perfectly. Just because it might not be common doesn't mean it isn't completely valid, and a likely scenario for many developers.

Second, I know how the URI class works, and I'm getting offended by your demeaning responses. I feel as if you never take a single thing I contribute to CodeIgniter seriously. Even now, in this unnecessary, drawn out reply, I'm essentially repeating everything I've already said just because I feel like I have to defend myself and my ability to write proper code. I'm not some lazy dev who doesn't bother trying to figure out a problem before spouting off that something is broken. I already know this is a problem, and I want to fix it. I brought it up because I wanted to know if there was another reason why rsegment() was being used that I didn't see or grasp. If there was a good, valid reason, I would incorporate it and make sure every scenario works. But I cannot see a reason why it is utilized, so I am trying to show you why it should not be.

Third, your example for the home/ URI is irrelevant (and saying that it's my twisted logic is untrue and further demeaning). This is about pagination URLs with a specific segment that specifies the current page/offset. home/ clearly doesn't have that, so I don't understand why you're arguing it. In either case, the page/offset will end up being 0 after type casting.

Similarly, I don't understand why you're telling me how the example.com/blog/archive/page/15 maps to segments has anything to do with this. I'm not talking about how the URI's of CodeIgniter work, I'm talking about how the pagination library figures out what fucking page it's on.

I'll use your blog example to definitively show you why this is wrong. Here's the controller:

class Blog extends CI_Controller {

    public function archive($p1, $p2)
    {
        var_dump($p1, $p2, $this->uri->rsegment_array());
    }

}

All I'm doing is checking the values of the parameters, and what is available to me through rsegment().

With zero routing, if I visit the URL http://ci.dev/index.php/blog/archive/page/15 (my exact localhost URL), this is the result: http://cl.ly/image/2x0c0M0r3d0F Just what you'd expect -- the segments pass to the params, and the rsegment array has all four available just the same as the URL.

Now, what happens when I plug this route in:

$route['blog/archive/page/:num'] = 'blog/archive';

Result: http://cl.ly/image/0s0h0E212O01

The rerouted URL has zero parameters in its URI, therefore nothing is passed to the method or the URI's rsegment array beyond the controller and method. You are correct that, when using rsegment, the third segment will always be the first parameter of the controller method. IF IT EXISTS IN THE FIRST PLACE. Which, when using a certain type of routed URL, it might not.

How do we fix this? Well let's check out the difference between rsegment() and segment(). Let's remove the parameters to get rid of the notice errors, and see what the raw values are:

class Blog extends CI_Controller {

    public function archive()
    {
        var_dump($this->uri->rsegment(4), $this->uri->segment(4));
    }

}

With no routes: http://cl.ly/image/0j2U3N0M1b2t
With the same route added again: http://cl.ly/image/090v282C0m3X

segment() is always consistent, because the page/offset segment is always in the URI that is in the browser. It is not always in the rerouted URI. That is why it is not a good idea to keep.

And again still, disregarding the actual technical aspects, this will inevitably lead to an increase in frustrations with the pagination library because people will NEVER assume that it is using the rerouted segments instead of the normal, browser address segments. Just because the URI class is well documented doesn't mean that people are going to read through the pagination class's source code to figure out which URI segment they should use. They will do what the user guide says (which, by the way, still says that it will "automatically" discover the segment, which is not technically correct).

Want even more reasoning? Here's an exact copy of part of the segment checks:

elseif ( ! $this->cur_page && $CI->uri->segment($this->uri_segment) !== $base_page)
{
    $this->cur_page = (int) $CI->uri->rsegment($this->uri_segment);
}

Notice anything inconsistent?

So, to recap:

  • It's broken.
  • It's not intuitive / user-friendly.
  • It's unnecessarily different from previous CI versions. Isn't CI supposed to be the king of backwards compatibility? Hello potential broken applications.
  • It's not even consistent in the current code base, mixing the use of segment() and rsegment().

Your move, dude.

Contributor

narfbg commented Jan 22, 2013

No, I just complain about broken shit without validating anything. Give me a break.

I'll ignore this, for obvious reasons.

First, I didn't give you a broken example of anything. That is a 100% valid route.

Valid doesn't mean appropriate and if it isn't appropriate (granted, in my own view) I consider it broken.

I have a single index() method of my Blog controller. I want to route my paginated URLs to it (which should look something like example.com/blog/page/5. I don't need to use the param option because I can use the URI class to fetch the segment to generate my query offset. So I keep the route simple. I have this exact scenario on my localhost for testing this PR and I am watching the page numbers not work correctly. If I change both instances to segment(), the pages work perfectly.

Now to show you what (I believe) approprite in this case:

$route['blog/page/(:num)'] = 'blog/index/$1';

I don't see this as complicated or something that needs to be simplified. In fact, you're making your code more complicated the way that you're trying to achieve the same effect. Routes are meant to simplify your code, not the other way around.

Just because it might not be common doesn't mean it isn't completely valid, and a likely scenario for many developers.

I (naturally) admit that I couldn't possibly see everybody's code, but seriously - this is the first time that I can even imagine that scenario. You've said it yourself - it's not common, and that to me doesn't make it a likely scenario for many developers.

Second, I know how the URI class works, and I'm getting offended by your demeaning responses. I feel as if you never take a single thing I contribute to CodeIgniter seriously. Even now, in this unnecessary, drawn out reply, I'm essentially repeating everything I've already said just because I feel like I have to defend myself and my ability to write proper code. I'm not some lazy dev who doesn't bother trying to figure out a problem before spouting off that something is broken.

Calm down. If I didn't take your or anybody's contributions seriously, then I would either not respond at all or close the issue/PR in question. In fact, I was happy to wait on your next commit and merge it, instead of participating in this pointless argument. I'm sorry if I've offended you, that has not been my intention at any point.

Sure, I'm hard to convince - that's my job when I don't see (the) logic behind a suggestion, in order to ensure the quality of whatever code gets into CodeIgniter. And while we might completely disagree on this particular topic, I have been nitpicking on every single character that gets submitted, which is (I suspect) where your negative impression comes from. Again, this is all quality assurance.

And you're no different, or at least I haven't seen you reconsider anything that you've said - why the harsh reaction? I too get the impression that you're arguing about everything with me and I don't feel good about it, but I also realize that your point of view just might happen to be different than mine on many occasions.

While I'm at the "negative part", you're also turning many threads into off-topic discussions and this current one is a beautiful example of that - it's about a GET parameter, not segments vs. rsegments. You did the same on another PR, demanding that we accept your style of (no-)alignment, you did it on the startup errors, etc.
Honestly, a friendly suggestion - please don't do that, I'm sure you understand why.

I already know this is a problem, and I want to fix it. I brought it up because I wanted to know if there was another reason why rsegment() was being used that I didn't see or grasp. If there was a good, valid reason, I would incorporate it and make sure every scenario works. But I cannot see a reason why it is utilized, so I am trying to show you why it should not be.

Sorry, but so far I see that it's a problem for you. And it doesn't make sense to you because of your specific scenario, for which you've said it yourself - it's not common.

$config['uri_segment'] = 4;

... problem solved and that's why you have configuration options. Routed segments work in 99% out of the remaining cases, you simply can't make every scenario to work out of the box - it's programming, you know how it works.

Third, your example for the home/ URI is irrelevant (and saying that it's my twisted logic is untrue and further demeaning). This is about pagination URLs with a specific segment that specifies the current page/offset. home/ clearly doesn't have that, so I don't understand why you're arguing it. In either case, the page/offset will end up being 0 after type casting.

My example was meant to show you that rsegments are more predictable and have non-changeable positions for every element, even if that element doesn't exist in the HTTP request. And to address the 'demeaning' accusations - I assumed that you'd understand that and didn't care to give more lengthy examples. Since you didn't get that however, here's a more detailed one (assuming defaults, no custom routes):

  • /home/5 routes to /home/index/5
    • rsegments would be 1 - home, 2 - index, 3 - 5
    • segments would be 1 - home, 2 - 5
  • /home/index/5 also routes to /home/index/5
    • rsegments would be 1 - home, 2 - index, 3 - 5
    • segments would be 1 - home, 2 - index, 3 - 5

What this depicts is consistency in routed segments which isn't present for regular URI segments, and consistency means reliability - it's a natural choice. I didn't say that it's your twisted logic, I said any twisted logic. And again - no twisted logic can prove choosing consistency to be the wrong choice, hence why it doesn't make any sense that using routed segments can be a brain fart.

Similarly, I don't understand why you're telling me how the example.com/blog/archive/page/15 maps to segments has anything to do with this. I'm not talking about how the URI's of CodeIgniter work, I'm talking about how the pagination library figures out what fucking page it's on.

If figures that out by looking at values that fucking depend on how CodeIgniter's URIs work.

I'll use your blog example to definitively show you why this is wrong. Here's the controller:

class Blog extends CI_Controller {

    public function archive($p1, $p2)
    {
        var_dump($p1, $p2, $this->uri->rsegment_array());
    }

}

All I'm doing is checking the values of the parameters, and what is available to me through rsegment().

With zero routing, if I visit the URL http://ci.dev/index.php/blog/archive/page/15 (my exact localhost URL), this is the result: http://cl.ly/image/2x0c0M0r3d0F Just what you'd expect -- the segments pass to the params, and the rsegment array has all four available just the same as the URL.

Now, what happens when I plug this route in:

$route['blog/archive/page/:num'] = 'blog/archive';

Result: http://cl.ly/image/0s0h0E212O01

The rerouted URL has zero parameters in its URI, therefore nothing is passed to the method or the URI's rsegment array beyond the controller and method. You are correct that, when using rsegment, the third segment will always be the first parameter of the controller method. IF IT EXISTS IN THE FIRST PLACE. Which, when using a certain type of routed URL, it might not.

Back do bad examples ... Why the hell would you route blog/archive/whatever to blog/archive? It gets there by default and you're obviously breaking it by declaring that route.
Furthermore, it is your job to make sure that you have default values for any publicly accessible controller method - why are you showing me these errors?

How do we fix this? Well let's check out the difference between rsegment() and segment(). Let's remove the parameters to get rid of the notice errors, and see what the raw values are:

class Blog extends CI_Controller {

public function archive()
{
    var_dump($this->uri->rsegment(4), $this->uri->segment(4));
}

}

With no routes: http://cl.ly/image/0j2U3N0M1b2t
With the same route added again: http://cl.ly/image/090v282C0m3X

As I just explained - the whole idea is wrong in the first place.

You're placing a route that overrides the default (and proper) behavior and then you override that bogus route with code in order to revert to how it would've worked originally.

Basically, you're creating the problem that you're trying to fix here.

segment() is always consistent, because the page/offset segment is always in the URI that is in the browser. It is not always in the rerouted URI. That is why it is not a good idea to keep.

... and I was talking about consistency, lol.
Is it consistent when I visit /blog/archive/15? It isn't, while routed segments are, when you do proper routing.

And again still, disregarding the actual technical aspects, this will inevitably lead to an increase in frustrations with the pagination library because people will NEVER assume that it is using the rerouted segments instead of the normal, browser address segments.

I tend to agree on that, even though I don't think that they would need to.

Just because the URI class is well documented doesn't mean that people are going to read through the pagination class's source code to figure out which URI segment they should use. They will do what the user guide says (which, by the way, still says that it will "automatically" discover the segment, which is not technically correct).

You said that rsegments were not documented, I just pointed out that they are. If you meant that it's not documented that the Pagination class uses routed segments, then I'm sorry - I didn't understand that. If that's true, well then more power to you - it should be. +1

The word "automatically" isn't technically correct - completely agree. +2
I didn't write the docs and I didn't put that word in there.

Want even more reasoning? Here's an exact copy of part of the segment checks:

elseif ( ! $this->cur_page && $CI->uri->segment($this->uri_segment) !== $base_page)
{
    $this->cur_page = (int) $CI->uri->rsegment($this->uri_segment);
}

Notice anything inconsistent?

I do, and I agree - it needs to be fixed. What I don't agree on is that rsegment should be replaced by segment.

So, to recap:

  • It's broken.
  • It's not intuitive / user-friendly.
  • It's unnecessarily different from previous CI versions. Isn't CI supposed to be the king of backwards compatibility? Hello potential broken applications.
  • It's not even consistent in the current code base, mixing the use of segment() and rsegment().
  • That last part, the elseif that you've shown is broken indeed.
  • Yes it is - that's my whole point. You're just explaining and trying to fix a problem that you've created yourself.
  • Hello, lengthy upgrade notes that need to be updated with this as well. Goodbye to any progress if we don't change anything existing at all.
  • See the [X]-marked reply.

Your move, dude.

I hope that move makes sense to you.

Contributor

cryode commented Jan 22, 2013

I (naturally) admit that I couldn't possibly see everybody's code, but seriously - this is the first time that I can even imagine that scenario. You've said it yourself - it's not common, and that to me doesn't make it a likely scenario for many developers.

What you believe to be ideal or appropriate is completely irrelevant. It's impossible that every single developer will code like you. I am a clear example of this. ;) Creating this standard of how routes are supposed to be done is the opposite of what the flexible route system is there for. There should be zero requirement that segments need to be passed to a controller method, ever. Not for routes, not for pagination, nothing. Who cares if this example is "making things difficult" or is not common in your view.

Regarding off topic: first, I don't see how bringing up a flaw in a library that this PR happens to be about is off topic. I'm not going to create a separate issue for every little thing I find, especially when it's related to a library in discussion already. I could've easily been silent, fixed it to my standards and pushed the changes. At which time, you probably would've asked why I changed it, and we'd be right back here anyway. Same for the error reporting -- the PR was literally only about error reporting!

You did the same on another PR, demanding that we accept your style of (no-)alignment

Second, there is a PERFECT example of demeaning tone. I didn't demand a damn thing with my whitespace comments. I made a suggestion, and then I argued my point behind it, which is the entire point of pull requests in the first place. I felt it relevant to that PR because it was in the class I was editing, and it was brought up (by you!). I then agreed that it was a broader topic that could be PR'ed and discussed separately, and I moved on. I can concede that you probably don't mean to be rude in your critique of PRs, but accusing me of demanding things is inaccurate. Forgive me for bringing up thoughts about the quality of CodeIgniter, and being defensive when I feel confronted.

you simply can't make every scenario to work out of the box

YES WE CAN!!! $this->uri->segment() will ALWAYS BE THE SAME. ALWAYS. Because it will always be the URL in the address bar. Standard URLs, routed URLs, query strings -- it doesn't matter!

My example was meant to show you that rsegments are more predictable and have non-changeable positions for every element, even if that element doesn't exist in the HTTP request. And to address the 'demeaning' accusations - I assumed that you'd understand that and didn't care to give more lengthy examples. Since you didn't get that however, here's a more detailed one (assuming defaults, no custom routes):

Once again, I understand how rsegments work... Showing your example just gives me another impression of being talked down to.

(assuming defaults, no custom routes):
/home/5 routes to /home/index/5

No, it doesn't. That's a 404 error. If you wanted that URI to work, you'd need to route it. And if you routed it like this: $route['home/:num'] = 'home'; you'd get a broken pagination library. You want an example of an instance where you need a route because it isn't standard routing? There it is. Yes, a proper route would solve the problem. But a proper route is not guaranteed, nor should it be enforced.

If figures that out by looking at values that fucking depend on how CodeIgniter's URIs work.

And I've already explained how that isn't going to happen 100% of the time, based on the way that you want rsegment to work. Just because that third segment will be consistent doesn't mean it will always exist.

Back do bad examples ... Why the hell would you route blog/archive/whatever to blog/archive?

I'm tired of beating this dead horse. It's a perfectly valid possible route that shows a blatant bug in the software. I don't give a shit how bad it is. You fix bugs for all scenarios, not just the ones you like.

Furthermore, it is your job to make sure that you have default values for any publicly accessible controller method - why are you showing me these errors?

Undefined variables further show how those rsegments params do not exist in certain circumstances. If they were given a default, that value would still never change, and the same problem with the pagination library page not being detected would be present.

Let me clarify something about consistency, because I think this is where a lot of this is getting conflicted. When I say that $this->uri->segment() is consistent, I mean that the developer can look at his address bar, pick out the segment he needs, and plug it in. It's right there to be referenced at any point. It will never be different from what you see in the URI with your own eyes. YES, segment(4) is almost guaranteed to be completely different everywhere in the app -- controllers, methods, params, even useless garbage and what have you. That's not what I meant. It is consistent in the way that it retrieves info from the current URI. I can easily look at a URI, count my segment, and plug it in without any further thought.

You said that rsegments were not documented, I just pointed out that they are. If you meant that it's not documented that the Pagination class uses routed segments, then I'm sorry - I didn't understand that. If that's true, well then more power to you - it should be. +1

Thissssssss. 🍻

I will acknowledge that I can see a possible benefit of using rsegment(): as the default. If no uri_segment is defined, it could check rsegment(3). I was originally going to suggest that the pagination library simply use the last URI segment as the default, since that appears to be the most common structure that I've ever seen. I could see rsegment() working, also, though I do like the last segment idea better personally (I think it would be more common and work for more people out of the box).

However, I still whole-heartedly disagree with using it as the go-to segment array. For all the reasons I've already said.

Contributor

narfbg commented Jan 22, 2013

What you believe to be ideal or appropriate is completely irrelevant. It's impossible that every single developer will code like you. I am a clear example of this. ;)

It's also impossible that every single developer is coding like you, yet you're the only one complaining about it and my opinion is completely irrelevant, but yours does matter?

Creating this standard of how routes are supposed to be done is the opposite of what the flexible route system is there for. There should be zero requirement that segments need to be passed to a controller method, ever. Not for routes, not for pagination, nothing. Who cares if this example is "making things difficult" or is not common in your view.

Let's see if I got this right ...
The router, a core part of CodeIgniter, which translates segments into controller/method/p/a/r/a/m/s at all times (meaning you can't use CI without it), should have a zero requirement of passing the first to the latter? This is as possible as dropping a stone and having the option of it not dropping onto the ground.

The example doesn't just make it difficult, it overrides any default behavior - you should expect it to require additional configuration instead of working out of the box.

Regarding off topic: first, I don't see how bringing up a flaw in a library that this PR happens to be about is off topic. I'm not going to create a separate issue for every little thing I find, especially when it's related to a library in discussion already.

Even if this thing that we're discussing is to be considered a bug - it would be a separate bug. "Fixing" it wouldn't (in practice) require fixing the first one, it would need a separate changelog entry and as you can see - a discussion on an entirely different level. And if anybody wants to look it up - they'll be endlessly looking for an appropriately named issue, but they won't find one, because we're flooding this one instead.

I could've easily been silent, fixed it to my standards and pushed the changes. At which time, you probably would've asked why I changed it, and we'd be right back here anyway.

Yes, and neither I, nor anybody else would accept it until we come up with a decision on it. And by the looks of it, a 3.0 release might happen before we agree on anything. Is it fair that a bugfix doesn't make it into a release because you wanted to make a pair of them?

Same for the error reporting -- the PR was literally only about error reporting!

It was about startup errors.

Second, there is a PERFECT example of demeaning tone. I didn't demand a damn thing with my whitespace comments. I made a suggestion, and then I argued my point behind it, which is the entire point of pull requests in the first place. I felt it relevant to that PR because it was in the class I was editing, and it was brought up (by you!). I then agreed that it was a broader topic that could be PR'ed and discussed separately, and I moved on. I can concede that you probably don't mean to be rude in your critique of PRs, but accusing me of demanding things is inaccurate. Forgive me for bringing up thoughts about the quality of CodeIgniter, and being defensive when I feel confronted.

So, you don't remember writing this while arguing your point on alignment?

If these types of contributions are going to be argued and, more importantly, discouraged, then I have no reason to continue contributing to and defending CodeIgniter.

If that wasn't a demand, then I don't know what it was.

YES WE CAN!!! $this->uri->segment() will ALWAYS BE THE SAME. ALWAYS. Because it will always be the URL in the address bar. Standard URLs, routed URLs, query strings -- it doesn't matter!

No, you can't. $this->uri->segment() will always be the same, provided that the URL is always the same. The requested URL is under the control of the visitor. Routing is what you can control and rely on - play it safe. And here's an example of when it WON'T work with regular segments - using a controller inside a directory, something way more common than what you want to do.

Once again, I understand how rsegments work... Showing your example just gives me another impression of being talked down to.

Not seeing the benefit of them is what gives me the impression that you're not completely understanding how they work.

No, it doesn't. That's a 404 error. If you wanted that URI to work, you'd need to route it. And if you routed it like this: $route['home/:num'] = 'home'; you'd get a broken pagination library. You want an example of an instance where you need a route because it isn't standard routing? There it is. Yes, a proper route would solve the problem. But a proper route is not guaranteed, nor should it be enforced.

Right, I gave a bad example as well. A proper route is not enforced, it is required when you're not using the standard [directory/]controller/method[/param1/param2] scheme and it's your job to guarantee that it's properly done, for your own sake.

And I've already explained how that isn't going to happen 100% of the time, based on the way that you want rsegment to work. Just because that third segment will be consistent doesn't mean it will always exist.

OK, it won't work for you - fine, I get it.

I'm tired of beating this dead horse. It's a perfectly valid possible route that shows a blatant bug in the software. I don't give a shit how bad it is. You fix bugs for all scenarios, not just the ones you like.

Yes, it is a valid route, and a pointless one. The only reason that you're allowed to do it is because it's assumed that you won't misuse it and writing an actual restriction is, well ... bloat.

Undefined variables further show how those rsegments params do not exist in certain circumstances. If they were given a default, that value would still never change, and the same problem with the pagination library page not being detected would be present.

Yes, of course their values wouldn't change - you told the router not to pass those values. What the hell did you expect?

Let me clarify something about consistency, because I think this is where a lot of this is getting conflicted. When I say that $this->uri->segment() is consistent, I mean that the developer can look at his address bar, pick out the segment he needs, and plug it in. It's right there to be referenced at any point. It will never be different from what you see in the URI with your own eyes. YES, segment(4) is almost guaranteed to be completely different everywhere in the app -- controllers, methods, params, even useless garbage and what have you. That's not what I meant. It is consistent in the way that it retrieves info from the current URI. I can easily look at a URI, count my segment, and plug it in without any further thought.

I don't think that consistent is the proper term for what you're describing, but ignore that. The goal is to make things working out-of-the box. Routed segments guarantee that in the vast majority of cases, while your particular one requires additional configuration for sure.

I will acknowledge that I can see a possible benefit of using rsegment(): as the default. If no uri_segment is defined, it could check rsegment(3). I was originally going to suggest that the pagination library simply use the last URI segment as the default, since that appears to be the most common structure that I've ever seen. I could see rsegment() working, also, though I do like the last segment idea better personally (I think it would be more common and work for more people out of the box).

Finally, something constructive. There are many options to be considered and that's what I like to see, you could even for() through the segments until you find a numeric, or directly pass the current page - more options.

However, I still whole-heartedly disagree with using it as the go-to segment array. For all the reasons I've already said.

... and unless we agree to disagree - there doesn't seem to be an end of this.

Contributor

cryode commented Jan 23, 2013

It's also impossible that every single developer is coding like you, yet you're the only one complaining about it and my opinion is completely irrelevant, but yours does matter?

Your opinion that my example is "broken" doesn't matter. I'm not saying that because I disagree with it, or because I'm ignoring it in favor of my own. I wouldn't do something like that in my application, nor would I recommend it. Both are besides the point. The fact is that it is an example of a 100% valid route scenario, and the current state of the Pagination library is ill-equipped to handle such a route. Period.

Let's see if I got this right ...
The router, a core part of CodeIgniter, which translates segments into controller/method/p/a/r/a/m/s at all times (meaning you can't use CI without it), should have a zero requirement of passing the first to the latter? This is as possible as dropping a stone and having the option of it not dropping onto the ground.

Honestly, dude, do you think that's what I meant? Do you think I would suggest something that means breaking the entire way CI handles URIs? This kind of response is what's making me so defensive. If you simply said "Wouldn't this break the way CI works? I don't understand.", I would've taken no offense and simply clarified my statement. I know we're both a bit guilty of being douche bags, but that's my reaction and perspective to things like this.

To clarify, there should be zero requirement that all segments, specifically method parameter segments, should be included in the re-routed URI. $route['this/is/a/long/uri/of/segments'] = 'controller/method'; is 100% acceptable, as is = 'controller';. The Pagination class should not expect, or rely on a re-routed parameter segment existing.

On that subject, I could suggest that the Pagination library not rely on the CI object at all, and require things like the current page/offset be passed to it, amongst other things. But that's off topic... :octocat:

The example doesn't just make it difficult, it overrides any default behavior - you should expect it to require additional configuration instead of working out of the box.

Why? It works out of the box right now in 2.1.3. It has worked out of the box in every single version of CI I've ever used. I cannot fathom how implementing code that changes conventions that CI devs are used to, potentially breaks existing applications, and introduces possible confusion and aggravation for existing and new CI devs, can possibly be considered a better choice than the existing code that works just fine.

So, you don't remember writing this while arguing your point on alignment?

I won't get too into this, because a lot of my attitude was because I felt my rational argument was being completely disregarded, like it had no merit. I understand that it was mostly due to not being the time or place, and I let it go.

No, you can't. $this->uri->segment() will always be the same, provided that the URL is always the same.

The actual returned value of segment() will differ, of course. I did further clarify what I meant here, and I hope you saw that. segment() will return the correct value regardless of if or how the URI is routed. That is how it will "always be the same" -- its value will not be changed by a route.

And here's an example of when it WON'T work with regular segments - using a controller inside a directory, something way more common than what you want to do.

Did YOU actually try and see if this isn't working? Let me demonstrate again!

// Assuming ZERO routes.

// Controller file: ./controllers/sub/blog.php

class Blog extends CI_Controller {

    public function archive()
    {
        var_dump($this->uri->rsegment(5), $this->uri->segment(5));
    }

}

The example URL to test: http://ci.dev/index.php/sub/blog/archive/page/15 Looking at this URL, the page/offset segments counts out to be 5, so I've set that in my controller. The output result: http://cl.ly/image/2V1M3A2Z3c0P

rsegment(5) does not exist. segment(5) gives us the correct page/offset value from the URI in the address bar, exactly where the developer would look to reference said segment. 🐴

Zero routing, 100% standard functionality of CodeIgniter, nothing forced or "broken" here. Exact. Same. Bug.

Not seeing the benefit of them is what gives me the impression that you're not completely understanding how they work.

What benefit? Your own example of how segment() "won't work" shows the exact opposite problem! How is that a benefit?!? I completely understand the consistency behind the structure of rsegments. How do you not completely understand that sometimes certain rsegments won't exist? 🐴

Right, I gave a bad example as well. A proper route is not enforced, it is required when you're not using the standard [directory/]controller/method[/param1/param2] scheme and it's your job to guarantee that it's properly done, for your own sake.

You're absolutely right, and I have never suggested otherwise. All of my examples so far have been proper routes. They follow the accepted URI structure and are mapped accordingly. And in some of those situations, the rsegment parameters do not exist. That is the fundamental flaw with using rsegment. 🐴

Yes, of course their values wouldn't change - you told the router not to pass those values. What the hell did you expect?

🐴 🐴 🐴

If you tell the router not to pass the values, they won't exist in the rsegment array. If you try to access them using the pagination library, they won't exist, and your pagination won't work. Once again, you are listing the exact reasons why it doesn't work.

The goal is to make things working out-of-the box. Routed segments guarantee that in the vast majority of cases, while your particular one requires additional configuration for sure.

It works out of the box in 2.1.3, for all cases, not just the "vast majority". I'm not suggesting anything other than using the code that already exists and already works. There is zero "additional configuration" required. YOUR solution requires additional configuration -- verifying that your routes are always in a certain format, adjusting your URI segment number to match the rerouted segments instead of what a developer can see in their browser, forcing developers to upgrade their applications because of this change. 🐴

Finally, something constructive.

Condescending.

I don't get it, man. I don't know why this is so complicated. Ping someone else in here to take a look, because apparently this is going nowhere.

Contributor

cryode commented Jan 23, 2013

For my own reference if I need them: #1476, #1909, e66d624

Contributor

narfbg commented Jan 23, 2013

Your opinion that my example is "broken" doesn't matter. I'm not saying that because I disagree with it, or because I'm ignoring it in favor of my own.

The fact that you say this for probably the fifth time shows that you're simply refusing to accept anything other but your own opinion.

I wouldn't do something like that in my application, nor would I recommend it. Both are besides the point. The fact is that it is an example of a 100% valid route scenario, and the current state of the Pagination library is ill-equipped to handle such a route. Period.

I thought that you would and that was the problem. Now it seems that nobody would do it, so why are you still arguing? Who cares if it's a valid route? I'll just say this: define('TRUE', FALSE); is valid code.

Honestly, dude, do you think that's what I meant? Do you think I would suggest something that means breaking the entire way CI handles URIs? This kind of response is what's making me so defensive. If you simply said "Wouldn't this break the way CI works? I don't understand.", I would've taken no offense and simply clarified my statement. I know we're both a bit guilty of being douche bags, but that's my reaction and perspective to things like this.

You provoked me to say this, it just shows how flawed your arguments are on this.

Everything besides backwards-compatibility that you've said makes absolutely no sense for anybody to do. Your whole thesis is built around a rare, specific route that is both horrificly wrong an unnecessary for the purposes of pagination.

To clarify, there should be zero requirement that all segments, specifically method parameter segments, should be included in the re-routed URI. $route['this/is/a/long/uri/of/segments'] = 'controller/method'; is 100% acceptable, as is = 'controller';. The Pagination class should not expect, or rely on a re-routed parameter segment existing.

It is possible dude, not acceptable - there's a big difference. You are again and again claiming that this route should throw away every piece of logic that I've explained.
Every godddamn tutorial, lesson, manual, whatever out there that tries to teach you CodeIgniter will show you how to use routes to map segments into controller/method/p/a/r/a/m/e/t/e/r/s and not one will show you a route that ignores a segment which will be used later - it is meant to work like this. I don't know why you are refusing to understand this.

On that subject, I could suggest that the Pagination library not rely on the CI object at all, and require things like the current page/offset be passed to it, amongst other things. But that's off topic... :octocat:

You just wrote multiple lenghy comments on why it shouldn't require something and now you're proposing that it requires another ... Other than that - I agree that it should be possible to pass them manually.

Why? It works out of the box right now in 2.1.3. It has worked out of the box in every single version of CI I've ever used. I cannot fathom how implementing code that changes conventions that CI devs are used to, potentially breaks existing applications, and introduces possible confusion and aggravation for existing and new CI devs, can possibly be considered a better choice than the existing code that works just fine.

It didn't work out of the box with directories. Out-of-the-box means zero configuration.

Again, your point revolves around a specific route - that's configuration.
The specific URL scheme that you've shown/chosen doesn't match the default URI segment index - requires configuration.

This is not out of the box, fact.

The actual returned value of segment() will differ, of course. I did further clarify what I meant here, and I hope you saw that. segment() will return the correct value regardless of if or how the URI is routed. That is how it will "always be the same" -- its value will not be changed by a route.

Yes, you've explained what you meant by this and your explanation simply says that 1/2/3 will map to array(1 => '1', 2 => '2', 3 => '3');. I know that and this doesn't make it doesn't make it right if it can be manipulated.

The whole argument on this is dumb - I say that values shouldn't be changeable by a site visitor and you say that the values shouldn't be changeable by a route.
The fact is, the visitor can do whatever they want and you can't control them, what you can control is routes - this on its own should be enough for you to conclude that routes are the better choice, for one simple reason - they are reliable.

Did YOU actually try and see if this isn't working? Let me demonstrate again!

// Assuming ZERO routes.

// Controller file: ./controllers/sub/blog.php

class Blog extends CI_Controller {

    public function archive()
    {
        var_dump($this->uri->rsegment(5), $this->uri->segment(5));
    }

}

The example URL to test: http://ci.dev/index.php/sub/blog/archive/page/15 Looking at this URL, the page/offset segments counts out to be 5, so I've set that in my controller. The output result: http://cl.ly/image/2V1M3A2Z3c0P

rsegment(5) does not exist. segment(5) gives us the correct page/offset value from the URI in the address bar, exactly where the developer would look to reference said segment. 🐴

Zero routing, 100% standard functionality of CodeIgniter, nothing forced or "broken" here. Exact. Same. Bug.

The goal is for it to work with zero configuration, out of the box. The fact that you can dump the value from the controller is irrelevant - what's relevant is how the Pagination library decides to use it, you've said that yourself. If it's the 5th segment - you'll have to configure the library to use it. Yes, rsegment(5) won't exist - that's exactly my point, it will be rsegment(3) and CI_Pagination will use it - zero routing AND zero configuration.

What benefit? Your own example of how segment() "won't work" shows the exact opposite problem! How is that a benefit?!? I completely understand the consistency behind the structure of rsegments. How do you not completely understand that sometimes certain rsegments won't exist? 🐴

The one I just explained above.

You're absolutely right, and I have never suggested otherwise. All of my examples so far have been proper routes. They follow the accepted URI structure and are mapped accordingly. And in some of those situations, the rsegment parameters do not exist. That is the fundamental flaw with using rsegment. 🐴

Back to the same old stuff.

valid !== proper
valid !== acceptable

The only flaw is that you seem to be using === in the above expressions.

If you tell the router not to pass the values, they won't exist in the rsegment array. If you try to access them using the pagination library, they won't exist, and your pagination won't work. Once again, you are listing the exact reasons why it doesn't work.

Exactly. Exactly. Exacly. You tell them not to exist - it's your choice, not a bug or a flaw.

It works out of the box in 2.1.3, for all cases, not just the "vast majority". I'm not suggesting anything other than using the code that already exists and already works. There is zero "additional configuration" required. YOUR solution requires additional configuration -- verifying that your routes are always in a certain format, adjusting your URI segment number to match the rerouted segments instead of what a developer can see in their browser, forcing developers to upgrade their applications because of this change. 🐴

No, it doesn't - it doesn't work with directories out of the box.
You are suggesting that we change back to code that doesn't and in this particular instance, you're saying that his would force developers to upgrade their applications. Yes, they will have to upgrade - their applications are built around that flaw, because that flaw existed. And this is in a major new version - they will update a lot of code anyway.

Your own examples require more than zero additional configuration, don't expect mine not to.

I don't get it, man. I don't know why this is so complicated. Ping someone else in here to take a look, because apparently this is going nowhere.

Ping all of GitHub if you'd like. I personally don't want to get people involved in such an absurd argument unless they decide to do so uninvited.

Contributor

Dentxinho commented Jan 23, 2013

$route['blog/page/:num'] = 'blog'; // broken

How would Pagination Library find the correct page, if this route maps to a controller blog with no parameters, since it uses rsegment?
The only benefit i see of using segment with a route like this is not having parameters on the function Blog/index, paginating with the numbers on the URL without passing anything to your controller. But, what if you want to navigate by accessing the routed url (blog/index/123 in this case)?

IMHO routes should be used like this:

$route['my_confused_url/i/dunno/how/to/seo/:num'] = 'blog/list/$1'; //to pass (:num) to the first parameter of method list from controller blog. You know this.

rsegment(3) will always exist if you route like it is suppose to be routed. Maybe that's why 3 is the library's default.

Contributor

cryode commented Jan 23, 2013

Okay, it's clear we're not really on the same page here yet. I think you're basing everything on out of the box functionality, and I'm trying to show multiple scenarios that focus on uri_segment being user defined. The circumstances behind the two arguments will not be the same, so let's focus on each scenario and see if we can figure this out.


Default, out of the box functionality, assuming no uri_segment option set.

I wholly agree that using segment(3) as a default is not optimal, and I never intended to give the opposite impression. Using rsegment(3) is a much better option in comparison, because it will indeed always map to the first parameter of the controller method that is called. Regardless of the impression given in this lengthy debacle, I have always understood this functionality.

Now, I don't think it's the best solution for default logic. I believe that using the last segment of the address bar URI will best match the way most people use paginated URLs -- by putting the page/offset segment at the end of their URLs. You'd want to calculate the number of the last segment, then retrieve it using segment(X), because it is not guaranteed that a parameter segment will be available in the rsegment array.

Some non-routed URL examples with their results:

  1. ci.dev/controller/method/15 -- A standard, non-routed URL with a single parameter. rsegment(3) works great.
  2. ci.dev/controller/method/page/15 -- Same, but two params. rsegment(3) matches page; doesn't work.
  3. ci.dev/folder/controller/method/15 -- A non-routed, controller-in-a-subfolder URL with one param. rsegment(3) works great.
  4. ci.dev/folder/controller/method/page/15 -- Same, but with two params. Same result as # 2.

Examples with routes, and their results:

  1. blog/page/(:num) => blog/index/$1 -- A route that passes the page segment to the controller method's first param. rsegment(3) works great.
  2. blog/page/:num => blog -- Same route location to blog/index, but no param passing. rsegment(3) = NULL; doesn't work.
  3. blog/tag/(:any)/page/(:num) => 'blog/tags/$1/$2 -- A common category-style route with pagination. Page/offset is sent as the second param. rsegment(3) returns the tag name. Any time the page/offset segment is passed to a parameter other than the first one, it won't work.

Now, I know you will argue with me about the "validity" or "properness" of number two, as you have. I cannot help but to repeat myself in saying that it is simply a valid route. A perfectly viable option to a developer should they so choose. If CodeIgniter itself does not expect, require, or force a user to pass parameters to controller methods, why should the Pagination library?! Again, I'm not saying that slapping segment(3) back in is a better solution; it's not.

IMO, I'm not "refusing to understand" anything, you are. I am showing examples of default CodeIgniter functionality. If you have a problem with the flexibility that CI offers developers, then make your own issue or PR (which, by the way, I think you should be doing -- your code should be subject to peer review just as much as mine is).

Just for kicks, look at the user guide page for URI routing and tell me how many of those routes do not pass a parameter. If the official documentation says it's possible, if the actual code allows for it to work perfectly fine and without exception, then it is completely unacceptable for a library to expect or force such a standard.

Using the solution of retrieving the last URI segment from the segment array results in a functioning pagination library out of the box for all the above solutions, routed or not.


uri_segment option explicitly set by the dev.

Assuming these scenarios involve either A) existing application code built on anything pre-3.0, or B) a dev used to the conventions the Pagination library has used thus far, utilizing the proposed rsegment()-based solution:

  1. ci.dev/controller/method/15 -- The dev sets the segment to 3. Ends up being the same as the default rsegment(3). Works great.
  2. ci.dev/controller/method/page/15 -- The dev sets the segment to 4. rsegment(4) matches the second parameter. Works great.
  3. ci.dev/folder/controller/method/15 -- The dev sets the segment to 4. rsegment(4) = NULL; doesn't work.
  4. blog/tag/(:any)/page/(:num) => 'blog/tags/$1/$2 -- A repeated route example from earlier. The dev sets the segment to 5. rsegment(5) = NULL; doesn't work.

The change to rsegment() will result in backwards-compatibility issues, requiring some developers to update their applications, since segment(X) and rsegment(X) are not always the same. Since the orignal segment() code works fine (and to be clear, I'm talking about more than strictly out of the box here), requiring an update seems frivolous. I know you at least acknowledge it, so yay agreement on something. 🍻

Using rsegment() is also different from the conventions that CI developers are used to the Pagination library following. This may lead to confusion in developing applications for some. Yes, clear documentation about how the segment key maps to rsegment() would likely alleviate a lot of possible confusion, so this specific argument can be subjective.

Then, there's also the same issue of the rsegment array not containing any parameters, because of how the user has routed their URIs. If they don't pass any params to the re-routed URI, it won't matter what segment they put in there because it will never work.


The solution.

The way the Pagination library tried to retrieve the default page/offset segment by simply using segment(3) sucked, and did not work in the way that the user guide specified (automagically). However, when a user explicitly set the uri_segment option, or when their application happened to match the default third segment, it worked perfectly.

Thus, the only thing that needs to be addressed is the default segment logic of the library. The use of segment() to pull the page/offset should not be changed. It may be rearranged depending on how the logic is laid out in the end, but "if it ain't broke, don't fix it".

I am fond of using the last segment of the pre-routed URI, or what is seen in the browser address bar, as the default segment number. This number is easily found via something like:

$this->uri_segment = count($this->segment_array());

I hope this alleviates some of the confusion and differences in our points of view. Depending on your response, I'd really like to hear from other parties. Because the main issue still appears to be the "legitimacy" of my examples, which isn't even my code, idea, or doing (which is making me kind of tired of having to argue it). Additional perspectives can only increase the likelihood of a solution, which is my ultimate goal. Even if I'm wrong, doesn't matter. I don't mind being wrong at all.

Regarding off-topic, this PR needs to be updated anyway since this submission is incomplete with bugs that need to be addressed. If there's some way to rededicate the PR / issue / whatever to this subject, I'm happy to do so.

If anything, this has helped me figure out better ways of wording my own arguments and interpreting others' as well. So thanks for that. 🍪

Contributor

narfbg commented Jan 24, 2013

Default, out of the box functionality, assuming no uri_segment option set.

I wholly agree that using segment(3) as a default is not optimal, and I never intended to give the opposite impression.

That's all the impressions that you've been giving, intended or not. :)

Using rsegment(3) is a much better option in comparison, because it will indeed always map to the first parameter of the controller method that is called. Regardless of the impression given in this lengthy debacle, I have always understood this functionality.

The impression given is not that you don't understand how it works, but why it is a better option. In fact, every time that I've said it's the better option you've claimed that I'm giving arguments showing that it's not. This is the first time that you've actually said it is, even for this scenario only.

Now, I don't think it's the best solution for default logic. I believe that using the last segment of the address bar URI will best match the way most people use paginated URLs -- by putting the page/offset segment at the end of their URLs. You'd want to calculate the number of the last segment, then retrieve it using segment(X),

That I agree on, I've agreed on it before and it most likely how it should and will end up as a final solution. You've probably missed it because your goal up until this point seemed to be to convince me in your next words in that sentence:

because it is not guaranteed that a parameter segment will be available in the rsegment array.

This is a claim that you've repeated over and over and over again and I'd rather die than accept it, for one simple reason - the only way that it's not guaranteed is when you explicitly tell the router not to have it. You make it, your choice - your bug.
Your deliberately wrong choice that nobody else would make, I dare to say. You know it, I know it and any sane CI developer would know it.

Some non-routed URL examples with their results:

  1. ci.dev/controller/method/15 -- A standard, non-routed URL with a single parameter. rsegment(3) works great.
  2. ci.dev/controller/method/page/15 -- Same, but two params. rsegment(3) matches page; doesn't work.
  3. ci.dev/folder/controller/method/15 -- A non-routed, controller-in-a-subfolder URL with one param. rsegment(3) works great.
  4. ci.dev/folder/controller/method/page/15 -- Same, but with two params. Same result as # 2.

Examples with routes, and their results:

  1. blog/page/(:num) => blog/index/$1 -- A route that passes the page segment to the controller method's first param. rsegment(3) works great.
  2. blog/page/:num => blog -- Same route location to blog/index, but no param passing. rsegment(3) = NULL; doesn't work.
  3. blog/tag/(:any)/page/(:num) => 'blog/tags/$1/$2 -- A common category-style route with pagination. Page/offset is sent as the second param. rsegment(3) returns the tag name. Any time the page/offset segment is passed to a parameter other than the first one, it won't work.

Now, I know you will argue with me about the "validity" or "properness" of number two, as you have. I cannot help but to repeat myself in saying that it is simply a valid route. A perfectly viable option to a developer should they so choose. If CodeIgniter itself does not expect, require, or force a user to pass parameters to controller methods, why should the Pagination library?! Again, I'm not saying that slapping segment(3) back in is a better solution; it's not.

You're not saying it now, but the fact is - that's what you've said, multiple times.

  • You've said that segment should be used instead of rsegment - fact.
  • You've said that (and this is your strongest argument so far) it would cause problems for existing applications, because they use segment - fact, suggesting that keeping it the old way is better.
  • You've said that segment works where rsegment doesn't - fact, yet segment doesn't work in half of the above examples.

Now, you know that I'll argue on number two, but you don't know that I'll argue on number three - switch the places of $1 and $2 and you're all set. This is the same argument as with number two - if you'll be routing something, do your routing properly. You're doing it wrong, period.

And the fact is - CodeIgniter does expect you to pass parameters. If it didn't, you wouldn't see Missing argument warnings when you write a controller method with parameters that don't have default values and you don't pass anything to them.
If you'll be doing this, CodeIgniter expects you to do your thing properly, otherwise it would use Reflection to pass NULLs to these empty parameters. But this would be bloat and so nobody would write it, in the same way that nobody would write the code to actually force you to pass parameters from within your routes - it's completely unnecessary AND you're not supposed to skip parameters that you won't use.

Just to be clear - again, I'm not saying that rsegment(3) is the best option either. I say it's better than segment(3).

IMO, I'm not "refusing to understand" anything, you are. I am showing examples of default CodeIgniter functionality. If you have a problem with the flexibility that CI offers developers, then make your own issue or PR (which, by the way, I think you should be doing -- your code should be subject to peer review just as much as mine is).

IMO, it's the other way around, but we can throw that statement at each other until the end of time, because ... you've said it - it gets people into being defensive. I perfectly understand what you mean, but the problem is - it's not flexible, it's dumb.
And here's the big difference - you refuse to understand the latter, while I am refusing to accept it. The fact that I've been given the priviledge to commit code without review and to decide when a rewiew is complete should mean that I would know what's acceptable and what not.
When I have doubts on something, then I'd request a review - see the feature/replace_cookies branch, it's not a coincidence that it's been sitting there for nearly 2 months already. But I have absolutely no doubt about this.

And speaking of being defensive - please, don't tell me how to do my job here.

Just for kicks, look at the user guide page for URI routing and tell me how many of those routes do not pass a parameter. If the official documentation says it's possible, if the actual code allows for it to work perfectly fine and without exception, then it is completely unacceptable for a library to expect or force such a standard.

Two (assuming that you're talking about parameters that represent data to be used for lookup):

  1. $route['product/:num'] = "catalog/product_lookup";, which is simply intended to show you how to use wildcards. Note that this is not under the Examples section.
  2. $route['blog/joe'] = "blogs/users/34";, which is a direct one-to-one relashionship - you don't do lookup here, you obviously know that the user "joe" has a user_id of 34.

Just until after the second one, there are not examples hinting that a segment would be processed further, and there's a reason for that - the guide is written in a typical step-by-step fashion, you show the basics first, then you show how to utilize them. It's one of the very first pages of the documentation, you just don't start with the complicated stuff first.

Using the solution of retrieving the last URI segment from the segment array results in a functioning pagination library out of the box for all the above solutions, routed or not.

Cheers on that, again.

uri_segment option explicitly set by the dev.

Assuming these scenarios involve either A) existing application code built on anything pre-3.0, or B) a dev used to the conventions the Pagination library has used thus far, utilizing the proposed rsegment()-based solution:

  1. ci.dev/controller/method/15 -- The dev sets the segment to 3. Ends up being the same as the default rsegment(3). Works great.
  2. ci.dev/controller/method/page/15 -- The dev sets the segment to 4. rsegment(4) matches the second parameter. Works great.
  3. ci.dev/folder/controller/method/15 -- The dev sets the segment to 4. rsegment(4) = NULL; doesn't work.
  4. blog/tag/(:any)/page/(:num) => 'blog/tags/$1/$2 -- A repeated route example from earlier. The dev sets the segment to 5. rsegment(5) = NULL; doesn't work.

The change to rsegment() will result in backwards-compatibility issues, requiring some developers to update their applications, since segment(X) and rsegment(X) are not always the same. Since the orignal segment() code works fine (and to be clear, I'm talking about more than strictly out of the box here), requiring an update seems frivolous. I know you at least acknowledge it, so yay agreement on something. 🍻

You could then well assume that PDO doesn't work, oci8 executes each statement multiple times, all of the code is written in PHP4 style and you have close to 150 bugs present. This is a major version man, it is the perfect moment to make breaking changes - there are enough already for people to expect them.

Using rsegment() is also different from the conventions that CI developers are used to the Pagination library following. This may lead to confusion in developing applications for some. Yes, clear documentation about how the segment key maps to rsegment() would likely alleviate a lot of possible confusion, so this specific argument can be subjective.

Yep.

Then, there's also the same issue of the rsegment array not containing any parameters, because of how the user has routed their URIs. If they don't pass any params to the re-routed URI, it won't matter what segment they put in there because it will never work.

Sigh. I don't even want to get back on this.

The solution.

The way the Pagination library tried to retrieve the default page/offset segment by simply using segment(3) sucked, and did not work in the way that the user guide specified (automagically).

Agreed, for the Nth time.

However, when a user explicitly set the uri_segment option, or when their application happened to match the default third segment, it worked perfectly.

And so it does when you do your routes properly.

Thus, the only thing that needs to be addressed is the default segment logic of the library. The use of segment() to pull the page/offset should not be changed. It may be rearranged depending on how the logic is laid out in the end, but "if it ain't broke, don't fix it".

It was broken, you even referenced the issue number.

I am fond of using the last segment of the pre-routed URI, or what is seen in the browser address bar, as the default segment number.

"Browser address bar" is a term that you must never use in web development, that is - unless you code the browser itself. I've already explained why in my previous comments.

This number is easily found via something like:

$this->uri_segment = count($this->segment_array());

In a friendly manner:

Explaining how a complicated mix of code (the URI, Router classes) works is demeaning to you, but hinting me on how to find the last element of an array shouldn't be to me? :)

I hope this alleviates some of the confusion and differences in our points of view. Depending on your response, I'd really like to hear from other parties. Because the main issue still appears to be the "legitimacy" of my examples, which isn't even my code, idea, or doing (which is making me kind of tired of having to argue it). Additional perspectives can only increase the likelihood of a solution, which is my ultimate goal.

👍
Our arguments will never match, but it looks like we both agree on a better solution - that's what matters.

Even if I'm wrong, doesn't matter. I don't mind being wrong at all.

Oh, yes you do, lol.

Regarding off-topic, this PR needs to be updated anyway since this submission is incomplete with bugs that need to be addressed. If there's some way to rededicate the PR / issue / whatever to this subject, I'm happy to do so.

Unless one bug can't be fixed without reworking something else - they should be submitted separately.

If anything, this has helped me figure out better ways of wording my own arguments and interpreting others' as well. So thanks for that. 🍪

Yes, getting straight to the solution would've been better. :)

Contributor

cryode commented Jan 24, 2013

Look, man. I'm trying to acknowledge that I probably gave the wrong impression because I was arguing something a bit different than you were. You probably did the same. It's a misunderstanding. I just wanted to clarify the technicals behind my argument under specific scenarios to try and get us on the same page. I know I repeated myself sometimes, but each time I was addressing a specific scenario.

You've said that segment works where rsegment doesn't - fact, yet segment doesn't work in half of the above examples.

I think you're trying to say that the default segment(3) doesn't work in some of my examples. We both already know it sucks, so I'm not defending it. Maybe you didn't read my whole comment before responding to each part, but I said:

Using the solution of retrieving the last URI segment from the segment array results in a functioning pagination library out of the box for all the above solutions, routed or not.

That solution uses segment(), so that's what I meant.

And the fact is - CodeIgniter does expect you to pass parameters. If it didn't, you wouldn't see Missing argument warnings when you write a controller method with parameters that don't have default values and you don't pass anything to them.

You're right. And it was probably not the best example when I showed such errors earlier. But CodeIgniter does not require you to have method parameters, either. So again, if CI itself doesn't require it, then the Pagination library certainly shouldn't, either.

Just to be clear - again, I'm not saying that rsegment(3) is the best option either. I say it's better than segment(3).

Agreed. But saying this just leads me to believe that you think that's the solution I've been arguing this whole time, because it is most definitely not. I really hope that's clear now, also.

And speaking of being defensive - please, don't tell me how to do my job here.

I'm sorry, I didn't mean to tell you how to do your job. I said that I think you should be subject to peer review just as much as anyone else. Not because your code is always wrong, but because it promotes better code and a better community. Maybe if I have a problem with this, someone else might have a problem with another commit. I think you'd agree that it's better to address issues before a release than after as much as you can.

This is a major version man, it is the perfect moment to make breaking changes - there are enough already for people to expect them.

I don't disagree with the timing of requiring the changes, I disagree with the necessity to make the changes in the first place.

It was broken, you even referenced the issue number.

Here is where I have a problem. The use of segment() in general was not broken. The use of segment(3) as a default was not intuitive and therefore "broken" in the sense that it did not automagically find the segment like the user guide stated it should.

Issue #1476 addresses the hardcoded default value specifically. They did state that the library "breaks" for anyone not using the third segment, which is partially true -- it doesn't work without the uri_segment explicitly set. If they had done that, pagination would've worked just fine.

Issue #1909 -- A direct quote from the dev:

I know it can be solved just by changing the uri_segment in the config array passed to initialize pagination class

Never was the use of segment() in general an issue -- only the use of defaulting to the third segment to find the page/offset.

Explaining how a complicated mix of code (the URI, Router classes) works is demeaning to you, but hinting me on how to find the last element of an array shouldn't be to me? :)

I rewrote my whole comment in an effort to cover all bases, and provide examples of issues and their solutions. That code is part of that comment, not meant to be addressed specifically towards you. I'm sorry if you took offense to it, that was not my intention. I regret mentioning demeaning comments earlier in light of we probably misunderstood the hell out of each other.

Unless one bug can't be fixed without reworking something else - they should be submitted separately.

I'm saying this PR needs reworking and stuff anyway to address its own bug and a bug related to it, which I haven't described yet. So a new PR for that might be better suited, meaning this conversation can be converted into just about segments. I don't know if there's a way to do that, that's what I was asking.


That I agree on, I've agreed on it before and it most likely how it should and will end up as a final solution.

Our arguments will never match, but it looks like we both agree on a better solution - that's what matters.

I'd just like to point out that my solution involves reverting the library back to using all segment() calls, and adding better logic to detect the default segment number. If you do indeed agree with this, then we don't have anything else to argue about, because that fixes all the issues I've described thus far in this entire thread.

Contributor

cryode commented Jan 24, 2013

@alexbilbie @derekjones @philsturgeon @ericbarnes @pkriete -- I'm sorry to drag anyone into this, but I think we could really use an additional perspective here. You don't have to read everything (we repeat ourselves a lot, lol), but please take a look if you have a moment.

Contributor

narfbg commented Jan 24, 2013

We just agreed on using the last segment and now you ping people? :)

Just PR the changes needed. If what you've described as "the solution" is what you've meant all along - a pull request with it would literally be worth well above a thousand words, this whole conversation wouldn't happen and it didn't even belong here in the first place.

Contributor

cryode commented Jan 25, 2013

I'm working towards fixing the original problem with the empty query string. In the meantime, you should be able to specify the first_url config option when initializing the pagination library to prevent an empty query string from being appended.

Contributor

cryode commented Jan 28, 2013

Completed in #2199.

@cryode cryode closed this Jan 28, 2013

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