Fix container resolution and morph map for extended models

[theimerj]

This PR aims to fix #1930 as proposed here: #1932

Problem:
The problem appears to be double serialization of certain casted attributes, specifically $shippingBreakdown and $taxBreakdown, because at first the model is Cart, but when it goes through pipelines, it is resolved as CustomCart and this namespace corrupts attribute serialization, so when it tries to set eg. $order->shipping_breakdown, it tries to set a json encoded value instead of the ShippingBreakdown class.

Specifically this part of ShippingBreakdown / TaxBreakdown gets called when it should not.

public function serialize($model, $key, $value, $attributes)
{
    return json_encode(
        $this->set($model, $key, $value, $attributes)
    );
}

Possible solution:
Resolve correct models in pipelines:

use Lunar\Models\Contracts\Order as OrderContract;
use Lunar\Models\Contracts\OrderLine as OrderLineContract;

public function handle(OrderContract $cart, Closure $next): mixed
{
    // ...
    
    // Instead of new OrderLine
    $orderLine = App::make(OrderLineContract::class);

    // ...
}

• Sample implementation
• Tests

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lunar-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 7, 2024 5:52pm

[theimerj]

This change edb019f was the missing piece to get green in our projects.

[theimerj]

I know this PR is huge, but here is the gist.

I basically went through the whole codebase and did just a few things:

