Add more flexibility to JTable ordering #12464

davidjosephhayes · 2016-10-18T20:30:24Z

Pull Request for Issue # .

Summary of Changes

Set JTable and JModelAdmin to use the JTable::_columnAlias property for ordering. Defaults to original behavior when there is no ordering property of JTable::_columnAlias.

Basically this implements what the function documentation for JTable::getColumnAlias says:
* Method to return the real name of a "special" column such as ordering, hits, published
* etc etc. In this way you are free to follow your db naming convention and use the
* built in Joomla functions.

Testing Instructions

Use any built in component and reorder items.

Documentation Changes Required

None.

…for ordering

brianteeman · 2016-10-19T08:38:44Z

libraries/joomla/table/table.php

 	{
 		// If there is no ordering field set an error and return false.
-		if (!property_exists($this, 'ordering'))
+		$orderingField = $this->getColumnAlias('ordering');


Surely this is not doing what the doc block above it says?

How would you change the doc block?

davidjosephhayes · 2016-10-19T20:33:45Z

I updated the code comments where it is throwing an exception instead of setting an error and returning false.

davidjosephhayes · 2016-10-24T15:29:30Z

Is there something else I should change?

davidjosephhayes · 2016-11-01T01:23:58Z

bump

andrepereiradasilva · 2016-11-01T01:33:39Z

needs two successfull tests to get merged in the core.

csthomas · 2016-11-22T15:54:25Z

IMHO you should not call quoteName($orderingField) more than once per one method. Create a variable and use it in other places.
You missed one https://github.com/joomla/joomla-cms/pull/12464/files#diff-8254e2c441a41d3fa26f0f83fbbb3043R1477

…le times; fix missing quoteField

davidjosephhayes · 2016-11-22T16:55:28Z

Both should be fixed now @csthomas

RonakParmar · 2016-11-30T09:02:26Z

I have tested this item ✅ successfully on fd08d7b

Tested ordering of articles before and after PR, in both case ordering working fine.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12464.}

csthomas · 2016-11-30T10:34:41Z

What about other files:

libraries/joomla/table/nested.php:1299:  if (property_exists($this, 'ordering'))
libraries/joomla/table/asset.php:168:    if (property_exists($this, 'ordering'))

I'm not sure but IMHO nested.php should be changed too.

davidjosephhayes · 2016-11-30T11:59:21Z

Never used those classes. I bet you are right.

I will run through the rest of the libraries and see if there are others.

mbabker · 2016-11-30T13:36:42Z

Only JTable and JTableNested need to have this abstraction layer. The other subclasses are "final" implementations for each table.

davidjosephhayes · 2016-11-30T21:17:12Z

I added the abstraction layer to JTableNested.

I feel like it should be on JTableAsset as well @mbabker since it is using a variable for the table name.

mbabker · 2016-11-30T21:22:51Z

A lot of the JTable subclasses reference the class variables in the methods versus hardcoding things like the table key(s) or name. JTableAsset is the class directly mapping to the #__assets table so it shouldn't need direct mapping like the other two classes have. Unfortunately, we neither declare the base classes (JTable or JTableNested) as abstract nor the "final" subclasses as final, so by that argument every column name supported by the abstraction layer should technically be mapped in the same way in all subclasses in case they are subclassed further by other developers. Personal opinion, I don't think it's needed for instances where it's supposed to be a final object, but I'll let someone else make the final judgement call on it.

davidjosephhayes · 2016-11-30T21:24:56Z

Gotcha. I end up extending core classes a lot, so its always nice when the abstraction is already there. However, abstracting everything is likely out of the scope of this PR.

csthomas · 2016-12-01T15:04:11Z

libraries/joomla/table/nested.php

 			// If the table has an ordering field, use that for ordering.
-			if (property_exists($this, 'ordering'))
+			$orderingField = $this->getColumnAlias('ordering');
+			if (!property_exists($this, $orderingField))


You have to remove exclamation mark.

davidjosephhayes · 2016-12-01T22:38:09Z

That was just terrible. What I get for rushing.

csthomas · 2016-12-02T08:22:15Z

I tested successful
and on review I have not found any ordering column in JTableNested subclasses.
It is not used by joomla at all but because of B/C it probably has to stay.
That OK.

Code style:
I thought that before if () code line there should be a comment or empty line.
Am I wrong?

davidjosephhayes · 2016-12-02T16:06:31Z

No idea. I don't see anything in here though however: http://joomla.github.io/coding-standards/?coding-standards/chapters/php.md

csthomas · 2016-12-02T17:44:36Z

I have tested this item ✅ successfully on 32a6436

Joomla works normal.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12464.}

davidjosephhayes · 2016-12-20T20:59:49Z

How does the HumanTestResults test work?

dgrammatiko · 2016-12-20T21:01:51Z

I have tested this item ✅ successfully on 32a6436

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12464.}

jeckodevelopment · 2016-12-20T21:54:58Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12464.}

zero-24 · 2016-12-20T23:14:13Z

How does the HumanTestResults test work?

@davidjosephhayes you can take a look here: https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results

It is a feature of ours custom issue tracker arround GitHub :)

zero-24 · 2016-12-21T20:56:23Z

With the last 4 commit i have fixed some CS Issue travis did not mention (why?) and updated this branch to the last staging.

zero-24 · 2016-12-21T20:57:18Z

@rdeutz @wilsonge please take a final review and merge decision here.

jeckodevelopment · 2016-12-22T19:34:18Z

@davidjosephhayes could you please look at the conflicts?

davidjosephhayes · 2016-12-22T21:56:06Z

Is it just me or is the conflict page not loading?

csthomas · 2016-12-22T21:58:16Z

For me it is not working too.

jeckodevelopment · 2016-12-22T22:44:14Z

@davidjosephhayes this is the conflicting file:
libraries/joomla/table/table.php

davidjosephhayes · 2016-12-26T17:28:24Z

How do I do know what to fix? When I click the Resolive Conflicts, nothing loads.

wilsonge · 2016-12-27T02:05:53Z

I should have fixed conflicts here. Can I have one tester to confirm this PR still works please

RonakParmar · 2016-12-27T04:00:38Z

I have tested this item ✅ successfully on fc626b1

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12464.}

Set JTable and JModelAdmin to use the _columnAlias propery of JTable …

460d86c

…for ordering

joomla-cms-bot added the PR-staging label Oct 18, 2016

fix concat spacing

839da09

brianteeman reviewed Oct 19, 2016

View reviewed changes

davidjosephhayes added 2 commits October 19, 2016 15:23

Merge remote-tracking branch 'upstream/staging' into staging

a42ca1d

update code comments

79add99

andrepereiradasilva mentioned this pull request Nov 17, 2016

[RFC] Another way to fast reorder table with single SQL query #12839

Closed

add quoted ordering field as var to prevent calling quoteField multip…

fd08d7b

…le times; fix missing quoteField

add jtable ordering abstraction layer to jtablenested

e77aa91

csthomas reviewed Dec 1, 2016

View reviewed changes

fix incorrect negation

32a6436

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 20, 2016

jeckodevelopment added this to the Joomla 3.7.0 milestone Dec 20, 2016

zero-24 added 4 commits December 21, 2016 21:52

Update nested.php

2d4e3e4

Update table.php

7f0dc80

Update admin.php

689a1aa

Merge branch 'staging' into staging

3b2dc6e

Merge branch 'staging' into staging

fc626b1

wilsonge removed the RTC This Pull Request is Ready To Commit label Dec 27, 2016

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 27, 2016

rdeutz merged commit 4eb5371 into joomla:staging Dec 27, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 27, 2016

Uh oh!

Add more flexibility to JTable ordering #12464

Add more flexibility to JTable ordering #12464

Uh oh!

Conversation

davidjosephhayes commented Oct 18, 2016 • edited by jeckodevelopment Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Documentation Changes Required

Uh oh!

brianteeman Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

davidjosephhayes Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

davidjosephhayes commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidjosephhayes commented Oct 24, 2016

Uh oh!

davidjosephhayes commented Nov 1, 2016

Uh oh!

andrepereiradasilva commented Nov 1, 2016

Uh oh!

csthomas commented Nov 22, 2016

Uh oh!

davidjosephhayes commented Nov 22, 2016

Uh oh!

RonakParmar commented Nov 30, 2016

Uh oh!

csthomas commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidjosephhayes commented Nov 30, 2016

Uh oh!

mbabker commented Nov 30, 2016

Uh oh!

davidjosephhayes commented Nov 30, 2016

Uh oh!

mbabker commented Nov 30, 2016

Uh oh!

davidjosephhayes commented Nov 30, 2016

Uh oh!

csthomas Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

davidjosephhayes commented Dec 1, 2016

Uh oh!

csthomas commented Dec 2, 2016

Uh oh!

davidjosephhayes commented Dec 2, 2016

Uh oh!

csthomas commented Dec 2, 2016

Uh oh!

davidjosephhayes commented Dec 20, 2016

Uh oh!

dgrammatiko commented Dec 20, 2016

Uh oh!

jeckodevelopment commented Dec 20, 2016

Uh oh!

zero-24 commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zero-24 commented Dec 21, 2016

Uh oh!

zero-24 commented Dec 21, 2016

Uh oh!

jeckodevelopment commented Dec 22, 2016

Uh oh!

davidjosephhayes commented Dec 22, 2016

Uh oh!

csthomas commented Dec 22, 2016

Uh oh!

jeckodevelopment commented Dec 22, 2016

Uh oh!

davidjosephhayes commented Dec 26, 2016

Uh oh!

wilsonge commented Dec 27, 2016

davidjosephhayes commented Oct 18, 2016 •

edited by jeckodevelopment

Loading

davidjosephhayes commented Oct 19, 2016 •

edited

Loading

csthomas commented Nov 30, 2016 •

edited

Loading

zero-24 commented Dec 20, 2016 •

edited

Loading