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

Working with Lunar model override (ModelManifest) #89

Open
mtx-z opened this issue Feb 11, 2024 · 10 comments
Open

Working with Lunar model override (ModelManifest) #89

mtx-z opened this issue Feb 11, 2024 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@mtx-z
Copy link

mtx-z commented Feb 11, 2024

Hello,

I'm using the "Lunar way" to override some Lunar models (using the ModelManifest). I need to add some media collection and custom methods to models.

From the lunar-api package, I tried to play with the domains.php config file. But I always encounter Type issues (eg TypeError: Dystcz\LunarApi\Domain\Collections\Policies\CollectionPolicy::viewProducts(): Argument #2 ($collection) must be of type Dystcz\LunarApi\Domain\Collections\Models\Collection, App\Models\Lunar\Collection given, called i).

Of course, I tried to override more and more of the domains.php file to see if I could make something work but was not able to.
During my tests, I also noticed that editing domains.php config file made my morph foreign key invalid (on URLs or Medias). I needed to set my custom model path as foreign keys to have my data displayed in the hub. Something that should not be needed with lunar).

Commenting the ModelManifest code in my AppServiceProvider (so my Lunar models override), results in all the lunar-api routes working as expected.

It seems many lunar-api files are using the default Lunar models namespace. Would it be better if all were related to the domains.php config file? I could try to work something if it's the way to go.

@theimerj
Copy link
Contributor

Hey, you have to target the Lunar models, not Lunar API models when swapping for your own implementation.

It goes like this: LunarApi swaps Lunar models for LunarApi models and your App swaps Lunar models (hence overwriting the first swap) for you own models.

@mtx-z
Copy link
Author

mtx-z commented Feb 12, 2024

Hi @theimerj,

thank you for your answer.
I'm not sure I get it, here's what I've done :

$models = collect([
       \Lunar\Models\Product::class            => \App\Models\Lunar\Product::class,
        ...
]);
ModelManifest::register($models);

Then in domains.php

'products' => [
    ...
     'lunar_model' => \App\Models\Lunar\Product::class,
    ...
],

It seems to me that I'm swapping the Lunar Models to mine.

@theimerj
Copy link
Contributor

Hi @mtx-z, yes, this looks good. Do you have an example repo? Could you list steps to reproduce the issue?

It is still a bit vague to me right now, I would have to try it out myself in order to help you.

@mtx-z
Copy link
Author

mtx-z commented Feb 15, 2024

Hi @theimerj,

I created a test repository here: https://github.com/mtx-z/lunar-api-test
The last commit is about model swapping.

I test on the /products (and I include thumbnail,images,prices,cheapest_variant) route. I created (from the hub) a single product with the variant.
API works fine until I start to swap models.

  • When I swapped on domains.php + ModelManifest: I got a 401 Unauthenticated errors. Think a gate is failing.
  • When I swapped on domains.php only (or manifest only): I got Call to the undefined relationship [cheapestVariant] on the model [Lunar\\Models\\Product]. (added by lunar-api)

Another note: I had to create a default Tax Zone (with a tax rate) from the hub, in order to get the cheapestVariant included to work. I'm not sure that it's required by Lunar. Not doing it throws a null pointer exception (from SystemTaxDriver).

@theimerj
Copy link
Contributor

theimerj commented Feb 15, 2024

Hey @mtx-z, I will get to it over the weekend. But at first glance I can see, that you are extending Lunar models instead of Lunar API models. It makes sense that the functionality is missing then.

I will keep you posted after I get to it.

@mtx-z
Copy link
Author

mtx-z commented Feb 15, 2024

Ho, so I misunderstood you. I'll try to only swap lunar API ones (and extend them from my custom models). Thank you a lot!

@theimerj I just tested to extend lunar-api models: same 401 response (gate failing)

@theimerj theimerj self-assigned this Feb 17, 2024
@theimerj theimerj added the bug Something isn't working label Feb 17, 2024
@mtx-z
Copy link
Author

mtx-z commented Feb 23, 2024

Hi @theimerj;

please do not hesitate if I can help you on this.

Best regards.

@theimerj
Copy link
Contributor

Hey @mtx-z I created a PR (mtx-z/lunar-api-test#1) to your test repo and now the extension seems to work. The changes are self-explanatory I think.

Let me know if you have any other issues.

@theimerj
Copy link
Contributor

theimerj commented May 8, 2024

Hello @mtx-z, were you able to resolve the issues?

@mtx-z
Copy link
Author

mtx-z commented May 14, 2024

Hi @theimerj, I'm sorry for getting back to you so late.

I made a small test case and it works as expected. I need to make extended tests on my "real world" project, but I'm on something else atm.

This issue can be closed. I'll report back here if I encounter another related problem.

Thank you again.

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
Development

No branches or pull requests

2 participants