laravel-shift / blueprint

A code generation tool for Laravel developers.
MIT License
2.82k stars 270 forks source link

belongsToMany Foreign UUID not working. #659

Closed makzumi closed 6 months ago

makzumi commented 8 months ago

Issue:

The pivot table on a belongsToMany is not respecting UUID from the related models. Should be foreignUuid

image

draft.yaml:

models:
  Brand:
    uuid
    name: string index
    slug: string unique index
    logo: string nullable
    softDeletes
    relationships:
      belongsToMany: Product
  Product:
    uuid
    name: string index
    slug: unique index
    price: double:8,2
    softDeletes
    relationships:
      belongsToMany: Brand
jasonmccreary commented 8 months ago

Could you pull from master and see if it work. A PR was recently merged that may address this...

makzumi commented 8 months ago

Thanks Jason, will give it a try and thanks for Blueprint!

makzumi commented 8 months ago

sorry, nope, it is still not working, just tried master. :(

jasonmccreary commented 8 months ago

Ok. @rcrosbourne is this something your PR might have fixed?

makzumi commented 8 months ago

I have found the issue (i think...) there's this method in the MigrationGenerator class that passes the type to the buildForeignKey method. BUT the type is hard coded, trying to figure out how to correct it:

image

jasonmccreary commented 8 months ago

You definitely aren't pulling from master as that code has changed. So it'd be good to ensure you have set your composer constraint to dev-master and run composer update.

Try again and see if it generates the "correct" code. Also, if not, disable use_constraints and see if it does (without the constraints of course).

makzumi commented 8 months ago

I switched to dev-master again, but I'm still seeing no uuid on the pivot tables, I still think it has to do with that hardcoded "id" in that method.

image

image

makzumi commented 8 months ago

UPDATE: It worked with the use_constraints set to false: image

makzumi commented 8 months ago

I guess I can use it like this, thanks so much @jasonmccreary

jasonmccreary commented 8 months ago

The other path should be supported.

@rcrosbourne, seems your PR missed the path for setting foreign UUID/ULID columns with constraints (i.e. use_constraints enabled). I'd appreciate if you could fix that path.

rcrosbourne commented 8 months ago

Yea I will take a look at it.

rcrosbourne commented 8 months ago

This PR addresses the issue.

funder7 commented 6 months ago

Any news about this?

jasonmccreary commented 6 months ago

Nope.

funder7 commented 6 months ago

No plan to merge the #661? It seems ready. It's not a big problem to edit migrations manually, but if it's fixed...

jasonmccreary commented 6 months ago

As noted on the PR, it has some changes requested and like 46 commits which it difficult to review/trust. I plan to give more time to Blueprint after the release of Laravel 11. However, until then I don't have the time to wade through this PR unless the author can clean it up a bit.

funder7 commented 6 months ago

I understand, sorry I saw the commits, but didn't have time either to read all the comments. Anyway if this issue is easy to fix (I guess it is), would be opening a new PR just for it, be a solution?

jasonmccreary commented 6 months ago

I welcome any PRs. It's easier on the maintainer if the changes are concise, have a well explained PR description, and contain passing tests.