Skip to content

Conversation

@rdeutz
Copy link
Contributor

@rdeutz rdeutz commented Aug 13, 2016

PR for issue #11103

Summery of Changes

The PLT decided to revert #8576 because it is a B/C break

Testing instructions

  • Apply patch
  • You need a category view with articles
  • Set the the secondary ordering in Articles options to „Ordering“
  • Set the the secondary ordering for the menu to „Use Global“
  • Create a new Article

Expected result

The new Article should be listed first in the frontend category view

@alikon
Copy link
Contributor

alikon commented Aug 13, 2016

I have tested this item ✅ successfully on ca43296

despite the loss of perfomance ;)


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

@ggppdk
Copy link
Contributor

ggppdk commented Aug 13, 2016

The Joomla release that will have this revert,

  • should also include (if possible) a JTable::reorder performance fix

#11184

@alikon
Copy link
Contributor

alikon commented Aug 13, 2016

@ggppdk i'll look at #11184 more deep after the end of GSoC....
but reordering all artcicle after adding a new article is not performant by design

@Sieger66
Copy link
Contributor

I have tested this item ✅ successfully on ca43296

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


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

@zero-24
Copy link
Contributor

zero-24 commented Aug 13, 2016

RTC. Thanks


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 13, 2016
@brianteeman brianteeman added this to the Joomla 3.6.3 milestone Aug 13, 2016
@jeckodevelopment
Copy link
Member

I have tested this item ✅ successfully on ca43296


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

@wilsonge wilsonge merged commit 17bd7f2 into staging Aug 13, 2016
@wilsonge wilsonge deleted the revert-8576-performance-1 branch August 13, 2016 17:40
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 13, 2016
@VasyaV01
Copy link

I have tested successfully.

@tgv604
Copy link
Contributor

tgv604 commented Sep 6, 2016

Tested successfully;

For the sake of performance I'd suggest shifting the ordering field instead of reordering:
UPDATE #__content SET ordering = ordering + 1 [WHERE catid = N]

Basically, the same outcome in one query instead of many.

@ggppdk
Copy link
Contributor

ggppdk commented Sep 6, 2016

That can be an enhancement, have thought of it, but after executing

UPDATE #__content SET ordering = ordering + 1 [WHERE catid = N]

you still need to check for unique ordering, as existing orderings (for the given category) are not guaranteed to be unique

@tgv604
Copy link
Contributor

tgv604 commented Sep 7, 2016

Well, here's the proper renumbering then:

UPDATE #__content AS c,
    (SELECT @rownum := @rownum + 1 AS rownum, p.id
    FROM #__content AS p, (SELECT @rownum := 0) AS r
    [WHERE p.catid = N]
    ORDER BY p.ordering ASC) AS g 
SET c.ordering = g.rownum
WHERE c.id = g.id

@ggppdk
Copy link
Contributor

ggppdk commented Sep 7, 2016

Similar has been proposed already, it does not work on ALL DBs, (only MySQL ?)
and there was an argument not to use DB specific code

@ggppdk
Copy link
Contributor

ggppdk commented Sep 7, 2016

Here it is:
#8563

@tgv604
Copy link
Contributor

tgv604 commented Sep 7, 2016

Whatever, I'm implementing this for myself.

@ggppdk
Copy link
Contributor

ggppdk commented Sep 7, 2016

@tgv604

Personally,
I am in favour of accepting the MySQL specific optimizations !

I think both #8563 and #11184 should be accepted

@jeckodevelopment
Copy link
Member

@tgv604 and @ggppdk
this PR has been already merged, if you have further ideas about optimisation, you should open a new PR.

@mbabker
Copy link
Contributor

mbabker commented Sep 7, 2016

As long as Joomla claims to support multiple database engines, accepting patches which penalize users of a certain platform with slower performance should not be acceptable. That's why I argue against switch conditionals for different queries based on the engine. Some exceptions apply, and those are pretty much all already in the code base (like the driver specific subclasses for the Smart Search indexer).

roland-d pushed a commit to roland-d/joomla-cms that referenced this pull request Sep 11, 2016
@ronaldhoek
Copy link

Tested this fix with latest official release 3.6.2 and the order of newly created articles is correct again.
Don't see any perfomanc issues for myself.

@matrix630307
Copy link

Tested with 3.6.2 and work's fine.

@matrix630307
Copy link

i hope, this change will come in short time. it is very strange with our clients for this problem!

@wilsonge
Copy link
Contributor

wilsonge commented Oct 5, 2016

This has already been merged and as you know we have the 3.6.3 release candidate out now. So this will come out next Tuesday

@matrix630307
Copy link

thank you - yes, i have see the message from flow... happy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.