PR-staging

Success

User tests: Successful: Unsuccessful:

avatar csthomas
csthomas
15 Jul 2016

Pull Request for Issue #11103.

Summary of Changes

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?

Testing Instructions

[UPDATED]

  1. Download and Install Joomla 3.6.0 with sample data "test", "testing" or something like that

  2. Install com_patchtester.zip latest stable version

  3. On backend go to Menu -> All Front End Views -> Featured Articles

  4. In layout search for "Category Order" and change to "No Order". Article Order should be set to "Featured Article Order"

  5. Go to /index.php/featured-articles

  6. Back to backend and add a new article with title "Featured 1".

  7. Set it as featured. Category could be "Uncategorised"

  8. Go to /administrator/index.php?option=com_content&view=featured

  9. Change select box "Sort Table by:" to "Ordering ascending"

  10. Your new added article "Featured 1" should be at the bottom

  11. Go to /index.php/featured-articles and try to find your article "Featured 1", should at the end

  12. Go to /administrator/index.php?option=com_patchtester

  13. Fetch Data

  14. Search for "Fix the ordering of articles and featured"

  15. Apply Patch 11139

  16. Add next article with title "Featured 2", set as featured. Category could be "Uncategorised"

  17. Go to /administrator/index.php?option=com_content&view=featured

  18. Sort Table by should be "Ordering ascending"

  19. Now "Featured 2" is on the top and "Featured 1" is the last one.

  20. Go to /index.php/featured-articles and try to find your article "Featured 2", should at the top

  21. Go back to /administrator/index.php?option=com_content&view=featured and reorder "Featured 1" to the first place.

  22. Go to /index.php/featured-articles and "Featured 1" and "Featured 2" should at the top.

  23. You can do a few more detailed test if you like to.

Votes

# of Users Experiencing Issue
2/2
Average Importance Score
4.50

avatar csthomas csthomas - open - 15 Jul 2016
avatar csthomas csthomas - change - 15 Jul 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 15 Jul 2016
Labels Added: PR-staging
avatar killoltailored
killoltailored - comment - 15 Jul 2016

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.

avatar csthomas csthomas - change - 15 Jul 2016
The description was changed
avatar csthomas
csthomas - comment - 15 Jul 2016

@killoltailored I updated instruction. You have to use ex. Article Order in category/menu item/modules.

avatar csthomas
csthomas - comment - 15 Jul 2016

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)

avatar csthomas csthomas - change - 15 Jul 2016
The description was changed
avatar mbabker
mbabker - comment - 15 Jul 2016

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.

avatar csthomas
csthomas - comment - 15 Jul 2016

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.

avatar csthomas csthomas - change - 15 Jul 2016
The description was changed
avatar ggppdk
ggppdk - comment - 18 Jul 2016

Here is an alternative:
#11184

We can

  • fix performance of JTable::reorder
  • revert the B/C break in ordering
  • and continue to use ordering from 0 and higher
avatar ggppdk
ggppdk - comment - 18 Jul 2016

And forgot to say , should work in all database servers ? someone please confirm for sqlazure

avatar bertmert bertmert - test_item - 19 Jul 2016 - Tested unsuccessfully
avatar bertmert
bertmert - comment - 19 Jul 2016

Please see test #11139 (comment)
Made a mistake here.

I have tested this item 🔴 unsuccessfully on 9a95809

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.

Without patch:

Added new article aaaaa.
Gets ordering 12.
Other orderings unchanged.

With patch:

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.


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

avatar bertmert bertmert - test_item - 19 Jul 2016 - Tested successfully
avatar bertmert
bertmert - comment - 19 Jul 2016

I have tested this item successfully on 9a95809

@csthomas

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 ;-) .

avatar csthomas
csthomas - comment - 19 Jul 2016

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:

  • the best performance
  • not require reorder on each new row
  • break B/C for all tables except: banner, contact, newsfeed
  • new row always have to get ordering=max+1
  • require script to reorder almost all tables (which use ordering column) in user database before applied

2) Current PR is:

  • good for performance - second place,
  • not require reorder on each new row
  • almost B/C (we can not use that sort for banner, contact, newsfeed because they use max + 1 for new row)
  • tables with applied big ordering values always have to get ordering=min-1
  • worse looks for users - high ordering values

3) #11184

  • good performance - third place
  • require reorder on each new row
  • full B/C
  • new row may get ordering=0 or ordering=max+1

4) back to 3.5.1 solution but surround queries loop with transaction start and commit

  • performance should be better than on joomla 3.5.1 but worse from above
  • require reorder on each new row
  • full B/C
  • new row may get ordering=0 or ordering=max+1
avatar bertmert
bertmert - comment - 19 Jul 2016

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.

avatar csthomas
csthomas - comment - 19 Jul 2016

Yes this also can be an option but:

  • negative values are treated as reserved, row with ordering=-1 is not reordered, check joomla code
  • personally I can change my PR to use negative values but I'm worry that somebody may be against. I have to think about it more. Mean time I can wait for more comments.
avatar ggppdk
ggppdk - comment - 19 Jul 2016

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

  • 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

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

  • PHP loop that retrieves the records inside
$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

avatar csthomas
csthomas - comment - 19 Jul 2016

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)?

  • In short: it transfer every query to slave database. (not too much size)

How does replication of data work in mysql innodb?

  • In short: it transfer piece of changed data to slave database. (could be a lots of KB or MB)

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.

avatar ggppdk
ggppdk - comment - 19 Jul 2016

Arguably if you use MySQL max_packet_size of 4 Mbytes, instead of default 1 MB

  • you can update 200,000 records 1 query (i speak of the specific update query)

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

  • with so many records the PHP execution and PHP memory needed to handle so many records become very big (as it was before my PR)

even this is slow and needs a lot of memory because it will retrieve 174000 records

$rows = $this->_db->loadObjectList();
avatar ggppdk
ggppdk - comment - 19 Jul 2016

@csthomas

I am not against this PR i will test this PR,

And if this PR is accepted

  • then i will remake my PR to be able to update all needed records in single query in reorder method

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 !

avatar csthomas
csthomas - comment - 19 Jul 2016

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.
avatar ggppdk
ggppdk - comment - 19 Jul 2016

I think test and accept this PR first

  • then apply a new PR for avoiding multiple queries in table reorder

(because JTable reorder performance improvement involves a new query that should be indepedently accepted and make sure it is good for ALL database cases)

avatar csthomas
csthomas - comment - 19 Jul 2016

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?

avatar mbabker
mbabker - comment - 19 Jul 2016

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
.

avatar csthomas
csthomas - comment - 20 Jul 2016

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

avatar mbabker
mbabker - comment - 20 Jul 2016

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.

avatar csthomas
csthomas - comment - 20 Jul 2016

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);
    }
avatar joomla-cms-bot joomla-cms-bot - change - 20 Jul 2016
Category Administration Components Libraries
avatar csthomas
csthomas - comment - 20 Jul 2016

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.

avatar csthomas
csthomas - comment - 21 Jul 2016

Can I ask to test?

avatar csthomas
csthomas - comment - 25 Jul 2016

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.

avatar ggppdk
ggppdk - comment - 25 Jul 2016

i will test, but first please read and answer some questions

If this PR makes use of large ordering numbers, to avoid re-ordering

  • then maybe the alternative ordering in reverse is not needed ?

Also since the reorder() performance of JTable can be fixed, another reason not to introduce the "alternative" reverse re-ordering

  • 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)

Just i am a little confused
You are forcing reverse ordering on "articles" and on "featured articles" ,

  • 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 ?

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 ...)
avatar csthomas
csthomas - comment - 25 Jul 2016

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:

  • get MIN(ordering) per $where.
  • for big numbers I can use offset: 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.

avatar csthomas
csthomas - comment - 27 Jul 2016

@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.

avatar ggppdk
ggppdk - comment - 27 Jul 2016

yes i see it , so this is good for testing,

I will test tomorrow, thanks

avatar csthomas
csthomas - comment - 29 Jul 2016

@wilsonge Can you take a look on that PR #11139 and issue #11103 before you set final 3.6.1

avatar xupacabra
xupacabra - comment - 1 Aug 2016

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.

avatar ggppdk
ggppdk - comment - 1 Aug 2016

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 ?

avatar csthomas
csthomas - comment - 1 Aug 2016

@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.

avatar csthomas csthomas - edited - 1 Aug 2016
avatar csthomas csthomas - change - 1 Aug 2016
The description was changed
avatar csthomas csthomas - edited - 1 Aug 2016
avatar csthomas
csthomas - comment - 1 Aug 2016

I have added simpler and more detailed test instruction #11139 (comment)

avatar Sieger66 Sieger66 - test_item - 3 Aug 2016 - Tested successfully
avatar Sieger66
Sieger66 - comment - 3 Aug 2016

I have tested this item successfully on 1d2beb0

also testet with Hathor Administrator Template and Sticky Ordering(negative Ordering Numbers).


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

avatar ggppdk ggppdk - test_item - 3 Aug 2016 - Tested successfully
avatar ggppdk
ggppdk - comment - 3 Aug 2016

I have tested this item successfully on 1d2beb0

Works,

  • fixes B/C break (articles and featured articles get ordering number lower than existing = B/C fix)
  • does not** trigger re-orderings on new article creation
  • does not** trigger re-orderings on new article creation that are featured
  • no problems with drag and drop re-orderings

(**) 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


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

avatar Sieger66
Sieger66 - comment - 4 Aug 2016

(**) 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 )

avatar ggppdk
ggppdk - comment - 4 Aug 2016

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

  • at new article creation which before was re-ordering all items of the category or all featured items resulting in thousands of SQl update queries

and that is the purpose of using large ordering numbers

avatar csthomas
csthomas - comment - 4 Aug 2016

Now we have enough tests success.
What milestone/tag should I write in code:

 * @since  3.6.1

What is required to RTC?

avatar andrepereiradasilva
andrepereiradasilva - comment - 4 Aug 2016

@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

avatar csthomas
csthomas - comment - 5 Aug 2016

Thanks to all of you for your help.

Can I ask for RTC?

avatar rdeutz rdeutz - change - 7 Aug 2016
Status Pending Needs Review
avatar rdeutz
rdeutz - comment - 7 Aug 2016

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.


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

avatar csthomas
csthomas - comment - 7 Aug 2016

My PR has only one advantage against #11475 or #11134: it is B/C.

If there will be a solution for B/C of above PRs (ordering=(max+1) then it will be more natural to use.

avatar rdeutz
rdeutz - comment - 8 Aug 2016

@csthomas #11475 was never meant to be a fix for the B/C break

avatar csthomas
csthomas - comment - 8 Aug 2016

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:

  • new 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.

avatar rdeutz
rdeutz - comment - 9 Aug 2016

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.

avatar ggppdk
ggppdk - comment - 9 Aug 2016

@rdeutz

That button will decrease the re-configuration labour a lot,
for all sites that now use "ordering"

  • and you are right it should be in category manager, because if you add it to article manager and it works on current page, it will make a mess

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

  1. all ordering number will get reversed without user action,
  2. also the "order" parameters in all configuration places, become from "ordering" "reverse ordering"

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

avatar rdeutz
rdeutz - comment - 9 Aug 2016

ok, I will try to make a PR today

avatar rdeutz
rdeutz - comment - 9 Aug 2016

please check #11529, it is not a real fix for the B/C break but a way to go around it

avatar Sieger66
Sieger66 - comment - 21 Aug 2016

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 ??

avatar csthomas
csthomas - comment - 22 Aug 2016

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.

avatar csthomas
csthomas - comment - 22 Aug 2016

Off course we can still work on #11184 which is full B/C.

avatar rdeutz
rdeutz - comment - 22 Aug 2016

Because we reverted #8576 this can be closed, thanks to all for working on this issue

avatar rdeutz rdeutz - close - 22 Aug 2016
avatar rdeutz rdeutz - change - 22 Aug 2016
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2016-08-22 20:19:50
Closed_By rdeutz
avatar rdeutz rdeutz - close - 22 Aug 2016
avatar csthomas
csthomas - comment - 22 Aug 2016

After joomla reverted #8576 then this PR does not fix the bug but still can fix performance problem.
How? Because that PR does not need to use reorder method on adding a new article.

If there is no followers then it can be closed.

avatar ggppdk
ggppdk - comment - 22 Aug 2016

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

  • thus re-order (on new records) will be triggered 1 every 100 new records (e.g. articles)
  • it will also be made display-friendly for the cases that the ordering number is displayed, as the main objection for this PR was the use of high numbers ?

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

avatar csthomas
csthomas - comment - 8 Dec 2016

For everybody interested in improving performance of creating new articles I have prepared a next PR at #12839.

Add a Comment

Login with GitHub to post a comment