Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
sitepark-schaeper
left a comment
sitepark-schaeper
left a comment
There was a problem hiding this comment.
First of all - I had to finish this up since it's getting late and github unfortunatly does not save pending comments. So some methods might not have gotten the attention they deserve, but with the rather architectual concerns I address here I think it's not worth investing too much time in these smaller chunks before those are settled.
With that out of the way, I wrote some (unstructured) thoughts down that I had while reading through your code and would like to discuss them with you:
I've ignored the translations so far, but is the .translation-cach directory intended to be checked in?
Why are Layouts Elements?
The names of email specific classes are a bit wordy in my opinon. For example, Dto/Email/EmailHtmlMessageRendererResult could just be Dto/Email/RenderingResult, right?
A few classes here include the term Model, which I don't quite understand what it means in this context. May it just be a Dto?
There are a lot of mutable properties in the Dtos which I did not find why. Do we really need to expose these (and if so, can we properly differentiate them)?
We might have gone a little bit overboard with the array typing here. It's really hard to review it and figure out what is a class, what is just an array structure and which of their properties are actually gurantieed to be typed as defined and which might not. As such I did "skip" some of the implementations (like FormDataModelFactory) because I did not have the time to comprehend them. I feel like this complexity and mental overhead is not neccessary and we could just type out most of these as classes.
This also applies to all the $options (for which it is also difficult to figure out what exactly is beeing configured and from where). I get that it's hard to declare these but having it as just array<string, mixed> but checking for explicit values seems like a red flag.
Another reoccuring theme seem to be nullable array types. I don't think there is a reason for an array that is allowed to be empty to also be nullable. an alternative would be something like ?non-empty-array<string, string> but that is even less intuitive.
Then there are classes with comments saying "not yet implemented". I get that creating them helps development but do they need to be checked in?
Lastly a couple of class comments could be nice. For example the Constraint interface might profit from a small sentence explaining what it is, why it exists and what it's used for. To be clear - I don't want to be pedantic and don't think it's too big of a deal, I just think someone that does not happen to have the context could have a hard time figuring this out where all it takes are 20ish words.
.github/workflows/verify.yml
Outdated
| with: | ||
| phpVersions: '["8.2","8.3"]' |
There was a problem hiding this comment.
why do we verify with 8.2 and 8.3 but release with 8.4?
There was a problem hiding this comment.
One dependency is not yet PHP 8.4 ready. I'll change this when it's solved, see: phpro/api-problem-bundle#27
| * } $options | ||
| * @throws Html2TextException | ||
| * @throws TransportExceptionInterface | ||
| * @throws Exception |
There was a problem hiding this comment.
what Exception? under what circumstances?
There was a problem hiding this comment.
League\Csv\Exception
https://csv.thephpleague.com/9.0/connections/#exceptions
| private function identifyType(Control $control, array $schema): string | ||
| { | ||
|
|
||
| $type = $schema['type'] ?? ''; | ||
| $format = $schema['format'] ?? ''; | ||
|
|
||
| if ($type === 'string' && $format === 'data-url') { | ||
| return 'file'; | ||
| } | ||
|
|
||
| if ($type === 'string' && $format === 'html') { | ||
| return 'html'; | ||
| } | ||
|
|
||
| if ($type === 'boolean') { | ||
| return 'checkbox'; | ||
| } | ||
|
|
||
| if ($type === 'array' && isset($schema['items']['oneOf'])) { | ||
| return 'checkbox-group'; | ||
| } | ||
|
|
||
| if ( | ||
| $type === 'string' | ||
| && isset($schema['oneOf']) | ||
| && ($control->options['format'] ?? '') === 'radio') { | ||
| return 'radio-buttons'; | ||
| } | ||
|
|
||
| if ($type === 'string' && isset($schema['oneOf']) | ||
| ) { | ||
| return 'select'; | ||
| } | ||
|
|
||
| return 'text'; | ||
| } |
There was a problem hiding this comment.
I don't know the values $schema can contain, but this implementation would return 'text' for basically anything since there is no validation.
There was a problem hiding this comment.
Yes, text is the fallback.
UISchema and Json schema are used to identify which UI component is to be used.
See https://sitepark.github.io/atoolo-docs/develop/form/controls/#json-schema
Without fallback, it would mean that new components can only be generated once the FEDS module has been updated. With fallback, it makes the system more stable during an update process.
src/Service/FormReader.php
Outdated
| if ($control->scope === null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
What does it mean for a Control not to have a $scope? Why are we not processing these cases? Or should this just straight up not be possible?
There was a problem hiding this comment.
Yes, scope cannot be null.
| function ($data, $schema) use ($constraint) { | ||
| return $constraint->check($data, $schema); | ||
| }, |
There was a problem hiding this comment.
this function does not need to bind $this, it should probably be static.
There was a problem hiding this comment.
Here I don't understand what you mean. Where is $this bind.
| if (!is_string($value)) { | ||
| throw new CustomError('Value is not a string'); | ||
| } |
There was a problem hiding this comment.
shouldn't this be something that is cought by the JsonSchemaValidator? I mean we don't also check weather or not a field expecting a number actually is one right? so why do we need to check if a data-url is a string? or could this constraint technically be configured to any field? if so, is that not validated either? does this have to err at runtime?
There was a problem hiding this comment.
It could be that the JsonSchemaValidator already checks this beforehand, since we have defined String as the type.
But since the method is passed a mixed, I have to check that anyway, so that phpstan is satisfied.
| /** | ||
| * @param array<mixed>|object $array | ||
| * @return array<string,mixed>|array<mixed> $array | ||
| */ | ||
| public function objectToArrayRecursive(mixed $array): array | ||
| { | ||
| if (is_array($array)) { | ||
| foreach ($array as $key => $value) { | ||
| if (is_array($value)) { | ||
| $array[$key] = $this->objectToArrayRecursive($value); | ||
| } | ||
| if ($value instanceof stdClass) { | ||
| $array[$key] = $this->objectToArrayRecursive((array) $value); | ||
| } | ||
| } | ||
| } | ||
| if ($array instanceof stdClass) { | ||
| return $this->objectToArrayRecursive((array) $array); | ||
| } | ||
| /** @var array<string,mixed>|array<mixed> $array */ | ||
| return $array; | ||
| } | ||
|
|
||
| /** | ||
| * @param array<mixed>|array<string,mixed> $array | ||
| * @throws \JsonException | ||
| */ | ||
| public function arrayToObjectRecursive(array $array): object | ||
| { | ||
| $json = json_encode($array, JSON_THROW_ON_ERROR); | ||
| return (object) json_decode($json, false, 512, JSON_THROW_ON_ERROR); | ||
| } |
There was a problem hiding this comment.
These don't seem like Platform reliant functions; It looks a bit like this is beeing (ab)used as a Util class.
Are these functions really that commonly required? if so - why?
There was a problem hiding this comment.
The methods are only used once. I have pulled them out so that I can test them individually.
I don't want an Util class. What could I call them?
https://jsonforms.io/docs/uischema/layouts#elements Layouts can be nested. Layouts have an |
2 participants
See