Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistentie en gebrek aan duidelijk gedefinieerde variabelen in objecttypen van Entities #14

Open
EduardWitteveen opened this issue Jun 21, 2023 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@EduardWitteveen
Copy link

Beschrijf de bug
Het plugin-owc-gravityforms-zaaksysteem maakt momenteel gebruik van een implementatie waarbij de Zaak-objecten niet zijn gedefinieerd met duidelijk omschreven variabelen, maar in plaats daarvan gebruikmaken van een lijst van casts in het bestand src/Zaaksysteem/Entities/Zaak.php. Dit belemmert de mogelijkheid om de objectstructuur en gegevensuitwisseling op een consistente en voorspelbare manier te hanteren.
Naast het Zaak-objecten geldt dit ook voor andere objecten zoals Enkelvoudiginformatie-objecten (documenten)

Te reproduceren
Stappen om het gedrag te reproduceren:

  1. Navigeer naar de broncode van het Zaaksysteem.
  2. Zoek het bestand src/Zaaksysteem/Entities/Zaak.php.
  3. Observeer dat de Zaak-klasse gebruikmaakt van een lijst van casts in plaats van expliciete properties.

Verwacht gedrag
In plaats van de huidige implementatie met casts, zou het wenselijk zijn dat de Zaak-objecten worden gedefinieerd met expliciete membervariabelen (properties) die de relevante gegevens bevatten. Dit zou de consistentie en voorspelbaarheid van de objectstructuur verbeteren en zorgen voor een meer gestroomlijnde gegevensuitwisseling met de gebruikersinterface.

Schermafbeeldingen
Niet van toepassing.

