User tests: Successful: Unsuccessful:
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.
index.php/en/component/content/featured
limitstart=0
or empty limitstart=
start=0
No more limitstart
parameter in URL when mode sef is on.
The parameter query limitstart=0
stay in URL.
No
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries Unit Tests |
Title |
|
I have tested this item
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
Status | Pending | ⇒ | Ready to Commit |
Ready to Commit after two successful tests.
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 :)
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.
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.
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)
.
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.
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.
I have tested this item
Breaks Pagination in eg SermonSpeaker as you can't navigate anymore back to page 1
Status | Ready to Commit | ⇒ | Pending |
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;
}
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?
I do not understand why it break something.
Found it: #3725
That PR was about removing it when zero
This PR is about using &start= instead of &limistart= regardless of its value
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
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
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 :)
if the extensions are built to read 'start' variable they should be fine
It's Joomla code, not extension code
joomla-cms/libraries/src/MVC/Model/ListModel.php
Lines 634 to 636 in fab5637
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 ?
OK, I found the problem:
joomla-cms/libraries/src/Router/SiteRouter.php
Lines 600 to 603 in fab5637
there is requires one more strict comparison.
I have tested this item
Looks fine now with my extensions. I can get back to page 1.
Haven't tested core extension though.
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
Ready to Commit after two successful tests.
I have tested this item
For kunena it failed the url.
Before patch
After Patch:
hhmm quick test
it works with Joomla category view and FLEXIcontent category view going back to page 1 when
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)
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 ?
Status | Ready to Commit | ⇒ | Pending |
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.
Yes, that worked, But why do we want to change it to start=0.
Is this not a bad seo change?
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
.
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
In this PR I just want to add some standardization:
http://kunena.test/develop/forum/welcome-mat/18-wcw
http://kunena.test/develop/forum/welcome-mat/18-wcw?start=0
http://kunena.test/develop/forum/welcome-mat/18-wcw?start=20
...
because at now (on staging) we have:
http://kunena.test/develop/forum/welcome-mat/18-wcw
http://kunena.test/develop/forum/welcome-mat/18-wcw?limitstart=0
http://kunena.test/develop/forum/welcome-mat/18-wcw?start=20
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.
I have tested this item
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.
@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.
Status | Pending | ⇒ | Ready to Commit |
RTC
i am removing the rtc status as it is clearly not ready
maybe move to 3.9 then everyone is happy
Yes, it can wait for 3.9. I'm fine with that.
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.
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 ?
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.
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.
OK, I exaggerated, "a little bit" is a better word.
Test results in Kunena 5.0.13 =
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.
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).
Should we even consider it for J4?
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.
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).
Thanks for the clarification.
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:
?
?
|
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.