Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

URL prefix and suffix were not being applied to the link for page 1 #1414

Closed
wants to merge 2 commits into from

2 participants

@ckd

No description provided.

@narfbg
Owner

Applied the style guide typo fix, but after looking at the prefix/suffix code - it's obvious that this is intentional for the first page, it's not a bug. And there's a reason for that - you shouldn't have 2 links for the same page.

@narfbg narfbg closed this
@narfbg narfbg referenced this pull request
Closed

Bug fixes with pagination #2016

@ckd

Actually, it obviously IS a bug.

If you apply a suffix or prefix and click through to another page, you lose both in the link back to the first page.

Prefix and Suffix should be applied to ALL links in pagination. Not sure why you think that would lead to having 2 links for the same page.

My patch, as it turns out, was a giant pile of garbage and only really worked for our particular use case, doesn't fix the link for "First". But the bug is definitely there.

@narfbg
Owner

Isn't the first link your index page for whatever you're paginating? And what get lost, functionally (excluding the suffix and/or prefix) ?

As obvious it is for you that it's a bug, it's just as obvious that it is done intentionally. I don't consider intentional behavior to be a bug.

@ckd

Dropping the prefix/suffix intentionally for any page, first or not, (especially without documenting it) is bizarre and, if such bizarre behavior is intentional, should be documented as such.

A real-world scenario would be using a url segment to specify sort order. With the current code, one can persist the sort order by adding it as a suffix. But the problem is that, with the existing code, only pages 2 through infinity would be sorted properly- page 1 would not be. So if I'm on page 4 and click through to page 1, my list would be incorrectly sorted.

@narfbg
Owner

It's not bizarre to anybody doing SEO - all of them would tell you that you shouldn't have 2 different links pointing at the same content. I do agree that it should be mentioned in the documentation though.

Let's take for example the news section explained in our tutorial. You'd have news/index as your index page for all news, and that would also be your first page. Why would you link to the sections itself as news/index and then if you have pagination link to the same page (content) as e.g. news/index.html? It doesn't make sense to me.

Sort options (and any other parameters) are a different case, which is not covered by the prefix, suffix options. A prefix and/or suffix is a hard-coded value, I don't think that it should be used in actual application logic. There are query strings and the reuse_query_string option to use for that purpose. If that doesn't work - then I'd agree it is a bug.

@ckd

Your example is flawed, in that:

a) You assume that a duplicate link exists elsewhere in my content.

b) You assume that the presence of a duplicate link in my content is somehow within the purview of this library or any other library.

c) Nobody is talking about the distinction between index and index.html

This isn't the SEO library, this is the Pagination library. Thus, it should serve the needs of as many people as possible, not optimized for a very specific (and again, undocumented) use case that isn't relevant to the rest of us.

But that aside, adding a sort or filter segment to a URI DOES make sense, as vehicles/10/cars is a different resource than vehicles/10/boats.

Moreover, what a developer uses the prefix or suffix for is, quite frankly, none of your business. If the functionality exists at all, it should work consistently. Anything else is unexpected behavior and therefore a bug.

@narfbg
Owner

a) You assume that a duplicate link exists elsewhere in my content.

I assume that 99 out of 100 developers will not link to their e.g. "articles" page with "articles/page/1.html", which makes your content a non-common case. Suffixing the page 1 link for them is exactly what would duplicate it.

b) You assume that the presence of a duplicate link in my content is somehow within the purview of this library or any other library.

That's the problem - you complain that the prefix and suffix are not applied, somebody else will complain for the opposite behavior. How do you decide who's got the better argument?

c) Nobody is talking about the distinction between index and index.html

True, even I'm not - see my comment on a) above.

This isn't the SEO library, this is the Pagination library. Thus, it should serve the needs of as many people as possible, not optimized for a very specific (and again, undocumented) use case that isn't relevant to the rest of us.

It all comes down to assumptions here and again - see my comment on point a). Honesly, you're the first one that I know of who's actually losing any functionality (due to your use of the suffix as a filter). On my end - that makes your use case very specific, not the opposite.

But that aside, adding a sort or filter segment to a URI DOES make sense, as vehicles/10/cars is a different resource than vehicles/10/boats.

Agreed, but this is an URI parameter and not a suffix. Furthermore, every pagination feature that you'll find will work in a linear fashion and even your example would first filter out by either cars or boats and only then it will count the number of matched items. I don't want to sound like I'm simply refusing your arguments, but those URI examples are IMO the result of a bad application design.

Moreover, what a developer uses the prefix or suffix for is, quite frankly, none of your business.

Indeed - none of my business, but that doesn't make it the right way to do business. Nobody wants to implement a feature that's not done the right way (unless they are paid, so that they wouldn't care).

If the functionality exists at all, it should work consistently.

This is the one thing that I completely agree on. However, my next comment questions it:

Anything else is unexpected behavior and therefore a bug.

I'll repeat again my initial reasoning - the code itself makes it obvious that the intention is to not have a prefix/suffix for the first link. You can't label something as "bug" when it is that evident that it's a feature. I am not the author of that code, but whoever it is - this wasn't accidental and they had a reason to do it.


This is quickly getting into my reasoning vs. yours and is not productive, so we should get some input on it.

@robinsowell (initially implemented the feature)
@alexbilbie @pkriete @ericbarnes @seejohnrun

Care to comment?

@ckd

Thankfully, your opinons on application design aren't relevant here, you again miss the point:

The functionality to apply a suffix and/or a prefix exists. What that functionality is used for is, again, not your concern- it is not your place to determine how people should and should not use CodeIgniter. Your concern should be that said functionality is both consistent AND well documented. It is neither.

A link for page 1 should be generated in the exact same fashion as a link for page n.

Anything else is just a distraction.

It may have been intentionally coded that way, but the point again is that as well intentioned as it may be, it is still incorrect behavior.

@narfbg
Owner

I'm not missing anything and I've given you credit where it's due. Still, our opinions differ and we can go back and forth forever, repeating the same things over and over again.

Either way, I can't and won't do anything about it until some more opinions are given and we can have a conclusion on this. I've invited the original author and the rest of the Reactor team to join the conversation. Let's just wait, shall we?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 31, 2012
  1. @ckd
  2. @ckd

    Typo

    ckd authored
This page is out of date. Refresh to see the latest.
View
3  system/libraries/Pagination.php
@@ -256,8 +256,7 @@ public function create_links()
}
else
{
- $n = ($n == '') ? '' : $this->prefix.$n.$this->suffix;
-
+ $n = $this->prefix.$i.$this->suffix;
$output .= $this->num_tag_open.'<a '.$this->anchor_class.'href="'.$this->base_url.$n.'">'.$loop.'</a>'.$this->num_tag_close;
}
}
View
2  user_guide_src/source/general/styleguide.rst
@@ -52,7 +52,7 @@ whether introduced by the developer, user, or an FTP application, can
cause unwanted output, PHP errors, or if the latter are suppressed,
blank pages. For this reason, all PHP files should **OMIT** the closing
PHP tag, and instead use a comment block to mark the end of file and
-it's location relative to the application root. This allows you to still
+its location relative to the application root. This allows you to still
identify a file as being complete and not truncated.
**INCORRECT**::
Something went wrong with that request. Please try again.