implement sort on field like documentation describe it

[rycks]

module builder make that documentation for class fields:

.../...
* 'type' field format:
* 'integer', 'integer:ObjectClass:PathToClass[:AddCreateButtonOrNot[:Filter[:Sortfield]]]',
* 'select' (list of values are in 'options'),
* 'sellist:TableName:LabelFieldName[:KeyFieldName[:KeyFieldParent[:Filter[:Sortfield]]]]',
.../...

so there is a sortfield for sellist, that PR implements that sort configuration

[lvessiller-opendsi]

It's a lack in v18 but I'm afraid of breaking some modules :
If you check the documentation of "myobject.class.php" on line 85 :
Note: Filter must be a Dolibarr Universal Filter syntax string. Example: "(t.ref:like:'SO-%') or (t.date_creation:<:'20160101') or (t.status:!=:0) or (t.nature:is:NULL)"
So since v18, you can use Universal Filter Syntax (UFS) with ":" as separated parameters of Filter.
So the variable "$InfoFieldList[5]" is not always the "Sortfield" parameter.
If we want to add this "Sortfield" paramater, we must change the way to manage the UFS on v18 and maybe backport from a more recent version of Dolibarr

[eldy]

When you look at v21 version we see that in doc comment:

				// 0 : tableName
				// 1 : label field name
				// 2 : key fields name (if different of rowid)
				// optional parameters...
				// 3 : key field parent (for dependent lists). How this is used ?
				// 4 : where clause filter on column or table extrafield, syntax field='value' or extra.field=value. Or use USF on a second line separated by "\n".
				// 5 : string category type. This replace the filter.
				// 6 : ids categories list separated by comma for category root. This replace the filter.
				// 7 : sort field

May be fixes were done before in v19 or v20, may be in several steps

Nevermind, by reading the the doc in v21, i understand that:

• the trouble was addressed into a higher version than v18.
• the doc was updated to have the sort field at position 7 (some code was using at position 5, other at position 7, and that in some cases the field 5 and 6 were already used, even in v18 (for example when $type == 'chkbxlst') for filter on a category, so it confirms that doc was wrong and a fix of doc+code was necessary.
• the pb to allow to add more parameters like the sort field after the USF filter without conflict due to the : char has been addressed by the use of the caracter "new line" when we want to use an USF syntax, otherwise the old syntax is used. It seems the old syntax is still possible in v21 but with a sort field at position 7 like it was already implemented for the type=chkblst in v18.

Note: on old released version when doc does not match code, the code should rules prior to doc to reduce risk of regression. So i think this problem should remains as "won't be fixed in v18" as the fix seems to have needed a large amount of changes on higher version. Or at least we can just update the doc instead to say that sort is not yet supported in v18, and is in higher version

[rycks]

Hmmmm please consider the fix

old code one line

$sql .= ' ORDER BY ' . implode(', ', $fields_label);

new code replace that line it with 4 lines ...

					if (isset($InfoFieldList[5])) {
						$sql .= " ORDER BY ".$this->db->escape($InfoFieldList[5]);
					} else {
						$sql .= " ORDER BY ".$this->db->sanitize(implode(', ', $fields_label));
					}

we are in $filter_categorie === false condition block so what could be the risk ? that PR is 4 lines, ok the code around is ... complex ? hard to read and to unsderstand ... maybe but that fix is really basic, if there is a filter field configured just use it else use old sort on label field ...

[lvessiller-opendsi]

Hmmmm please consider the fix

old code one line

$sql .= ' ORDER BY ' . implode(', ', $fields_label);

new code replace that line it with 4 lines ...

					if (isset($InfoFieldList[5])) {
						$sql .= " ORDER BY ".$this->db->escape($InfoFieldList[5]);
					} else {
						$sql .= " ORDER BY ".$this->db->sanitize(implode(', ', $fields_label));
					}

we are in $filter_categorie === false condition block so what could be the risk ? that PR is 4 lines, ok the code around is ... complex ? hard to read and to unsderstand ... maybe but that fix is really basic, if there is a filter field configured just use it else use old sort on label field ...

The "$InfoFieldList[5]" can be the USF filter and not the SortByFiled as wanted because you explode by ":"
Maybe to be sure it's used for "SortByField", check with a regex for "$InfoFieldList[5]" and it must not contain ":" characters and only "alias.table_name ASC|DESC, ..".
Otherwise you break the SQL request with a filter in "ORDER BY".

[rycks]

hum, i'm sorry, that's not clear for me, USF is for dolibarr AFTER 18 no ?

[lvessiller-opendsi]

hum, i'm sorry, that's not clear for me, USF is for dolibarr AFTER 18 no ?

Yes, you're right the USF was only on documentation of module builder (in "myobject.class.php") and not used in v18 of Dolibarr.
It begins to be introduced in v19 of Dolibarr with "forgeSQLFromUniversalSearchCriteria" in common object.
So I'm ok for this FIX only on v18.

[eldy]

The message "we are ... so what could be the risk ? that PR is 4 line..."
is clearly the king of message that a maintenance merger should never say. Even a change of 1 line of code has very frequently side effect. More often that we can see/think.

This change means we introduce a feature that we can also call an "enhancement fix" (here the ability to sort) that is not yet available in v18 (this is why we call this an "enhancement fix" compared to a "maintenance fix" that fix a bug that was working or that announced to end users as working in the ChangeLog). A feature that has never worked correctly or that was never introduced into a stable version X must not be introduced into a "maintenance fix" of X.

There is always things we can't see ! A lot of bad developers will say "i'am sorry, there is nothing we can see, so there is no risk !". It is not because we can't see any trouble that there is no trouble. We never see the trouble ! ... until it is too late when a user or a developer complain about a side effect discover few weeks or month after.
And it is just because we can't see what can be the risk, that we must NOT introduce behaviour change into an already released version.
This is the common best practive rule. In the past we probably made exception but if we make an exception we must have a very serious reason (like a legal need for example, a security vulnerability)

this should be enough to refuse. If it is not enough, imagine we intoduce this change, it means some people will use it (if we introduce it, it is to be used !), for example by an external module or an end user. And then, after upgrade to v19, or v20 or more, it will be broken because the feature (or if you prefer the "enhancement fix"), was introduced into a higher version differently (the sort is at position 7 instead of 5 in higher versions). What can we say to user ?

"Sorry, we introduced a behaviour change when best practive say to not do it, and we did even tough we know that it will generate regression later ?"
Not looks good.
If we can avoid this to have a better ascending compatibility we must avoid.

A behaviour that was never implemented correctly into a past already released version must not be introduced into this past version, above all when we know it will be a regression after an upgrade, the number of line of codes does not matter in such situation, the end user don't care if it was 1 or 100 lines. It is always difficult to say "no", but we must sometimes accept to say "sorry, it is not ready in vX.0.* or not working well yet, will be ready fully available in a higher version".

Note: such changes, with behaviour change, could be accepted into a version, this is what we call a "minor version, for example X.1.0 or X.2.0" (this is the goal of the central number). We currently do not do that but if we do it one day and if the enhancement fix was already done in a higher version, it must be done by a similar backport.

To help to decide if a PR can/should be into a maintenance branch or into the develop branch, i have documentated this decision graph:

[rycks]

@eldy ok but i think we don't have to put 'Discussion' label then ... but "wont't fix" or something like that (may @lvessiller-opendsi and me use labels ?) and close that PR no ?

[lvessiller-opendsi]

@eldy
Maybe it will be interesting to have this graphic in Dolibarr Wiki.
I wonder if the section "Submit a patch, work and help on Dolibarr development" of "https://wiki.dolibarr.org/index.php?title=Developer_FAQ" is the good place for it ?
I'm can't do it myself because I'm not the author of this graphic.

[eldy]

@eldy
Maybe it will be interesting to have this graphic in Dolibarr Wiki.
I wonder if the section "Submit a patch, work and help on Dolibarr development" of "https://wiki.dolibarr.org/index.php?title=Developer_FAQ" is the good place for it ?
I'm can't do it myself because I'm not the author of this graphic.

Graph has been added into this page:
https://wiki.dolibarr.org/index.php?title=Category:RoadMap#Merging_PR_process

[hregis]

@eldy @lvessiller-opendsi @rycks

and for the "serious need" ? ;-) Who decides apart from you? ?
What to do if there are no other solutions? If it becomes blocking for many users? and the answer is no ?

[hregis]

@eldy @rycks @lvessiller-opendsi
what do we tell customers? wait in two versions?
We need intermediate versions! we can no longer have shaky versions that we cannot correct properly...
you have to become aware of this?!