ILIAS icon indicating copy to clipboard operation
ILIAS copied to clipboard

`Refinery`: Add more transformations (to improve `Refinery` usage in `ILIAS\Setup\Agent::getArrayToConfigTransformation`)

Open lscharmer opened this issue 1 year ago • 2 comments

This PR introduces new transformations for the refinery, mostly designed to be usable for the ILIAS Setup:

$refinery->to()->memberOf()

Enum validation: Checks if a value is in a specified set.

Example use case in the config would be: chatroom.log_level which must be one of emerg, alert, crit, error, warning, notice, info, debug or silly

$refinery->executable()

Checks if the given string specifies an executable path.

I could not find a suitable place for this transformation so I placed it on the top level of the refinery.

An example use case would be for config paths like: mediaobject.path_to_ffmpeg, style.path_to_scss, virusscanner.path_to_scan etc.

$refinery->int()->isBetween()

This is just a convenience transformation but IMO very nice to have. There is no actual class for this constraint as it can be composed from other transformations and constraints.

Example use case would be in the config would be: chatroom.deletion_interval.deletion_value which have both min and max values.

Other changes

DeriveTransformWithProblem

This trait is implemented to prevent boilerplate when implementing new transformations, as some methods are always implemented the same.

ProblemBuilder

No behavior is changed, just some simplifications (with one exception (pun intended): the InvalidArgumentExcpetion is now a ArgumentCountError)


Just FYI: Another PR will follow which will introduce deeper changes to the refinery and the relation between Transformations and Constraints but that PR will be independent of the changes made in this PR.

lscharmer avatar Sep 15 '23 14:09 lscharmer

Hi @lscharmer,

as with #6314, please assign back if back if this is ready for the next iteration.

Kind regards!

klees avatar Feb 06 '24 12:02 klees

Hi @klees,

this PR is updated an can be reviewed again. Below are my answers to your review:

  • [x] ProblemBuilder: You seem to be fixing some problem with the ProblemBuilder, but do not really understand what it is. Or is it just a refactoring? Please split that change into a single commit. If it is a problem: please add unit test, that fail without the changes.

Yes, it is just refactoring. It is now placed in it's own commit.

  • [x] DefaultToNull: I also came accross this problem several times. I'm not convinced that "all fields might be null" is the thing that really needs to be expressed. Instead we would want to say: this and that field could be null (and it is fine). Also, we would not only want to say it for the strict to variant, but also for the less kindlyTo variant. Hence I propose the following implementation: On toplevel, we add a transformation withDefault($transformation, $default) that replaces a null value with $default and passes non-null values to the $transformation. We could derive optional($transformation) from there. We could then introcude special cases for WithDefault in the recordOf-transformations for the to and kindlyTo variant. We could the write recordOf(["key" => $f->optional(...)]); or something similar for withDefault. Maybe we should discuss this, but I would at least suggest to split this topic from the PR at hand.

I will extract the DefaultToNull stuff into it's own PR (it is removed from this one). The Transformation was not ment to address the default / optional problem, but I see how this is relevant when one chooses a specific approach on how one wants to pursue to implement such a concept.

We can further discuss the details in that PR (coming soon).

  • [x] MemberOf: Although I would personally also prefer memberOf, I think we should name this transformation inArray. This is something most PHP developers will recognize immediately, while "member of" comes from a more mathy background imo.

Done

  • [x] Executable: Please rename to IsExecutable. We use this has/is naming scheme for constraints consistently, mostly.

Done

Best regards @lscharmer

lscharmer avatar Feb 08 '24 14:02 lscharmer

Hi @lscharmer,

thanks again and sorry that it took me so long to rereview this. I just merged this manually with two rather minor changes.

Kind regards!

klees avatar Apr 23 '24 14:04 klees

Thx @klees for your efforts

mjansenDatabay avatar Apr 23 '24 16:04 mjansenDatabay