QUAL: (breaking) Stop using deprecated $statut, $fk_statut

[mdeweerd]

QUAL: (breaking) Stop using deprecated $statut, $fk_statut

This change is a breaking change because 'statut' is replaced with 'status' in the API replies.

Remove/replace usages of $statut and $fk_status properties. Update the DolDeprecationHandler configuration (extra definition in classes that introduced fk_statut).

(Assignments to the deprecated identifiers are no longer needed as the DeprecatonHandler will access the new attribute when the deprecated name is accessed.

This change is a breaking change because 'statut' is replaced with 'status' in the API replies.

@eldy Do you want to maintain 'statut' in the replies alongside 'status' or make this a hard change with the APIs.

[d3v2a]

You can write a rector rule, so module developers also have the change in their code. And you don't have to search in the code where property is used, as rector does it for you.

[mdeweerd]

You can write a rector rule, so module developers also have the change in their code. And you don't have to search in the code where property is used, as rector does it for you.

Thank you for the feedback.

1. Sure, rector could be reused by other developers;
2. Rector has its own particularties - making it work can also be tedious;
3. Changing code with rector will hide certain issues, not resolve duplicate lines, etc. There were a few cases where the original line had to be removed, not replaced (for instance 'unset($obj->note)' had to be removed to not unset note_private. Using rector would make these case more prone to misses. Not all classes are using the fields the same way (stripe uses statut or status as a string).
4. I have my tools too ;-). I use the filtered phan error report to quickly go to the relevant lines. I have used phan in the past for some updates.

But feel free to propose a rector script - there are other deprecated items to update.

[eldy]

As this represent a high risk of regression, it will be postponed, we are close of starting beta and we must restore stability on already merged deprecated task or property change. First step is to duplicate old name and new name everywhere there is an assignement before releasing a version, so external module and developer can use the new name and then old name could be removed in a next step.

[mdeweerd]

First step is to duplicate old name and new name everywhere there is an assignement

The purpose of the DeprecationHandler is to avoid precisely those double assignments - several are missed currently.

[mdeweerd]

This property may be used by external code. We can remove an assignement that may break compatbility only 2 versions after we announce that property must be replaced in externzl code. And to announce this, we must first have all properties double assigned everywhere in the code so externzl development still work and user know he has 2 version to upfzte its code. So it is for the moment too early to remove such lines in method, when the assignement can be used in externzl code

You marked it as duplicate but maybe it's not clear what the DolDeprecationHandler does. When a property $A is the deprecated name for $B, then this is in the 'deprecatedProperties()' response as 'A' => 'B'.

Then whenever 'A' is written, it is written to 'B' ('A' does not exist for subclasses or other classes). When we read 'A', in reality 'B' is read, etc.

The only thing that is 'annoying' is the side effect of the 'private' variable for '$A' which was requeseted - it requires that nothing in the class itself references '$this->A'. The other annoying thing in the Dolibarr setup is that most of the time the property declarations are duplicated (which is unneeded when they are defined in the subclasses). So one needs to make sure that 'A' does not exist in any of the subclasses as a declaration anymore.

So third party classes could have this too (define 'public $statut' and 'public $status') but third party modules are compatible for a range of dolibarr versions so in that exception they need to amend their code to ensure that works for the required range by adapting their local use of $this->statut.

While adapting the code for $statut and a few related properties, I noted that sometimes the deprecated value was set alone, sometimes the new alone and in several cases the old or the new was accessed in the dolibarr code. With the handler and only accessing one of the two properties (or even both) both old and new property names have the same values.

[eldy]

Due to to high number of conflict i prefer to close this PR. Switching to "status" everywhere is a critical step and i think it is better to include more smaller step to reach this goal instead of this PR.