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

[1.x] Laravel way polymorphic relationship #18

Closed
wants to merge 8 commits into from
Closed

[1.x] Laravel way polymorphic relationship #18

wants to merge 8 commits into from

Conversation

JohnnyMaynne
Copy link

No description provided.

@milwad-dev milwad-dev changed the title Laravel way polymorphic relationship [1.x] Laravel way polymorphic relationship Dec 7, 2023
@milwad-dev
Copy link
Owner

Did breaking changes?

@JohnnyMaynne
Copy link
Author

JohnnyMaynne commented Dec 7, 2023

Did breaking changes?

Maybe 😊. I accidentally came across the package and did a PR in 2 minutes and didn't test it. I will find time and try to fix it.

I found that you don't use indexes in the database, which can slow down working with large amounts of data, and you also don't use migration helpers for polymorphic relationships, which slightly breaks the "correct way" that the documentation advises.

It also seems to me that the uuid setting is not particularly necessary since you give the opportunity to publish the migration.

@milwad-dev
Copy link
Owner

Good, so please fix the styleci

@JohnnyMaynne
Copy link
Author

Good, so please fix the styleci

I corrected the style

@JohnnyMaynne
Copy link
Author

and I just realized that I didn’t create a migration, but corrected an existing one, it turns out that it will be incompatible with previous versions

@JohnnyMaynne
Copy link
Author

so I'll post this soon too, so don't combine it yet

@JohnnyMaynne
Copy link
Author

seems like everything, but I still haven't tested in the application - I created a new migration in which the columns will be renamed and I also added indexes

{
Schema::table(config('laravel-attributes.tables.name', 'attributes'), function (Blueprint $table) {
$table->renameColumn('attributable', 'attributable_type');
$this->index(['attributable_type', 'attributable_id']);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$table is correct

{
Schema::table(config('laravel-attributes.tables.name', 'attributes'), function (Blueprint $table) {
$table->renameColumn('attributable_type', 'attributable');
$this->index(['attributable_type', 'attributable_id']);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here

@JohnnyMaynne
Copy link
Author

I apologize for the stupid typo, not a very good decision to do PR on the road

@JohnnyMaynne JohnnyMaynne deleted the relations branch December 7, 2023 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants