Feature/down the rabbit hole

[A-Daneel]

So, this should be the most basic implementation of fixtures. I am wholly aware that currently no pictures are uploaded with the activities.

However I would like your feedback on the rest. (And, if it were up to me. we merge this and do another one for the pictures. So that all the newly written tests can already use these fixtures and I can clean up the old ones)

[A-Daneel]

While I do agree that draft PR's are a lower priority, my reasoning what that it wasn't in a state that could ever be merged. I however really wanted some feedback for the route I took. In those cases I feel a draft with a requested review and a short explanation of the situation should most definitely not be ignored.

As for the merge conflict. I generally tend to resolve them when I'm the state of making the final PR.

[A-Daneel]

@jasperweyne

I feel just adding a comment won't accomplish the desired outcome. People might still use it. I've taken a very short look but would propose something akin to this example.

The issue is that phpStan does not appreciate it, however I think it would differentiate enough from our default setup. And everyone familiar with php code should know __set is used to when writing data to properties that should be inaccessible.

Not sure if it should be covered in the test code, since it's not exactly code anyone should every directly call.

Let me hear your thoughts and I'll implement the chosen solution asap.

    1     public function __set($name, $value): void                                                                      
    2     {                                                                                                                                                                                                        
    3         $this->$name = $value;                                                                                        
    4     } 

P.s. The conversation about when and how to request people looking at code feels more something for a public place. Not a PR that hopefully is merges and gone in 2/3 days. let's continue this on the discord.

[jasperweyne]

The problem with the __set magic method implementation is that this way, any field could be written to: if I'd write some code like...

$entity->foobar = 4;

this would add the foobar property to the entity, even if it's not present anywhere else in the code. In other words, the object is functionally turned into a dictionary. I feel that's worse compared to a developer writing to just a single invalid property.

Instead, how about adding a trigger_error with in the setId code? Like this:

    public function setId(string $id): self
    {
        trigger_error('The setId method is intended for use by Alice only, and should not be called manually');
        $this->id = $id;

        return $this;
    }

This way, a notice is triggered, actively informing a developer.

[A-Daneel]

As I said, I didn't look into it too deep. Your solution looks good, I'll implement it and try to to have the final PR ready tonight.

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication