User tests: Successful: Unsuccessful:
Pull Request for Issue #11103.
Fix the ordering of articles and do not loose performance.
Notice:
I do not know does 2147483647 is good number to be max of ordering field (look at code) across all database types?
[UPDATED]
Download and Install Joomla 3.6.0 with sample data "test", "testing" or something like that
Install com_patchtester.zip latest stable version
On backend go to Menu -> All Front End Views -> Featured Articles
In layout search for "Category Order" and change to "No Order". Article Order should be set to "Featured Article Order"
Go to /index.php/featured-articles
Back to backend and add a new article with title "Featured 1".
Set it as featured. Category could be "Uncategorised"
Go to /administrator/index.php?option=com_content&view=featured
Change select box "Sort Table by:" to "Ordering ascending"
Your new added article "Featured 1" should be at the bottom
Go to /index.php/featured-articles
and try to find your article "Featured 1", should at the end
Go to /administrator/index.php?option=com_patchtester
Fetch Data
Search for "Fix the ordering of articles and featured"
Apply Patch 11139
Add next article with title "Featured 2", set as featured. Category could be "Uncategorised"
Go to /administrator/index.php?option=com_content&view=featured
Sort Table by should be "Ordering ascending"
Now "Featured 2" is on the top and "Featured 1" is the last one.
Go to /index.php/featured-articles
and try to find your article "Featured 2", should at the top
Go back to /administrator/index.php?option=com_content&view=featured
and reorder "Featured 1" to the first place.
Go to /index.php/featured-articles
and "Featured 1" and "Featured 2" should at the top.
You can do a few more detailed test if you like to.
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
@killoltailored I updated instruction. You have to use ex. Article Order in category/menu item/modules.
I can add parameter (but better add it only in the next PR):
public function reorder($where = '', $use_big_numbers=false)
and then use it as:
if ($use_big_numbers || in_array($this->_tbl, array('#__content', '#__content_frontpage'), true))
or do it in the most right way and replace it by:
if ($use_big_numbers)
Then there maybe more changes in inheritance class. (need to add additional parameter too)
There aren't overrides of the reorder methods necessarily, but there is a JTableContent
and ContentTableFeatured
subclass specific for these two tables.
And without understanding the issue or if said parameter/behavior is multi-database compliant (remember Joomla is still playing the charade of supporting non-MySQL engines), I'm not commenting further on behaviors.
There aren't overrides of the reorder methods necessarily, but there is a JTableContent and ContentTableFeatured subclass specific for these two tables.
This is good point. I will wait for more comments.
Please take a look how it works in others models:
For others models ordering is set to 0 and is not reordered on adding.
For ex. create a new module.
If that patch should work without exceptions then above files (banner, contact, newsfeed) have to change way of adding a new item.
And forgot to say , should work in all database servers ? someone please confirm for sqlazure
Please see test #11139 (comment)
Made a mistake here.
I have tested this item
What I've done.
Current staging.
Added into HEAD of ISIS index.php to see ordering values in articles list.
<style>
body .text-area-order {
display: inline !important;
}
</style>
1 category / 1088 articles.
Lowest ordering -24 (set by a plugin and/or override that uses old fashioned ordering or similiar).
Several articles with different ordering < 0.
Several articles with ordering 0.
Several articles with ordering from 1 to 11.
Added new article aaaaa.
Gets ordering 12.
Other orderings unchanged.
Added new article bbbb.
All orderings < 0 stay unchanged.
New article gets ordering 2147482318. Whyever.
ALL(!!) OTHER orderings get 2147482318, too. Whyever.
From my point of view this is a fatal break of B/C while behavior before patch is a more or less acceptable break of B/C.
And these ordering numbers are completely intransparent. EDIT: If you don't use Drag&Drop feature but old fashioned ordering in a component.
I have tested this item
Sorry! Made a mistake while previous test : #11139 (comment)
That's wrong there:
ALL(!!) OTHER orderings get 2147482318, too. Whyever.
Correct:
ALL OTHER orderings are > 2147482318 (1-stepwise).
But I still don't like these high ordering values ;-) .
But I still don't like these high ordering values ;-) .
Yes it is a problem with that.
Review of solutions:
1) #11134 (currently closed) - I want to see that solution (now incomplete) on Joomla 4:
2) Current PR is:
3) #11184
4) back to 3.5.1 solution but surround queries loop with transaction start and commit
new row may get ordering=0 or ordering=max+1
Why not ordering=min-1 for new entries?
Ordering is a signed field. Negative values are allowed.
And only reorder table starting at 0 when field size needs it.
Yes this also can be an option but:
Negative ordering number are sticky orderings , not subject to reordering, this is an existing feature, if we want to break, or remove this feature then it will break more sites
@csthomas
as you say it does not have to trigger a re-ordering on every save
also about my PR #11184,
the SQL update cost will be very small , the cost when having e.g. 2000 records to reorder will be
$rows = $this->_db->loadObjectList();
and then the PHP loop that create the update SQL query, as i would like to avoid function calls e.g.
$pk = $pk_is_int ? $row->$keyname : $this->_db->quote($row->$keyname);
as the above despite being very little cost it executed for every record
The SQL query itself will cost / should cost a lot less than the above !
and in any case SQL cost should be reduced by 99.5%-99.9% when re-ordering 2,000 records
I'm fun of solution 1) from comment above #11139 (comment)
but it breaks B/C everywhere then maybe I have to wait for accepting that to Joomla 4.
Your solution Georgios is good, your PR does not have problems with big values,
performance is better because it use only a few query,
but
a) For content_frontpage:
My client has table #__content_frontpage
with 174678 total rows.
All tables that use ordering when adding a new row is usual fast, except big one #__content_frontpage
, and less #__content
.
I see some solution: maybe use your PR or other and then:
- remove content_frontpage table from database.
#__content_frontpage.ordering
move to #__content.featured
column which only use 0 or 1 but can have NULL or ordering value from frontpage.
b) Using reorder after add a new row is weird but we may have no choice.
Why I want to remove any re-reordering when adding a new row?
[Please check my below words. I may be wrong.]
How does replication of data work in mysql myisam (usually)?
How does replication of data work in mysql innodb?
On joomla 3.5.1, for table #__content_frontpage
on each adding a new featured article, it probably transfer all or half size of the table.
Replication data for joomla is hard.
A few month ago I disabled it and now I'm not used it but I still remember this problem.
Back to your comments:
Negative ordering number are sticky orderings , not subject to reordering, this is an existing feature, if we want to break, or remove this feature then it will break more sites
Then we can not use that
it can start re-ordering at e.g 100 and thus reordering is triggered rarely and for extensions that display the ordering number it will be more user friendly
If I well understand you want to have a buffer of [1-100] for new rows and I can add first 99 without reordering, next will be with re-ordering, and next 99 without, ....
Hmm, may be
the SQL update cost will be very small , the cost when having e.g. 2000 records to reorder will be
I'm maybe alone but my client has 174678 rows.
The best way will be to not load that rows and reorder it internal in database.
No one has found a solution for sqlazure and postgresql.
Arguably if you use MySQL max_packet_size of 4 Mbytes, instead of default 1 MB
or with defaults settings in my PR, you will need 18 queries as it defaults to 10000 records per query
So after my PR the sql queries are no longer the problem
even this is slow and needs a lot of memory because it will retrieve 174000 records
$rows = $this->_db->loadObjectList();
I am not against this PR i will test this PR,
And if this PR is accepted
The part of my PR that uses single query for updating multiple records is an improvement that can and should be added anyway to the reorder method , (of course then my new PR will not touch article.php)
so i say let's test this PR first, it seems good !
To get the best performance with B/C we can join our patches as you said.
If PLT guys can agree with me about change/reverse ordering in the future joomla 4 and always use max+1 for a new row - this break B/C (example from #11134 applied on current 3.6).
Then I will close my PR and will accept your only.
If joomla not planing to use above in the future then we could join our patches.
If only your will be merged then privately I will still use my current below patch (mysql only).
diff --git a/libraries/joomla/table/table.php b/libraries/joomla/table/table.php
index 121dbf2..c317bb0 100644
--- a/libraries/joomla/table/table.php
+++ b/libraries/joomla/table/table.php
@@ -1378,6 +1378,36 @@ abstract class JTable extends JObject implements JObservableInterface, JTableInt
throw new UnexpectedValueException(sprintf('%s does not support ordering.', get_class($this)));
}
+ /* Speed up patch for mysql - start */
+ if (in_array($this->_tbl, ['#__content', '#__content_frontpage'], true))
+ {
+ $select = 'SELECT @a := @a - 1 FROM (SELECT @a := 2147483647)';
+ $dir = 'DESC';
+ }
+ else
+ {
+ $select = 'SELECT @a := @a + 1 FROM (SELECT @a := 0)';
+ $dir = 'ASC';
+ }
+
+ $query = $this->_db->getQuery(true)
+ ->update($this->_tbl)
+ ->set('ordering = (' . $select . ' s)')
+ ->where('ordering >= 0')
+ ->order("ordering $dir");
+
+ // Setup the extra where and ordering clause data.
+ if ($where)
+ {
+ $query->where($where);
+ }
+
+ $this->_db->setQuery($query);
+ $this->_db->execute();
+
+ return true;
+ /* Speed up patch for mysql - end */
+
$k = $this->_tbl_key;
// Get the primary keys and ordering values for the selection.
I think test and accept this PR first
(because JTable reorder performance improvement involves a new query that should be indepedently accepted and make sure it is good for ALL database cases)
If others thing the same then should I change line?:
if (in_array($this->_tbl, array('#__content', '#__content_frontpage'), true))
and use subclasses JTableContent and ContentTableFeatured as suggested by @mbabker.
How to prevent the repetition of code of reoreder method?
Unfortunately you can't cleanly. You either make a new method in JTable
which seems a bit awkward for a specialized alternative behavior, bump us
to PHP 5.4 and use traits, or accept the duplication in subclasses, which
there's already a bunch of in Joomla.
On Tuesday, July 19, 2016, Tomasz Narloch notifications@github.com wrote:
If others thing the same then should I change line:
if (in_array($this->_tbl, array('#__content', '#__content_frontpage'), true))
and use subclasses JTableContent and ContentTableFeatured as suggested by
@mbabker https://github.com/mbabker.
How to prevent the repetition of code of reoreder method?—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11139 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfocVe0Q0rWh04KNnDN4Si5qACKVljks5qXUVkgaJpZM4JNBDj
.
I want to apply one of two options:
1) add method reorder_alternative (you can suggest better name) to JTable and in subclasses add method:
public function reorder($where = '')
{
return parent::reorder_alternative($where);
}
2) add parameter to JTable::reorder like:
public function reorder($where = '', $alternative=false)
but this option has one drawback for subclasses that has defined reorder:
$ php -a
Interactive mode enabled
php > error_reporting(-1);
php > class JTable { function reorder($where='', $alternative = false) { return $where;}}
php > class JTableContent extends JTable { function reorder($where='') { return $where;}}
PHP Strict Standards: Declaration of JTableContent::reorder() should be compatible with JTable::reorder($where = '', $alternative = false) in php shell code on line 1
Then I think I should take 1).
I do not want to duplicate the code too much because @ggppdk will have too many place to patch.
I can't bump you to PHP 5.4 because IMHO it should me merged in 3.6.1
We aren't creating a new global behavior though so I don't think it's necessarily important to create any form of "alternative" method in the base table class. We're talking about two tables here, you're going to have to live with a little code duplication for a bit. If it were applied to all tables it'd be another story but right now that's not the case. If you are determined to integrate this directly into the base reorder method then it must be done in a way that does not target specific table names (otherwise it is ONLY appropriate to place this in the table specific subclasses). Whether that be with method parameters or class variables is to be determined.
Nothing in the core dev strategy forbids adding new parameters to methods. Yes, it introduces strict standard warnings to subclasses which override the method, but that's a side effect of making changes. Otherwise by that argument you could say that the only way core could add parameters to methods is by introducing new methods which just creates API bloat. We aren't talking about a method defined in an interface so big deal if you slip a strict standard message into someone's code.
Then I will add parameter to JTable::reorder like:
public function reorder($where = '', $alternative=false)
and I will remove hardcoded table names
then I will add to JTableContent and ContentTableFeatured method:
public function reorder($where = '', $alternative=true)
{
return parent::reorder($where, $alternative);
}
Category | ⇒ | Administration Components Libraries |
For test drag&drop in articles/featured or categories list view do not use additional style like
<style>
body .text-area-order {
display: inline !important;width:70px;
}
</style>
(or similar from #11139 (comment))
to show numbers from ordering field because javascript does not work properly.
Can I ask to test?
I'm sorry for that comment but I do not like silence.
PLT guys or members if you can say a few words?
For example: this PR is (almost) OK or it is totally incorrect direction or I should still wait for decision.
This PR is full reversible.
After applied this PR; If you want to back to 3.5.1 (reorder method) then there won't be a problem.
Articles can be reordered back to lower numbers.
i will test, but first please read and answer some questions
If this PR makes use of large ordering numbers, to avoid re-ordering
Also since the reorder() performance of JTable can be fixed, another reason not to introduce the "alternative" reverse re-ordering
Just i am a little confused
You are forcing reverse ordering on "articles" and on "featured articles" ,
Also there should be a new Object property in JTable
I have problem with expression my thoughts in English.
If this PR makes use of large ordering numbers, to avoid re-ordering
then maybe the alternative ordering in reverse is not needed ?
This is not reversed. It is still ascending.
For auto conversion from <= 3.6.0 is needed.
When you add a new article after applied PR getEarlierOrder
method may return 0 or -1 then this is a signal to do auto fix numbers/reorder (https://github.com/joomla/joomla-cms/pull/11139/files#diff-f66af5cac40d67f3a63762ceb0c8b8c3R251).
Also since the reorder() performance of JTable can be fixed, another reason not to introduce the "alternative" reverse re-ordering
If you still want to reorder all table or category after each new article then yes.
But I want to avoid it.
I won't force my PR if nobody see benefits. I see them but for now I'm alone probably.
i don't mind alternative ordering in reverse being there, for extensions that will choose to use it, if Joomla built-in extensions themselves will not use, it is not bad allowing this in 3rd party apps, (preferably via a new object property (flag) of JTable class and not via a method parameter and thus avoid unneeded code)
This is not reverse ordering.
Before PR you have:
1,2,3,4,5,10,..., 1000
After PR you have:
2147482627, 2147482628, 2147482629, 2147482630, 2147482631, ..., 2147482636, ... , 2147483646
1 => 2147482627
...
1000 => 2147483646
It is still Ascending
But When I add new article with 2147482626 then I do not need to do more.
Without that PR you have to move 0 to 1 which means to apply offset=1 to all table or part of table.
Just i am a little confused
You are forcing reverse ordering on "articles" and on "featured articles" ,
Yes. I only applied that PR for articles and featured but in the future also other tables can start using it by changing subclass of JTable.
but all existing sites have already stored ascending orderings, what will happen ? will the stored orderings still work ?
what about drag and drop re-ordering , it assumes ascending orderings, right ?
Everything should works. This is still ASCENDING. But I can not force for all tables because some tables like banner add a new row at the end of ordering (MAX(ordeing) + 1
).
I'm wondering if I can display to users big numbers as low numbers:
big number - MIN(ordering) = number from joomla 3.5
Also there should be a new Object property in JTable
to define usage of large numbers or not, do not force this on all extensions , e.g. some extensions show the ordering number (myself i do not mind this, i will auto-shift it for display purposes only ...)
Yes, good point. Thanks.
I can use it, instead of parameter $alternative in reorder()
I can add a new property to JTable and then change it only for a few subclasses to true
.
Maybe you have some suggestion of name.
I always have problem with that.
@ggppdk I have added a new property to JTable as you say.
If this PR should also support tables which can add new rows at the end of ordering column then we can reduce $last = 2147483647;
to half (for example), but I do not think that will be useful.
From other issues I see that you want back sticky numbers and see that numbers directly in ordering column.
Adding code for recalculate that numbers to low numbers should not be too hard.
yes i see it , so this is good for testing,
I will test tomorrow, thanks
Good morning.
Anyone do this change with sucesss?
I do the changes, but not solve and the website down.
I reverse the changes and repeat each instruction in the patch slowly, but the result was the same. My joomla is update with the last version.
The solution for my user (A sweet lady with 83 years old, in love with joomla), is after insert new featured article, call in favorites for the especial URL to reordering (query) the articles. Work fine, but I dont like. Is dangerous and ungainly.
I am sorry i have not got around to testing it (yet)
I guess this is too late to be tested and added to J3.6.1 ?
@xupacabra If you use J3.6.0 then do not use modules mod_articles_category or similar for testing.
J3.6.0 has bug which was fixed in #11105.
I have added simpler and more detailed test instruction #11139 (comment)
I have tested this item
also testet with Hathor Administrator Template and Sticky Ordering(negative Ordering Numbers).
I have tested this item
Works,
(**) re-ordering happens only once, the very first time that the new code is used (which is the expected behaviour)
Tested with isis and hathor,
article manager and featured manager
I did a code review too ! looks good
I am sorry for late testing !! this should have been in J3.6.1 , but too late now
(some people will be disappointed, ok i don't know how many have seen the B/C break in J3.6.0 regarding the orderings, but i guess with the release J3.6.1 even more people will notice)
After this is merged i will update my PR for JTable::reorder() performance
(**) re-ordering happens only once, the very first time that the new code is used
and re-ordering happens if an feturead article drop to the end of the ordering (for example to the ordering-number: 2147483647 or 2147483646 )
and re-ordering happens if an feturead article drop to the end of the ordering (for example to the ordering-number: 2147483647 or 2147483646 )
yes of course,
but comment refers to re-ordering no-longer being triggered
and that is the purpose of using large ordering numbers
Now we have enough tests success.
What milestone/tag should I write in code:
* @since 3.6.1
What is required to RTC?
@since __DEPLOY_VERSION__
the __DEPLOY_VERSION__
string is replaced when the maintainers use bump.php script.
See https://github.com/joomla/joomla-cms/blob/staging/build/bump.php#L319
Thanks to all of you for your help.
Can I ask for RTC?
Status | Pending | ⇒ | Needs Review |
I have tested this, it works and it does what is says it would do. I must say I don't like the patch only for one reason, the big numbers in ordering. I always thought that giving a new article max+1 as ordering would be the right thing to do and it feels as we are trying to fix the problem we got with fixing a problem. I don't have a solution yet but I would like to have some more time to think about it, so I will set this to needs review and bring it into the PLT for a discussion.
I know, your PR #11475 only add ability to sort/use on frontend reversed ordering and not change too much.
The problem as you know is different approach (for ordering) in <= 3.5.1 and >= 3.6.0 for articles and featured.
You want to help users who has 3.6 but who cares about users with 3.5.1 and older.
I think that you (PLT guys) should first decide which way we go:
ordering=MAX+1
or "old ordering=0
with call reorder or this PR ordering=MIN-1
If you decide to go a new way then some conversion ordering (for J3.5.1 and lower) should be added.
Also there are a group of users who upgrade to J3.6 and now manually reorder new articles to first position.
What would you think about a "reverse ordering" button in the category manager? It would allow people to reverse the ordering, if needed and we could go with the simpler max+1 approach.
That button will decrease the re-configuration labour a lot,
for all sites that now use "ordering"
i see csthomas says:
If you decide to go a new way then some conversion ordering (for J3.5.1 and lower) should be added.
which i agree it would have been nice,
if it was even more automated, e.g. on new Joomla upgrade
but i think it will be much more work to do the above , and also a quite "tricky" and maybe a risk of breaking things ?
so i would vote to go for the button in category manager
ok, I will try to make a PR today
i see csthomas says:
Only minus is large numbers in ordering column (which J3.6 does not display now).
This is not correct because numbers in ordering column was also display in
Hathor-Admin-Template and for example Isis-Template-Override:
#11200 (comment)
and maybe other extensions(3rd party) who use the numbers in ordering column ??
I admit this is a problem, it depends on template.
I only checked the default template.
If we want to display large numbers as more human (lower numbers) then changes in template may be needed, but that break B/C for 3rd party templates.
This PR does not have many followers and probably won't be merged.
Then we have to wait for J4 for reverse ordering and use MAX+1.
Status | Needs Review | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-08-22 20:19:50 |
Closed_By | ⇒ | rdeutz |
I agree that this PR is useful regardless of reverting #8576
also this PR can be changed to reorder records starting at 100 instead of: 2147483644
it will good for performance to test and accept
as the will both give performance benefits, that are needed regardless of reverting #8576
please re-open
I have tested in Joomla 3.6.0 and I have follow below steps to test article ordering
1) First created a menu item Category List and assigned article category to it.
2) Then added all articles in assigned article category.
3) I have used drag-drop ordering functionality and changed ordering of articles but not able to see articles in proper order at front-end side.
4) Without applying PR created article and I can see it at the top of the article list at front end side.
5) With PR having same result, created new article and I can see at the top of the list.
Please correct me if doing anything wrong and provide more information to test the PR.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11139.