Data type LanguageTag with parsing

[lscharmer]

This PR introduces a new data type LanguageTag. The language tag is validated by the RFC as it is defined here: https://www.ietf.org/rfc/bcp/bcp47.txt. This is the continuation of PR #4653.

This will be used in: https://github.com/ILIAS-eLearning/ILIAS/pull/4082

[lscharmer]

On the 27.09.2022 we (@nhaagen, @kergomard and I) discussed where the parser should be placed and how it should be accessed. We decided that the Parser itself should be a private API (there will be no way to access e.g. the Brick class from the $DIC). Publicly there will be a transformation for the refinery (e.g. $DIC->refinery()->parseTo()->languageTag()) and a creation method for the Data Factory (e.g. $DIC->data()->languageTag('de')) added for each data type that uses the parser.

The code for the refinery will be located under src/Refinery/Parser. And under src/Refinery/Parser/Definitions new Definitions for different data types can be added.

All Documentation for the parser will be moved from doc blocks into README files.

@klees @mjansenDatabay If you approve of this approach I will implement these changes.

Best regards @lscharmer

[mjansenDatabay]

@lscharmer / @klees

For me the suggested changes/workshop results look/sound good. I like the new parseTo group of the Refinery and the suggested folder structure. From the consumer perspective it is good to hide the Brick as we learned the Parser` topic is hard and not many developers will (unfortunately) use it.

[Amstutz]

What is the status here? Note that there seems to be an other PR with a dep. to this one: https://github.com/ILIAS-eLearning/ILIAS/pull/4082

[mjansenDatabay]

@Amstutz The status is: There was another workshop after the presentation of @lscharmer at the ILIAS development conference. In this workshop, @lscharmer , @nhaagen and @kergomard spoke about how to finally integrate this into the Refinery and Data components.

As the coordinator of the Refinery I am fine with the workshop results and decisions.

The question is: Does @klees have any objections, especially for the src/Data part?

[klees]

@mjansenDatabay I'm on it...

[mjansenDatabay]

Any updates on this :)? A feedback is appreciated, so @lscharmer can continue to complete this until the end of the year.

[lscharmer]

Hi @klees, I highly appreciate your feedback. Your suggestions and change requests are great improvements that I will gladly implement. The following 2 points are the only ones that need imo further discussion:

• [ ] Factory: I would suggest to expose the creation of a LanguageTag on the factory of Data similar to the one for URI. Consumers could then access them in the context of the Refinery as $refinery->to()->data("languageTag").

I agree that the LanguageTag should be accessible from the Data Factory but I cannot make the LanguageTag available to the Data Factory (with constructor DI) because there is a chicken and egg problem: The Refinery needs the Data Factory which needs the ABNF definition for the LanguageTag which in turn needs the Refinery. The only way I can accomplish this is by accessing the global DIC or implement something like ILIAS\Data\Factory::setRefinery(...), which are both not optimal.

• [ ] Transform: I would suggest to move the methods from ABNF\Transform to Brick. It seems as if these are just other building blocks for the parser and we will mostly require Brick and Transform besides each other anyway.

Both Brick and Transform are solely there to form a structure. The classes are there to make it easier what the purpose of these methods is. Imo the purpose of these methods is different enough to have them split into multiple classes. I also think it is nice to call $transform->to(...) instead of $brick->to(...) or $brick->transformTo(...) which reflects this.

I would rather argue that even Brick could be split up into more classes (I placed appropriate comments as way to structure them softly: here and here which reminds me of those comments which should've been method names).

I would instead propose to use Transform in Brick to have something like: $brick->transform()->to(...).

Best regards.

[lscharmer]

Hi @klees, I saw that there is one question that I need to answer:

• [ ] to and either: If I'd use Transform::to on the result build by either: How would I find out if the left or right option was taken?

With Transform::as(...) you can give them a key which you can use to check for existence:

$left = $transform->as('left', $brick->sequence(['foo']));
$right = $transform->as('right', $brick->sequence(['bar']));

$transformation = $refinery->custom()->transformation(static function (array $parts) {
    if (isset($parts['left'])) {
        // Do something with left.
   } elseif (isset($parts['right'])) {
        // Do something with right.
    }
});

$transform->to($transformation, $brick->either([$left, $right]));

Or, because either is basically an if else you can do something like:

// No need to give them a key:
$left = $brick->sequence(['foo']);
$right = $brick->sequence(['bar']);

$leftTransformation = $transform->to($refinery->custom()->transformation(static function (string $left) {
    // Do something with left.
}), $left);

$rightTransformation = $transform->to($refinery->custom()->transformation(static function (string $right) {
    // Do something with right.
}), $right);

$transformation = $refinery->custom()->transformation(static function (array $parts) {
    // No need to check for left or right here.
});

$transform->to($transformation, $brick->either([$left, $right]));

Best regards.

[lscharmer]

Hi @klees,

I implemented the requested changes.

With the change of:

• [ ] as: I think the creation of the parser would look a lot simpler if we'd baked that array building via as into sequence. Currently, the naming of array keys and the actual usage are far apart. E.g.: To understand, which array keys are available here, I would need to scroll up there. If I could write $brick->sequence(["language" => $language, ...]) this would be a lot easier to understand. Please consider incorporating asintosequence`.

and the discussion of $brick->transform()->to(...) I decided to add a new class Primitives where the old implementation lives and the Brick::either and Brick::sequence use Primitives and Transform to achieve the feature described above. Because Transform would with this change have only one public method for the API, I changed the brick so it serves as the sole interface for the API which uses internally Primitives and Transform. This keeps the public API clean and keeps the structure of the code.

I didn't implement the following change:

• [ ] Transform::to: I think that building block is doing more than announced. It seems to apply the transformation and then wrap the result in an array.

Because the Transform::to method doesn't do more than announced, It just uses the same mechanism as Transform::as to transform its value. The method MUST wrap it's value into an array because Intermediate::push accepts an array.

Best regards.

[klees]

Really awesome work, have merged: https://github.com/ILIAS-eLearning/ILIAS/commit/af60dbb2c2e2811b21611d1c7c67e620789dba9e

Thanks a lot!

[mjansenDatabay]

Thx as well @klees for your ideas and review efforts.

[klees]

Definitely worth it =)