openchurch v2 #32
openchurch v2 #32
Conversation
There was a problem hiding this comment.
IMHO only the security.yaml (firewall rule) is blocking
I get the domain driven development hype, but
- it makes code analysis and review sincerily painful
- induces code duplication
- is reaaaaally verbose, without bringing much more clarity
I don't get the excessive use of interfaces, they don't bring anything to the table IMHO. Some classes and methods only serve as direct aliases to Doctrine EntityManager or querybuilders methods calls, which are a bad pattern.
Some behaviors (such as repositories cloning) might become dangerous and come and bite our fingers someday.
I appreciate the extensive tests!
| # Note: Only the *first* access control that matches will be used | ||
| access_control: | ||
| # - { path: ^/admin, roles: ROLE_ADMIN } | ||
| # - { path: ^/, roles: ROLE_USER } |
There was a problem hiding this comment.
(not blocking) No access control rules are defined: what happens if someone tries to call a CRUD endpoint without API key? Shouldn't we define PUT/POST endpoints as rules in here?
| @@ -0,0 +1,767 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
(not blocking) Shouldn't we remove this script?
| 'progress' => key_exists('currentBatch', $churchData) ? ceil(($churchData['currentBatch'] / $churchData['batchCount']) * 100) : 'undefined', | ||
| 'startDate' => key_exists('startDate', $churchData) ? (new \DateTime($churchData['startDate']))->format('Y-m-d H:i:s') : 'undefined', | ||
| 'endDate' => key_exists('endDate', $churchData) ? (new \DateTime($churchData['endDate']))->format('Y-m-d H:i:s') : 'undefined', | ||
| ], |
There was a problem hiding this comment.
(not blocking) Too much code duplication, please refactor
| private readonly PlaceRepositoryInterface $placeRepository, | ||
| private readonly FieldRepositoryInterface $fieldRepo, | ||
| private ValidatorInterface $validator, | ||
| private Security $security, |
There was a problem hiding this comment.
(not blocking) Please set all injected services as readonly when possible
| throw new ValidationException($violations); | ||
| } | ||
|
|
||
| $field->applyValue(); // Dynamycally set the value to the correct property (intVal, stringVal, ...) |
There was a problem hiding this comment.
(not blocking) Dynamycally
| foreach ($entity->fields as $field) { | ||
| call_user_func($detachCallback, $field); | ||
| } | ||
| call_user_func($detachCallback, $entity); |
There was a problem hiding this comment.
(not blocking) These call_user_func to call EntityManager::detach are far from elegant, and induce complexity: it hides this service dependency to the EntityManager
Just inject EntityManagerInterface in this service and call its detach function
| if ($e instanceof ValidationException) { | ||
| return $e->getMessage(); | ||
| } | ||
| if ($e instanceof ProblemExceptionInterface) { |
There was a problem hiding this comment.
(not blocking) This class does not extend Exception: is this dead code?
| call_user_func($detachCallback, $entity); | ||
|
|
||
| if ($e instanceof ValidationException) { | ||
| return $e->getMessage(); |
There was a problem hiding this comment.
(not blocking) This if branch isn't useful, since it's the same as the default (last return of the function)
|
|
||
| if (null !== $paginator = $models->paginator()) { | ||
| $resources = new Paginator( | ||
| new \ArrayIterator($resources), |
There was a problem hiding this comment.
(not blocking) Please always use imports instead of \Class usages (please fix across project)
| "minimum-stability": "stable", | ||
| "prefer-stable": true, | ||
| "require": { | ||
| "php": ">=8.2", |
No description provided.