[#4] Natural sorting before limiting. #41

charger · 2017-03-21T20:37:43Z

No description provided.

kghbln · 2017-03-21T22:09:08Z

Thanks for the pull request. I have requested a review by @JeroenDeDauw

JeroenDeDauw

Thanks a lot! There are a number of small issues, and tests for the new behavior are missing. If you could address those, that'd be extra awesome. If not, I'll try to poke at them soonish.

JeroenDeDauw · 2017-03-22T13:00:57Z

src/Lister/SimpleSubPageFinder.php

@@ -201,6 +219,11 @@ private function getOptions() {

 		$options['LIMIT'] = (int)$this->options[self::OPT_LIMIT];

+		$sort_oder = $this->options[self::OPT_SORT_ORDER];
+		if ($sort_oder != self::NONE) {
+            $options['ORDER BY'] = "LENGTH(page_title) $sort_oder, page_title $sort_oder";


What is LENGTH(page_title) $sort_oder for?

Is there no way to escape these values? My knowledge of the MW DB stuff is a bit rusty... Having no explicit escaping here makes me uneasy.

This is naive "native sort". It is not so polished as PHP's strnatcmp but works fine in the most cases.

What does that have to do with sorting by the length of the page title? That is what this SQL does in my understanding.

Is there no way to escape these values? My knowledge of the MW DB stuff is a bit rusty... Having no explicit escaping here makes me uneasy.

'desc' or 'asc' is not a value, so escape is not applicable.
Only way to set this attribute- is setter setSortOrder which reject any values except asc or desc,

JeroenDeDauw · 2017-03-22T13:01:07Z

src/Lister/SimpleSubPageFinder.php

@@ -45,6 +49,7 @@ public function __construct( DBConnectionProvider $connectionProvider ) {
 			self::OPT_INCLUDE_REDIRECTS => false,
 			self::OPT_LIMIT => 500,
 			self::OPT_OFFSET => 0,
+            self::OPT_SORT_ORDER => 'asc',


Wrong indent

JeroenDeDauw · 2017-03-22T13:01:17Z

src/Lister/SimpleSubPageFinder.php

+     */
+	public function setSortOrder( $sortOrder ) {
+		if ( $sortOrder != self::DESCENDING && $sortOrder != self::ASCENDING && $sortOrder != self::NONE ) {
+            throw new InvalidArgumentException( 'Invalid $sortOrder specified' );


Inconsistent indent

JeroenDeDauw · 2017-03-22T13:01:54Z

src/Lister/SimpleSubPageFinder.php

+	const OPT_SORT_ORDER = 'sort';
+	const ASCENDING = 'asc';
+	const DESCENDING = 'desc';
+	const NONE = '';


SORT_ASCENDING

JeroenDeDauw · 2017-03-22T13:04:12Z

src/Lister/SubPageList.php

 		$this->subPageFinder->setLimit( $limit );
+		$this->subPageFinder->setSortOrder( $sortOrder );


This relies on the constant values matching the user input, which is bad practice

Inside setSortOrder I check that $sortOrder is 'asc' or 'desc'. I think this is secure enough.
What alternative should I use?

It's cool you put in that check - it will make errors more apparent.

The issue here is that there is no abstraction between presentation layer, application logic and data access layer. If that is not clear to you, nvm, I'll take care of it then.

JeroenDeDauw · 2017-03-22T13:05:14Z

tests/Unit/SubPageList/Lister/SubPageListTest.php

@@ -73,6 +73,7 @@ private function getRenderedList( SubPageList $subPageList, $titleText ) {
 				'showpage' => new ProcessedParam( 'showpage', true, false ),
 				'default' => new ProcessedParam( 'default', '', true ),
 				'limit' => new ProcessedParam( 'limit', 200, true ),
+				'sort' => new ProcessedParam( 'sort', 'asc', true ),


Not entirely sure why this was added. I don't see why the test would fail without it just by looking at this PR, but then perhaps I'm being blind.

What is definitely the case is that the new behavior is not tested

It caused to error "Undefined index: sort"
I think this is because of mocks.
I'm not so good at PHPUnit.

charger · 2017-03-22T16:22:06Z

I found a problem with my solution. For example we have subpages:

Releases/1.10
Releases/1.5
Releases/1.2
Releases/1.15
Releases/2.1

<SubPageList/> Without limits uses only PHP sorting and returns:

Releases/1.2
Releases/1.5
Releases/1.10
Releases/1.15
Releases/2.1

<SubPageList sort="desc" limit=3> Uses naive "native sorting" in SQL (length(title) asc, title asc), will returns:

Releases/1.2
Releases/1.5
Releases/2.1 <- here should be 1.10

<SubPageList sort="desc" limit=3> If use plain SQL sorting (title asc), it will get from DB:

Releases/1.10
Releases/1.15
Releases/1.2

and then sort it in PHP:

Releases/1.2
Releases/1.10 <- here should be 1.5
Releases/1.15

This illustrates problem: sorting and limiting must be or in SQL or in PHP, but not be spread. It's hard to implement real native sorting in SQL so

we should remove PHP sorting and use not native "ORDER BY"
Or we should remove LIMIT from SQL and implement it in PHP (this is infective but I doubt is exist WiKi with 100500 subpages)

What do you think, which point I should implement?

JeroenDeDauw · 2017-03-22T23:39:41Z

I had a look at the code and found that the situation is not really as simple as I thought. It's not the case that we select some pages from the db and then just sort them. What happens is that they get selected from the db, then manipulated into a tree like data structure which is traversed in TreeListRenderer and has each level individually sorted. From a code architecture level this is kinda weird: why would sorting happen in TreeListRenderer, which is definitely UI code, and not application logic code.

I'd be fine with selecting ALL subpages, if that was just the direct ones. However with indirect ones selected as well, I can imagine having a 100 subages that each have 100 themselves, and then you have a problem. So it seems that doing the sorting in SQL is the only approach that is going to work. I'm fine with removing the PHP sorting if SQL is used instead.

JeroenDeDauw · 2017-03-23T19:54:48Z

src/Lister/SimpleSubPageFinder.php

+     *
+     * @throws InvalidArgumentException
+     */
+	public function setSortOrder( $sortOrder ) {


Indent is still messed up. This project uses tabs, you seem to be using spaces

Yeah, my Idea IDE was not configured properly. Sorry.

[ProfessionalWiki#4] Natural sorting before limiting.

8a6209b

kghbln requested a review from JeroenDeDauw March 21, 2017 22:08

JeroenDeDauw reviewed Mar 22, 2017

View reviewed changes

[ProfessionalWiki#4] fixes according to code review ProfessionalWiki#1

5368c9f

[ProfessionalWiki#4] add tests, remove sorting in PHP

fd5d1ed

JeroenDeDauw reviewed Mar 23, 2017

View reviewed changes

[ProfessionalWiki#4] Fix Indent

d63361e

JeroenDeDauw merged commit ce71289 into ProfessionalWiki:master Mar 24, 2017

JeroenDeDauw mentioned this pull request Mar 24, 2017

Security and testing follow up to sorting change #43

Open

charger deleted the sql_nat_sort branch March 24, 2017 09:44

JeroenDeDauw mentioned this pull request Apr 13, 2017

limit done before sort #4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4] Natural sorting before limiting. #41

[#4] Natural sorting before limiting. #41

charger commented Mar 21, 2017

kghbln commented Mar 21, 2017

JeroenDeDauw left a comment

JeroenDeDauw Mar 22, 2017

JeroenDeDauw Mar 22, 2017

charger Mar 22, 2017

JeroenDeDauw Mar 22, 2017

charger Mar 23, 2017

JeroenDeDauw Mar 22, 2017

JeroenDeDauw Mar 22, 2017

JeroenDeDauw Mar 22, 2017

JeroenDeDauw Mar 22, 2017

charger Mar 22, 2017

JeroenDeDauw Mar 22, 2017

JeroenDeDauw Mar 22, 2017

charger Mar 22, 2017

charger commented Mar 22, 2017

JeroenDeDauw commented Mar 22, 2017 •

edited

Loading

JeroenDeDauw Mar 23, 2017

charger Mar 23, 2017

		$this->subPageFinder->setLimit( $limit );
		$this->subPageFinder->setSortOrder( $sortOrder );

[#4] Natural sorting before limiting. #41

[#4] Natural sorting before limiting. #41

Conversation

charger commented Mar 21, 2017

kghbln commented Mar 21, 2017

JeroenDeDauw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charger commented Mar 22, 2017

JeroenDeDauw commented Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeroenDeDauw commented Mar 22, 2017 •

edited

Loading