1. Replace concrete model implementations in method arguments (Lunar\Models\Product → Lunar\Models\Contracts\Product) as well as method returns types
2. Instead of directly calling static properties on a concrete implementation, it is now prefixed with ::modelClass() (Lunar\Models\Order::find() → Lunar\Models\Order::modelClass()->find()
3. Introduce new test scenario where all models are swapped (if LUNAR_TESTING_REPLACE_MODELS env var is true, it will bind TestModelManifest singleton which registers stub models), Github test workflow is updated as well to reflect this change
4. There is a new phpunit.extending.xml which can be used for local testing as follows ./vendor/bin/pest -c phpunit.extending.xml --testsuite core --bail
5. Test "can get new draft order when cart changes" was skipped, because it seems to be a false positive

I also tested that everything works in https://github.com/dystcz/lunar-api, where I extend almost every Lunar model and it works great.

[alecritson]

@theimerj Looks like there was some overlap with a PR, are you able to take a look at the conflicts?

[theimerj]

@theimerj Looks like there was some overlap with a PR, are you able to take a look at the conflicts?

Hey @alecritson, I will get to it soon. Thank you @wychoong for pointing these things out.

[alecritson]

Thanks for this PR, I've had a little look and we've discussed it internally briefly, one point we raised was the introduction of modelClass() when using eloquent such as:

Language::modelClass()::getDefault();

I think this feels a bit off and not something we'd really want throughout the codebase as it doesn't really feel like a natural use of Eloquent and is a bit of a maintenance overhead we'd rather not introduce.

Was the main purpose of this so that you are always interacting with the extended model? Since I don't think the panel code should be concerned with this and using the Lunar models should just work as well. There might have been an issue with resolving the correct model by Lunar but hopefully this should be resolved in #1949 as there was a bug with this.

Additionally I think that same PR should resolve the issue with the shipping breakdown hydration 🤔

[theimerj]

1. Ok, I understand your concern, but Lunar's responsibility is to ensure that it always resolves the correct concrete implementation. To me it basically seems wrong to use concrete implementations of Lunar models throughout the codebase when you want to offer the ability to replace them. And the way to go around this problem is basically trying to find the Lunar model in ancestors and go from there. What if the replaced model does not extend Lunar model and just implements the contract? Unlikely, but there is no check that the concrete implementation has to extend a Lunar model. I just went with the ::modelClass(), because it was short. There are other ways to resolve the concrete class out of the container.

2. I already checked Fix extended model resolving #1949 and am not sure that it will work with more than one level of inheritance which may happen. I am not saying that this is the best practice, but I am sure you get my point.

3. At the same time this part seems to me like an introduction of possible performance issues if this kind of rehydration happens too many times.
   

   lunar/packages/core/src/Base/Traits/HasModelExtending.php

   Lines 25 to 30 in 047bfb2

   	$newModel = new $concreteClass;
   	$newModel->id = $this->id;
   	foreach (array_keys($this->getAttributes()) as $key) {
   	$newModel->setAttribute($key, $this->{$key});
   	}

   
   Plus do we know that this works with all types of relationships? Eg. "through" relationships and so on? Shouldn't this use the ::replicate() method provided by laravel, just modified to replicate into a different class to make it more robust? Is this even necessary?

The solution introduced in this PR seems tedious at first, which it is (I went through the process), but if you comply with the correct way of resolving implementations out of the container in the future as well, the problems introduced are basically mitigated for good.

Lastly I find the introduction of tests which cover the functionality with extended models in mind is quite important in order to globally catch possible bugs which may arise in the future.

---

Bottom line: I tested the #1949 briefly locally and it really seems to have resolved the discovered issues. However, the test coverage is not ideal and I personally believe that the whole codebase should rely on automatic dependency injection more (at the very least in all the method calls where you can type hint the interfaces instead of concrete implementations). This alone mitigates a lot of problems.

Let me know how to proceed. We can scratch this PR all together. We can keep some of the changes and scrach the rest you don't like. Your call. Please let me know and I will act accordingly. Thank you.

[alecritson]

1. Ok, I understand your concern, but Lunar's responsibility is to ensure that it always resolves the correct concrete implementation. To me it basically seems wrong to use concrete implementations of Lunar models throughout the codebase when you want to offer the ability to replace them. And the way to go around this problem is basically trying to find the Lunar model in ancestors and go from there. What if the replaced model does not extend Lunar model and just implements the contract? Unlikely, but there is no check that the concrete implementation has to extend a Lunar model. I just went with the ::modelClass(), because it was short. There are other ways to resolve the concrete class out of the container.

I think you're correct in the sense that Lunar should be able to resolve the concrete class, but the underlying core code does not need to resolve to those classes, Lunar should should still be able to use it's own models.

The point of model extending is to allow developers to bring their own model classes, which extend the Lunar ones, to add functionality on top of Lunar. But also for Lunar itself to not be concerned about that and still use the base models under the hood. For example if the developer adds their own model method, the Lunar core is never going to try and call that method so it doesn't really need to know about it.

I think if we wanted to go down the path of Lunar using the concrete class then really we should use dependency injection.

2. I already checked Fix extended model resolving #1949 and am not sure that it will work with more than one level of inheritance which may happen. I am not saying that this is the best practice, but I am sure you get my point.

We are currently not concerned about more than one level of inheritance and would consider it unsupported.

3. At the same time this part seems to me like an introduction of possible performance issues if this kind of rehydration happens too many times.
   

   lunar/packages/core/src/Base/Traits/HasModelExtending.php

   Lines 25 to 30 in 047bfb2

   	$newModel = new $concreteClass;
   	$newModel->id = $this->id;
   	foreach (array_keys($this->getAttributes()) as $key) {
   	$newModel->setAttribute($key, $this->{$key});
   	}

   Plus do we know that this works with all types of relationships? Eg. "through" relationships and so on? Shouldn't this use the ::replicate() method provided by laravel, just modified to replicate into a different class to make it more robust? Is this even necessary?

Yeah I understand what you're saying here and I updated the PR to use the replicate method, thanks for the heads up about that. I don't foresee any performance issues.

The solution introduced in this PR seems tedious at first, which it is (I went through the process), but if you comply with the correct way of resolving implementations out of the container in the future as well, the problems introduced are basically mitigated for good.

Lastly I find the introduction of tests which cover the functionality with extended models in mind is quite important in order to globally catch possible bugs which may arise in the future.

Bottom line: I tested the #1949 briefly locally and it really seems to have resolved the discovered issues. However, the test coverage is not ideal and I personally believe that the whole codebase should rely on automatic dependency injection more (at the very least in all the method calls where you can type hint the interfaces instead of concrete implementations). This alone mitigates a lot of problems.

Let me know how to proceed. We can scratch this PR all together. We can keep some of the changes and scrach the rest you don't like. Your call. Please let me know and I will act accordingly. Thank you.

In short I think the best things to take away from the PR are:

• Using DI where possible and type methods to their contracts
• Using extended models in the test suite seems like good coverage.

The only thing we wouldn't include is the ::modelClass() call.

[theimerj]

Hello @alecritson, thank you for your reply.

I think you're correct in the sense that Lunar should be able to resolve the concrete class, but the underlying core code does not need to resolve to those classes, Lunar should should still be able to use it's own models.

The point of model extending is to allow developers to bring their own model classes, which extend the Lunar ones, to add functionality on top of Lunar. But also for Lunar itself to not be concerned about that and still use the base models under the hood. For example if the developer adds their own model method, the Lunar core is never going to try and call that method so it doesn't really need to know about it.

I think if we wanted to go down the path of Lunar using the concrete class then really we should use dependency injection.

Understood. Your approach actually made me refactor Lunar API models, because creating deeply nested inheritance is not a way to go.

We are currently not concerned about more than one level of inheritance and would consider it unsupported.

Ok, if only one level of inheritance will be supported by Lunar it should probably be written in the docs and a check could be introduced in the model manifest. I can prepare something.

Yeah I understand what you're saying here and I updated the PR to use the replicate method, thanks for the heads up about that. I don't foresee any performance issues.

Great to hear. That should minimize the performance issue I guess.

In short I think the best things to take away from the PR are:

• Using DI where possible and type methods to their contracts
• Using extended models in the test suite seems like good coverage.

The only thing we wouldn't include is the ::modelClass() call.

Ok, I will merge conflicts and rework the PR removing ::modlClass().

Thanks again. Have a great day.