New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix the ordering of articles and featured #11139
Conversation
I have tested in Joomla 3.6.0 and I have follow below steps to test article ordering
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. |
@@ -1348,14 +1386,26 @@ public function reorder($where = '') | |||
throw new UnexpectedValueException(sprintf('%s does not support ordering.', get_class($this))); | |||
} | |||
|
|||
if (in_array($this->_tbl, array('#__content', '#__content_frontpage'), true)) | |||
{ | |||
// Use big numbers only for important tables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have different treating for some tables ? The purpose is to fix the B/C ordering break in all tables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because ordering number of content and frontpage on backend is not visible for users.
For test.
- Go to backend module list.
- Edit some module.
- For modules ordering you will see number, like 1,2,3,4 in select box of ordering field.
- Then replace:
if (in_array($this->_tbl, array('#__content', '#__content_frontpage'), true))
to ex:
if (1 || in_array($this->_tbl, array('#__content', '#__content_frontpage'), true))
- After that go back to module list an change sorting/edit some. After ordering get big numbers you can edit module again and check ordering field.
I think that people do not want to see big numbers:)
Off course we can set some offset before display to user, but is it not too complicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why we have subclasses. JTable is supposed to be a generic class for all tables. If you need table specific behaviors they need to be in those subclasses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can use subclasses JTableContent
, ContentTableFeatured
, and ..., but I have to duplicate code for that.
May be better is to create a new method JTable::reorder_alternative($where='')
and then only add to JTableContent class and others:
public function reorder($where = '')
{
return parent::reorder_alternative($where);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Team.
Thanks to all.
Thanks @csthomas for this: #11139 (comment)
Manually work fine and follow your detailed aswer too.
3.6.1 + Patch
Now the Joomla be fast and furious !
@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 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. |
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. If that patch should work without exceptions then above files (banner, contact, newsfeed) have to change way of adding a new item. |
Here is an alternative: We can
|
And forgot to say , should work in all database servers ? someone please confirm for sqlazure |
Please see test #11139 (comment) I have tested this item 🔴 unsuccessfully on 9a95809
1 category / 1088 articles. Without patch:Added new article aaaaa. With patch:Added new article bbbb.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11139. |
I have tested this item ✅ successfully on 9a95809 Sorry! Made a mistake while previous test : #11139 (comment) That's wrong there: Correct: But I still don't like these high ordering values ;-) . |
Review of solutions:
|
Why not ordering=min-1 for new entries? 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
also about my PR #11184,
and then the PHP loop that create the update SQL query, as i would like to avoid function calls e.g.
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 ! |
I'm fun of solution 1) from comment above #11139 (comment) Your solution Georgios is good, your PR does not have problems with big values, a) For content_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 Replication data for joomla is hard. Back to your comments:
Then we can not use that
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, ....
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. |
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
|
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). 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).
|
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. |
Unfortunately you can't cleanly. You either make a new method in JTable On Tuesday, July 19, 2016, Tomasz Narloch notifications@github.com wrote:
|
I want to apply one of two options:
public function reorder($where = '')
{
return parent::reorder_alternative($where);
}
public function reorder($where = '', $alternative=false) but this option has one drawback for subclasses that has defined reorder:
|
{ | ||
$ordering = $i + 1; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Joomla PHP coding standards allow using
$ordering = $extra_order ? ($last - $i - 1) : ($i + 1);
parenthesis are for readabilty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, but if I choose reorder_alternative($where);
then above won't be needed.
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);
} |
I have added simpler and more detailed test instruction #11139 (comment) |
I have tested this item ✅ successfully on 1d2beb0 This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11139. |
I have tested this item ✅ successfully on 1d2beb0
(**) 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, I did a code review too ! looks good (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. |
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. * @since 3.6.1 What is required to RTC? |
the 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? |
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. |
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:
If you decide to go a new way then some conversion ordering (for J3.5.1 and lower) should be added. |
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,
i see csthomas says:
which i agree it would have been nice,
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 |
please check #11529, it is not a real fix for the B/C break but a way to go around it |
i see csthomas says:
This is not correct because numbers in ordering column was also display in |
I admit this is a problem, it depends on 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. |
Off course we can still work on #11184 which is full B/C. |
Because we reverted #8576 this can be closed, thanks to all for working on this issue |
After joomla reverted #8576 then this PR does not fix the bug but still can fix performance problem. If there is no followers then it can be closed. |
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 |
For everybody interested in improving performance of creating new articles I have prepared a next PR at #12839. |
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]
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 endGo 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 topGo 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.