? ? Pending

User tests: Successful: Unsuccessful:

avatar csthomas
csthomas
25 Jan 2018

Summary of Changes

If SEF is on, then joomla replace almost every occurrence of parameter limitstart to start in pagination.
When parameter limitstart is empty or equal to 0 then it stay in URL.
This PR fix it.

Testing Instructions

  1. Mode SEF is on.
  2. Mark a few articles as featured to get a few page of featured articles.
  3. Go to featured view ex: index.php/en/component/content/featured
  4. Go to the second page.
  5. Check links for start/previous/first page:
    • before PR there is URL parameter limitstart=0 or empty limitstart=
    • after PR the parameter has been replaced by start=0

Expected result

No more limitstart parameter in URL when mode sef is on.

Actual result

The parameter query limitstart=0 stay in URL.

Documentation Changes Required

No

avatar csthomas csthomas - open - 25 Jan 2018
avatar csthomas csthomas - change - 25 Jan 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 25 Jan 2018
Category Libraries Unit Tests
avatar Quy
Quy - comment - 25 Jan 2018

I have tested this item successfully on a5cf2c5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19452.

avatar Quy Quy - test_item - 25 Jan 2018 - Tested successfully
avatar csthomas csthomas - change - 25 Jan 2018
Title
Replace &limitstart=0 to &start=0 if SEF is ON
Replace the URL parameter "limitstart=0" with "start=0" if the SEF mode is on
avatar csthomas csthomas - edited - 25 Jan 2018
avatar jsubri
jsubri - comment - 26 Jan 2018

I have tested this item successfully on a5cf2c5

With SEF ON, the URL is now updated with index.php/en/archive?month=0&year=0&start=0 instead of index.php/en/archive?month=0&year=0&limitstart=0
With SEF OFF, limitstart is still present in the URL index.php?option=com_content&view=archive&catid[0]=109&Itemid=246&lang=en&limitstart=0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19452.

avatar jsubri jsubri - test_item - 26 Jan 2018 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 26 Jan 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 26 Jan 2018

Ready to Commit after two successful tests.

avatar Bakual
Bakual - comment - 26 Jan 2018

I think we had that same PR before and it broke the CMS. If I remember right it was impossible to go back to page 0 afterwards. I'll see if I find the PR :)

avatar csthomas
csthomas - comment - 26 Jan 2018

This PR changes param name limitstart to start only.

You are mention about removing limitstart=0 at all.
But I have a plan to add it too, in separate PR. If you find it I will take a look. It would be helpful.
BTW If limitstart in not placed anywhere in session/cookie then it should be safe.

avatar Bakual
Bakual - comment - 26 Jan 2018

Found it: #3725 was the PR back then and I had to fix it with #4488 (J3.4.0).

Core wasn't affected by this sine it doesn't store the pagination in user session in frontend (it does store it in backend). Some 3rd parties like my own SermonSpeaker or Kunena however store it and were affected.

So please test this with 3rd parties that store the pagination state, not only with core.

avatar csthomas
csthomas - comment - 26 Jan 2018

This PR is safe because it changes limitstart=0 to start=0, see it in comment #19452 (comment)

This PR uses strict comparison (limitstart !== null).

avatar csthomas
csthomas - comment - 26 Jan 2018

IMO putting a page number in session is wrong and should not be supported. Page number always should be placed in GET or POST request. I would like to write a fix for that in J4.

avatar mbabker
mbabker - comment - 26 Jan 2018

IMO putting a page number in session is wrong and should not be supported.

Well, depending on user workflow, there are cases where you'd want to come back to a page (i.e. editing content on page 5, should save and close put you back on page 1?). Unless you're going to persist the pagination parameters in every request, the session is the only other place to do it.

avatar Bakual
Bakual - comment - 26 Jan 2018

I have tested this item ? unsuccessfully on a5cf2c5

Breaks Pagination in eg SermonSpeaker as you can't navigate anymore back to page 1


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19452.

avatar Bakual Bakual - test_item - 26 Jan 2018 - Tested unsuccessfully
avatar Bakual Bakual - change - 26 Jan 2018
Status Ready to Commit Pending
avatar csthomas
csthomas - comment - 26 Jan 2018

Hmm,

What about store page in session as before but do not use it directly on list view but earlier when a link to the list is generated.

For example, I want to edit an article=123 from current page=5. Session['page'] is saved as 5.
Now I save or close article and want to back to the list.

// Redirect to the list to the page 5
if (session[page] > 1) {
    $link = 'index.php?view=list&page=' . session[page];
else {
    $link = 'index.php?view=list;
}
avatar Bakual
Bakual - comment - 26 Jan 2018

I've removed RTC for now.
Do you want to try and fix it so 3rd parties don't get broken or shall we just close it?

avatar csthomas
csthomas - comment - 26 Jan 2018

I do not understand why it break something.

avatar ggppdk
ggppdk - comment - 26 Jan 2018

Found it: #3725

That PR was about removing it when zero

This PR is about using &start= instead of &limistart= regardless of its value

  • by also including the case of using &start=0 instead of &limitstart=0

if the extensions are built to read 'start' variable they should be fine

Also loading URLs that have page number in session sounds very wrong

  • you can not bookmark it
  • search engines can not index it properly

Well, depending on user workflow, there are cases where you'd want to come back to a page (i.e. editing content on page 5, should save and close put you back on page 1?). Unless you're going to persist the pagination parameters in every request, the session is the only other place to do it.

Why not just store the full URL to go back too ?

i mean why store in session

  • URL without page
  • and then seperately in session store the page ?
avatar Bakual
Bakual - comment - 26 Jan 2018

It's not just about editing an item. I expect it to work in my component so when a visitor listens to a sermon from page 3 and then gets back to the list (either using menuitem or browser navigation) it should still be on the same page where he was before. User session is perfectly fine for that.
And of course he also should be able to navigate back to the first page, which this PR breaks :)

avatar Bakual
Bakual - comment - 26 Jan 2018

if the extensions are built to read 'start' variable they should be fine

It's Joomla code, not extension code ?

$value = $app->getUserStateFromRequest($this->context . '.limitstart', 'limitstart', 0, 'int');
$limitstart = ($limit != 0 ? (floor($value / $limit) * $limit) : 0);
$this->setState('list.start', $limitstart);

avatar ggppdk
ggppdk - comment - 26 Jan 2018

It's Joomla code, not extension code ?

hhmm i think that code is not related,
because that codes says "i only read 'limitstart' value"

which logically means that if only read 'limitstart' value, and our pagination works, then somewhere before this code the limitstart was set to the value of 'start'

and this should have happened during SEF URL routing ... right ?

avatar csthomas
csthomas - comment - 26 Jan 2018

OK, I found the problem:

if ($start = $uri->getVar('start'))
{
$uri->delVar('start');
$vars['limitstart'] = $start;

there is requires one more strict comparison.

avatar csthomas
csthomas - comment - 26 Jan 2018

@Bakual Could you test it now, after the last commit?

avatar Bakual
Bakual - comment - 26 Jan 2018

I have tested this item successfully on 9a1855c

Looks fine now with my extensions. I can get back to page 1.
Haven't tested core extension though.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19452.

avatar Bakual Bakual - test_item - 26 Jan 2018 - Tested successfully
avatar csthomas
csthomas - comment - 26 Jan 2018

@jsubri @Quy Could someone of you retest it?

avatar grhcj
grhcj - comment - 26 Jan 2018

I have tested this item successfully on 9a1855c


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19452.

avatar grhcj grhcj - test_item - 26 Jan 2018 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 26 Jan 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 26 Jan 2018

Ready to Commit after two successful tests.

avatar 810 810 - test_item - 26 Jan 2018 - Tested unsuccessfully
avatar ggppdk
ggppdk - comment - 26 Jan 2018

hhmm quick test

it works with Joomla category view and FLEXIcontent category view going back to page 1 when

  1. Go to page 2
  2. Go to page 1 , URL is now ?start=0 (works)
  3. Go to page 2 and hit browser back to go to page 1 (works)

Also works for Joomla category view and FLEXIcontent category view
when we are at page 1 with or without ?start=0 at the URL
and we edit an article and then click cancel we return to page 1 with or without ?start=0 (respectively)

avatar ggppdk
ggppdk - comment - 26 Jan 2018

in any case if this does not work with popular extensions regardless of who is to be blamed, then it cannot be in J3.8.x

remove RTC ?

avatar Quy Quy - change - 26 Jan 2018
Status Ready to Commit Pending
avatar csthomas
csthomas - comment - 26 Jan 2018

IMO there is a bug in kunena router.php

        if (isset($query['start']) && $query['start'] == '@')
        {
                $query['start'] = '%' . ++$pos . '$d';
        }

When $query['start'] === 0 then it generates such problem because 0 == '@' is true. There should be strict comparison.

avatar 810
810 - comment - 26 Jan 2018

Yes, that worked, But why do we want to change it to start=0.

Is this not a bad seo change?

avatar csthomas
csthomas - comment - 26 Jan 2018

What is the difference for SEO between limitstart=0 and start=0?

I would like to remove ...start=0 at all but this break B/C as @Bakual mentioned above.
Probably the best solution would be to add additional parameter ($append_start_zero = true by default) to Pagination constructor and set it to false in order to remove start=0.

avatar 810
810 - comment - 26 Jan 2018

Yeah for the removing the part, i agree.

But now search engines will crawl both pages. Like as example our forum component.

On our support forum i have linked some answers to the limitstart url. If joomla merge this pr. Then search engines see the same content and will give error on the google search console.

I know that you can redirect the pages. But for our users who also linking the url's to their forum. They should all add a custom htaccess rewrite rule.

So i see only a bad impact for our users, no reason why it to change it to start=0

avatar 810
810 - comment - 26 Jan 2018

ok, i will add the router fix.

I agree to see some standardization. For new install its ok, but for others it can be having some negative seo effects.

My 2 cents is to close this pr, and remove it on Joomla 4.0

But if other devs still want it, it can be merged.

avatar ggppdk
ggppdk - comment - 26 Jan 2018

I have tested this item successfully on 9a1855c


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19452.

avatar ggppdk ggppdk - test_item - 26 Jan 2018 - Tested successfully
avatar Bakual
Bakual - comment - 26 Jan 2018

Imho, this isn't worth risking to break 3rd party extensions. It really is a minor improvement.
We can do it for 4.0, but honestly I wouldn't completely remove it. Just standardize it to "start", I can see that point.

avatar sshcli
sshcli - comment - 26 Jan 2018

@csthomas thanks for bring this topic to discussion.

IMHO, Joomla pagination should look like this:
Page 1: example.com/example-article
Page 2: example.com/example-article-2
Page 3: example.com/example-article-3

I know this was not your intention when created this PR, but...
This will have a high positive impact in SEO, that's for sure.

avatar csthomas
csthomas - comment - 26 Jan 2018

@sshcli Please wait a little I will add some separate PR for that, may be not with beautiful code at start but complete solution with B/C.

avatar Quy Quy - change - 26 Jan 2018
Status Pending Ready to Commit
avatar Quy
Quy - comment - 26 Jan 2018

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19452.

avatar csthomas
csthomas - comment - 27 Jan 2018

I added a separated PR #19467 for SEO improvements, which may remove ?limitstart=0 or ?start=0

avatar brianteeman
brianteeman - comment - 27 Jan 2018

i am removing the rtc status as it is clearly not ready

avatar csthomas
csthomas - comment - 27 Jan 2018

This PR is complete and I do not have any plan to change it.
If it won't be accepted then I will move it to J4 or close.

#19467 try to change the other part of code.

avatar 810
810 - comment - 27 Jan 2018

maybe move to 3.9 then everyone is happy

avatar csthomas
csthomas - comment - 27 Jan 2018

Yes, it can wait for 3.9. I'm fine with that.

avatar Bakual
Bakual - comment - 27 Jan 2018

Merging into 3.9.0 makes no sense.
It can either go into the next patch release (if it is B/C) or it has to go into 4.0 (if it is not B/C). So our evaluation if it is B/C dictates in which release it goes, next release or 4.0.

avatar ggppdk
ggppdk - comment - 27 Jan 2018

@Bakual

yes there is nothing important about this PR, other than having more proper SEF pagination URLs
[EDIT]
i mean regardless of calling it B/C or not
you are right , better do it in J4

@810
not related to this PR but related to removing limitstart / start from first page URLs aka related tto PR #19467

why store page number into session instead of storing a full return URL ?

and then again a similar question
if you know the return URL and then you also know the page number
why not create the full return URL that also includes the page number ?

having the same URL return different content seems wrong, if it was a posted form, then ok, but it is not a posted form

[EDIT]
does this behaviour work well with rel-canonical ?

avatar Bakual
Bakual - comment - 27 Jan 2018

Pagination comes into play when there is a list of items (eg articles). That list changes anyway as soon as there are new items added or if you apply search filters (which are stored in user session). So I don't see an issue with storing the page in user session as well. Technically, it's just another "filter" that is applied.

I don't care if Google indexes that list page correctly, there is no point indexing that anyway. It should (and does) index the individual items, that is the important page.

avatar csthomas
csthomas - comment - 27 Jan 2018

I invite everyone to test another PR #19467 that can significantly a little bit improve SEO for pagination.

avatar Bakual
Bakual - comment - 27 Jan 2018

can significantly improve SEO for pagination.

Any evidence for that statement? Maybe you can add that in said PR. Personally I doubt it increases SEO at all, it's just a visual improvement for humans.

avatar csthomas
csthomas - comment - 27 Jan 2018

OK, I exaggerated, "a little bit" is a better word.

avatar sshcli
sshcli - comment - 27 Jan 2018

Test results in Kunena 5.0.13 = Pass

avatar mbabker
mbabker - comment - 13 Feb 2018

As long as usages of limitstart in this condition do not break, then IMO this is fine for a patch release. But we need to ensure that code expecting limitstart to be present and with a value doesn't break for it to be accepted.

avatar csthomas
csthomas - comment - 16 May 2018

This PR is B/C but may generate errors for improper (not strict) comparison on 3rd party extensions, like at mentioned kunena forum<5.0.13.

I won't to force this PR as a patch for 3.8.x, because developers of 3rd party extensions may be unpleasantly surprised.

I would like to see it in 3.9 or later (together with #19467).

avatar laoneo
laoneo - comment - 17 May 2018

Should we even consider it for J4?

avatar csthomas
csthomas - comment - 22 Jun 2018

As long as usages of limitstart in this condition do not break, then IMO this is fine for a patch release.

This PR is full B/C.

There is a change that can break the faulty code in a 3rd party extension, like mentioned in Kunena 5.0.12 and older.

As a lot of people does not like such improvements in patch releases, so I suggest merging it into version 3.9/3.10.

avatar mbabker
mbabker - comment - 22 Jun 2018

As a lot of people does not like such improvements in patch releases, so I suggest merging it into version 3.9/3.10.

Well I'm not "a lot of people". SemVer decides where this lands, if it's a bug fix then it goes into next scheduled maintenance release; if it's a B/C break it goes into next major release. I don't see anything about this being a feature so there is no need to explicitly hold it for 3.9.0 or 3.10.0 because SemVer doesn't have a notion of "bug fixes which may be inconvenient for someone but aren't a B/C break should go into a minor release". Either it's a bug fix or a B/C break. At this point I'd rather not put it into 3.8.9 just because it's been sitting for a while and I intend on building RC when I get off work this afternoon (so with this being mildly controversial I'd rather not merge last minute before a RC build), but I'll merge this right after stable release (whether that next release is 3.8.10 or 3.9.0 is still to be determined).

avatar csthomas
csthomas - comment - 22 Jun 2018

Thanks for the clarification.

avatar mbabker mbabker - change - 29 Jun 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-06-29 01:40:48
Closed_By mbabker
Labels Added: ? ?
avatar mbabker mbabker - close - 29 Jun 2018
avatar mbabker mbabker - merge - 29 Jun 2018

Add a Comment

Login with GitHub to post a comment