Aanvullende context
In deze situatie zou werken met Data Transfer Objects (DTO's) een geschikte oplossing zijn. Door DTO's te gebruiken, kunnen de Zaak-objecten worden gedefinieerd met expliciete properties die de relevante informatie bevatten, waardoor de objectstructuur duidelijk wordt en de gegevensuitwisseling consistent is. DTO's bieden een gestructureerde en gecontroleerde manier om gegevens te verzamelen en over te dragen tussen verschillende lagen van een applicatie, waaronder de gebruikersinterface.

Het gebruik van DTO's zou de ontwikkelaars in staat stellen om het Zaaksysteem op een meer MVC-achtige manier te benaderen, waarbij de DTO's fungeren als de modellen die de gegevens en functionaliteit van het systeem vertegenwoordigen. Dit zou de samenwerking met andere gemeenten vergemakkelijken, aangezien de gegevensuitwisseling op een gestandaardiseerde en voorspelbare manier kan plaatsvinden.

Het aanpassen van de huidige implementatie om DTO's te gebruiken in plaats van casts zou de flexibiliteit, onderhoudbaarheid en bruikbaarheid van het Zaaksysteem binnen het component verbeteren. Het zou ook de mogelijkheid bieden om een duidelijk gedefinieerd gegevensmodel te hebben, wat belangrijk is voor de interoperabiliteit tussen verschillende systemen en de consistentie van de gebruikersinterface.

@EduardWitteveen EduardWitteveen added the bug Something isn't working label Jun 21, 2023
@sanderdekroon
Copy link
Member

sanderdekroon commented Jul 19, 2023

Hieronder een voorstel hoe dit probleem aan te pakken. Hierbij heb ik rekening gehouden met de mogelijkheid om de waarden uit de API te casten naar complexe typen en resources te lazy loaden.

Casten
Data uit de API zijn enkel strings en integers (of arrays met strings en integers). Deze waarden moeten nog omgezet worden naar complexere typen daar waar nodig. Denk bijvoorbeeld aan een datum die omgezet wordt naar een DateTimeImmutable of een Enum zoals bij vertrouwelijkheidaanduiding.

$apiData = [
    'url' => 'https://...', 
    'datum' => '2023-07-19', 
    'vertrouwelijkheidaanduiding' => 'openbaar'
];

$zaak = Zaak::fromState($apiData);

// Intern wordt de 'state' omgezet naar meer complexe data typen
var_dump($zaak);

// object(Zaak)[6761]
//   protected string 'url' => string 'https://....'
//   protected DateTimeInterface 'datum' => 
//     object(DateTimeImmutable)[6762]
//       public 'date' => string '2023-07-19 09:50:50.056553'
//       public 'timezone_type' => int 3
//       public 'timezone' => string 'UTC'
//   protected Attributes\Confidentiality 'vertrouwelijkheidaanduiding' => 
//     object(Confidentiality)[6763]
//       protected string 'name' => string 'confidentiality level'
//       protected string 'value' => string 'openbaar'

Lazy loaden
Niet alle resources hoeven direct ingeladen te worden. Het Zaaktype wat bij een Zaak hoort hoeft pas ingeladen te worden wanneer een property op een Zaaktype wordt benaderd. Door middel van een Proxy kunnen we dit gedrag implementeren.

Voorbeeld
Hieronder een (niet volledig uitgewerkte) voorbeeld implementatie die dit mogelijk maakt. De fromState method zorgt er voor dat we 'ruwe' API data kunnen gebruiken en tegelijk gedefinieerde properties hebben en type safety kunnen hanteren.

<?php

namespace OWC\Zaaksysteem\Entities;

/**
 * Base Entity class that defines:
 * - Getters and setters
 * - Casting behaviour
 * - fromState() method
 */
abstract class Entity
{
    protected array $casts = [];

    public function __set(string $name, mixed $value)
    {
        if (! property_exists($this, $name)) {
            // Do not allow dynamic property creation
            throw new \ErrorException(sprintf('Property %s is not defined on %s', $name, __CLASS__));
        }

        return $this->setValue($name, $value);
    }

    public function __get(string $name)
    {
        return $this->getValue($name);
    }

    public static function fromState(array $state)
    {
        $entity = new static();

        foreach ($state as $name => $value) {
            $entity->setValue($name, $value);
        }

        return $entity;
    }

    public function setValue(string $name, $value)
    {
        if ($this->hasCast($name)) {
            $caster = $this->getCaster($name);

            $value = $caster->cast($this, $name, $value);
        }

        return $this->setAttributeValue($name, $value);
    }

    public function setAttributeValue(string $name, mixed $value)
    {
        $this->{$name} = $value;

        return $this;
    }
}

class Zaak extends Entity
{
    protected array $casts = [
        'zaaktype'      => ZaaktypeProxyCaster::class,
        'startdatum'    => Casts\NullableDate::class,
        'einddatum'     => Casts\NullableDate::class,
    ];

    // Constructor property promotion (PHP >= 8)
    public function __construct(
        protected string $url,
        protected string $clientName,
        protected ZaaktypeInterface $zaaktype,

        // Non-required properties
        protected ?DateTimeImmutable $startdatum = null,
        protected ?DateTimeImmutable $einddatum = null,

        // ...more properties
    ) { }

    public static function fromState(array $state)
    {
        $entity = new static($state['url'], $state['clientName']);

        foreach ($state as $name => $value) {
            $entity->setValue($name, $value);
        }

        return $entity;
    }
}

class NullableDate {
    public function cast(Entity $model, string $key, $value)
    {
        if (is_null($value)) {
            return $value;
        }

        if (is_string($value)) {
            $value = new \DateTimeImmutable($value);
        }

        if (! is_object($value) || ! $value instanceof \DateTimeInterface) {
            throw new InvalidArgumentException("Invalid date given");
        }

        return $value;
    }
}

Daarnaast kunnen we complexe resources, die als URL terug worden gegeven in de API, lazy loaden door proxies te implementeren zoals ZaaktypeProxy.

<?php

interface ZaaktypeInterface 
{
    public function __construct(
        protected string $url,
        protected string $clientName,
        protected Confidentiality $vertrouwelijkheidaanduiding = null,
        protected ?DateTimeImmutable $doorlooptijd = null,
        protected ?DateInterval $verleningstermijn = null,
        // ...more properties
    );
}

class Zaaktype extends Entity implements ZaaktypeInterface {}

class ZaaktypeProxy extends Zaaktype implements ZaaktypeInterface {
    protected bool $loaded = false;

    public function __get(string $name)
    {
        if (! $this->loaded) {
            // Probably move this to some sort of Trait.
            
            // Load the full resource when a property is accessed.
            $uuid = substr($this->url, strrpos($this->url, '/') + 1);
            $entity = resolve($this->clientName)->zaaktypen()->get($uuid);

            // Inspect the loaded entity and find all promoted properties.
            $ref = new \ReflectionClass($entity);
            foreach ($ref->getProperties() as $property) {
                if (! $property->isPromoted()) {
                    continue;
                }

                // Assign the values to the proxy, so they are accessible by this instance.
                $this->setValue(
                    $property->getName(),
                    $entity->getValue($property->getName());
                );
            }

            $this->loaded = true;
        }

        return parent::__get($name);
    }
}

class ZaaktypeProxyCaster
{
    public function cast(Entity $model, string $key, $value)
    {
        // If the supplied data is a string, it's most likely an URL.
        if (is_string($value)) {
            // Maybe do a filter_var($value, FILTER_VALIDATE_URL) check
            $value = new ZaaktypeProxy($value);
        }

        if (is_object($value) && $value instanceof ZaaktypeInterface) {
            return $value;
        }

        throw new InvalidArgumentException("Invalid Zaaktype given");
    }
}

Ik hoor graag wat @EduardWitteveen, @robertbossaert en eventuele anderen van dit voorstel vinden.

@sanderdekroon sanderdekroon self-assigned this Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants