MannikJ / laravel-sti

Add single table inheritance for laravel eloquent models
MIT License
5 stars 1 forks source link

STI Model ::find method fails in model but succeeds in controller #1

Closed louischristner closed 3 years ago

louischristner commented 4 years ago

Description:

I have Car and Bike classes, each extended from Vehicle class. The Vehicle class uses the SingleTableInheritance trait. The Vehicle migration has a $table->sti()->nullable() which create a type field. The Vehicle model has a protected $table = 'vehicles'; field. In my database i have a Car object stored in the Vehicle table like this:

id: "id_string"
type: "App\Models\Car"

In a controller when I do Vehicle::find("id_string") it succeeds and returns me an instance of Car. But in a model Vehicle::find("id_string") returns me null when Car::find("id_string") returns me an instance of Car.

MannikJ commented 4 years ago

Can you provide some more information/code? Maybe your model is not booted already or something like this.

By the way the title says STI::find. What do you mean with that? Athough the package ships a facade it is still empty at the moment. So find does not even exist.

louischristner commented 4 years ago

I tried to keep it as simple as possible but it is a bit long. This is a description of the process that lead me to the problem.

My project is an API that allows you to create Vehicle objects and links them to VehicleShop objects.

Create a Car

With one POST request I create a new Car (in CarController):

public function store(Request $request)
{
    $new_car = Car::create();

    return response()->json([
        'type' => 'Car',
        'vehicle_url' => route('vehicle.show', ['vehicle' => $new_car->id])
    ]);
}

It returns me:

{
    "type":"Car",
    "vehicle_url":"http://localhost:8000/api/vehicle/id_string"
}

Then I do a GET request on the vehicle_url (in VehicleController):

public function show(String $vehicle_id)
{
    $vehicle = Vehicle::findOrFail($vehicle_id);

    return response()->json([
        'type' => get_class($vehicle),
        'vehicle_id' => $vehicle->id
    ]);
}

It returns me:

{
    "type":"Car",
    "vehicle_id":"id_string"
}

So here `Vehicle::find("id_string") returns me an instance of Car.

Create a VehicleShop

Then I create an object from the VehicleShop class which has a vehicles Many to Many relationship with Vehicle objects using a POST request (quite similar to the previous Car POST request).

Link the Car to the VehicleShop

Then I do a PUT request on the previously created VehicleShop. Here is the request:

{
    "list":[
        "id_string"
    ]
}

And here is the VehicleShopController update function:

public function update(Request $request, $vehicle_shop_id)
{
    $vehicle_shop = VehicleShop::findOrFail($vehicle_shop_id);

    if ($request->has('list') && is_array($request->list)) {
        $return_value = $vehicle_shop->updateVehicleShop($request->list); // this line call a VehicleShop model public function
    } else {
        return response('Bad Request', 400);
    }

    if ($return_value === true) {
        return response('No content', 204);
    } else {
        return response('Bad Request that shouldn't be triggered in this case', 400);
    }
}

And here is the VehicleShop model updateVehicleShop function:

public function updateVehicleShop($list)
{
    foreach ($list as $element) { // in this case $list has just one element
        $vehicle = Vehicle::find($element); // in this case $element = "id_string"

        if ($vehicle == null) { // triggered condition
            return false;
        } else { // condition that should be triggered
            $this->vehicles()->save($vehicle);
        }
    }
    return true;
}

And it returns me a Bad Request that shouldn't be triggered in this case because the Vehicle::find($element) returns null instead of an instance of Car.

But if I replace the Vehicle::find($element) by Car::find($element) it works.

louischristner commented 4 years ago

I created this project to give you an example : https://github.com/louischristner/ocap_sti.

The problem appears when doing a Model ::find(id) (where Model is extended from a model with the STI trait) in a model extended from the same parent as Model.

If you follow the "How to use" you will get a 400 Bad Request on the last part. The status code come from the FacetController update function and is the result of a null returned after a Facet::find in the ReadWriteFolderFacet writeTarget function.

If you replace in ReadWriteFolderFacet the Facet::find (line 55) by a ReadWriteDocumentFacet::find and repeat the last part of "How to use" by keeping the exact same values it will work (proof that the object exists).

I hope it will help you.

MannikJ commented 4 years ago

Well I don't really know where to start. There is so much funky stuff going on in this example app that it is hard to grasp the problem. Most likely the root of all evil comes from the fact that you overwrite the constructor in your Facet model. This way you destroy many of the mechanisms the package actually provides, because parent::__constructor does not call the trait's constructor, but only the constructor defined in the real parent class. This is how traits work. So it is apparently not the best practice to ship constructors with traits and maybe I will find a better solution for that in the future.

And also there are a lot of other potential flaws and bugs that will likely lead to problems. If it is going to be a real app I would dive a bit deeper into request validation, authorization and phpunit testing. The latter is actually very important. It would be a lot easier if you would setup your project with a configured testing environment and some tests that show the issues rather then a huge step by step guide.

I also configured your phpunit to use an sqlite in-memory database so one can just share the project and run the tests without the need to configure a project environment with database etc. first.

louischristner commented 4 years ago

Don't worry this was just a quickly made example so I skipped testing and request validation (shame on me).

I will try to replace the constructor overwriting by something else.

And thanks for all your work !

MannikJ commented 4 years ago

No problem. Your luck I had a free sunday :wink:

I already added the missing part to your constructor, so this should work so far. Can't say there are no other problems though...

louischristner commented 4 years ago

Indeed, it was the parent construct call in the Facet construct that was problematic. Thanks for your help and time !

kephas commented 4 years ago

Shouldn't that issue remain open, while the trait still uses __constructor()?

MannikJ commented 4 years ago

If you have a good idea how to avoid the constructor inside the trait, tell me and I will see if I can change it. But at the moment in my use cases the current implementation works fine.

MannikJ commented 3 years ago

The trait now doesn't come with a contructor overwrite anymore. Instead it uses laravel's quite recent trait initializing mechanism which is exactly designed for cases like this, where a trait needs to do some initializing work on the model instance and not from static context like boot mechanism does.

See: https://github.com/MannikJ/laravel-sti/commit/69a80cca100536c21dcd5a9afd9d8aebe453cc1a