search

joselfonseca / lighthouse-graphql-passport-auth

Add GraphQL mutations to get tokens from passport for https://lighthouse-php.com/
https://lighthouse-php-auth.com/
MIT License
228 stars 56 forks source link

Fix for migration error #148

Closed Parables closed 3 years ago

Parables commented 3 years ago

SQLSTATE[42000]: Syntax error or access violation: 1072 Key column 'user_id' doesn't exist in table (SQL: alter table social_providers add constraint social_providers_user_id_foreign foreign key (user_id) references users (id) on delete cascade)

joselfonseca commented 3 years ago

Thanks for the PR, the checks are not passing, can you please see why it does not pass and fix it to be able to check the PR.

Parables commented 3 years ago

Thanks for replying back. On a fresh installation of Laravel and your package, running php artisan migrate throws the following error

SQLSTATE[42000]: Syntax error or access violation: 1072 Key column
'user_id' doesn't exist in table (SQL: alter table social_providers add
constraint social_providers_user_id_foreign foreign key (user_id)
references users (id) on delete cascade)

So further digging into the problem before filing an issue lead me to discover that the foreign key constraint there is causing the migration to fail so I simply rewrote that part like this:


// Fix for migration error:
+        /*  SQLSTATE[42000]: Syntax error or access violation: 1072
Key column 'user_id' doesn't exist in table (SQL: alter table
`social_providers` add constraint `social_providers_user_id_foreign`
foreign key (`user_id`) references `users` (`id`) on delete cascade)
*/
+        Schema::table('social_providers', function (Blueprint $table) {
+            $table->foreignId('user_id')->after('provider_id')->constrained()->onDelete('cascade');;

I have resolved all the requested changes and tested it out and it works. Awaiting your approval.

Thanks again for this amazing package. Made my life so easy.

On Sun, Jun 13, 2021 at 1:21 AM Jose Luis Fonseca @.***> wrote:

Thanks for the PR, I am not sure I understand the reason to delete the user id from the table as well as the foreign key. Also the checks are not passing. Can you please elaborate on the issue that led you to propose the PR?

Thanks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/148#issuecomment-860134606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJDDF3TVPR3TWILNR5S55SLTSQBY5ANCNFSM46TE3DJA .

joselfonseca commented 3 years ago

Thanks for addressing the feedback @Parables Unfortunately the check for PHP 8 Laravel 6 is not passing

https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/148/checks?check_run_id=2820018279

The method foreignId was introduced in a later version of laravel and is not compatible with Laravel 6. The reason why we are currently supporting laravel 6 is because is the current LTS.

https://laravel.com/docs/8.x/releases#support-policy

So we need to keep it compatible. Thanks for the PR and I will take a look at the issue you mention to see if it is present in a fresh installation. If you want to update your PR to support Laravel 6 please go ahead and I will take a look again. Thanks!

Parables commented 3 years ago

Thank you so much for this amazing package and its a great honour for me to contribute to it. I will do as you have said and report back to you

On Mon, Jun 14, 2021 at 1:11 PM Jose Luis Fonseca @.***> wrote:

Thanks for addressing the feedback @Parables https://github.com/Parables Unfortunately the check for PHP 8 Laravel 6 is not passing

https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/148/checks?check_run_id=2820018279

The method foreignId was introduced in a later version of laravel and is not compatible with Laravel 6. The reason why we are currently supporting laravel 6 is because is the current LTS.

https://laravel.com/docs/8.x/releases#support-policy

So we need to keep it compatible. Thanks for the PR and I will take a look at the issue you mention to see if it is present in a fresh installation. If you want to update your PR to support Laravel 6 please go ahead and I will take a look again. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/148#issuecomment-860672742, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJDDF3TGIM5SPKVUEN22KYDTSX5YFANCNFSM46TE3DJA .

Parables commented 3 years ago

Apparently, it looks that the order of migrations on my end was not in order causing the social_provider_users_table to execute before the users migration was actually done

I don't think this is an actual bug in your package but it can lead to frustration for new developers.

I suggest that we can check if the users table exists before setting up a foreign key constraint or simply create it if it doesn't exist and set the foreign key constraint

On Mon, Jun 14, 2021 at 5:14 PM Parables Boltnoel @.***> wrote:

Thank you so much for this amazing package and its a great honour for me to contribute to it. I will do as you have said and report back to you

On Mon, Jun 14, 2021 at 1:11 PM Jose Luis Fonseca < @.***> wrote:

Thanks for addressing the feedback @Parables https://github.com/Parables Unfortunately the check for PHP 8 Laravel 6 is not passing

https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/148/checks?check_run_id=2820018279

The method foreignId was introduced in a later version of laravel and is not compatible with Laravel 6. The reason why we are currently supporting laravel 6 is because is the current LTS.

https://laravel.com/docs/8.x/releases#support-policy

So we need to keep it compatible. Thanks for the PR and I will take a look at the issue you mention to see if it is present in a fresh installation. If you want to update your PR to support Laravel 6 please go ahead and I will take a look again. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/148#issuecomment-860672742, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJDDF3TGIM5SPKVUEN22KYDTSX5YFANCNFSM46TE3DJA .

Parables commented 3 years ago

PR created. This pull might be bloated but well it does get rid of any error.

I create the users table if it doesn't exist and add the avatar column but if it exists, I add the avatar column to the existing users table.

The social_providers table is reordered with the user_id foreign key placed above the timestamps

On Mon, Jun 14, 2021 at 6:03 PM Parables Boltnoel @.***> wrote:

Apparently, it looks that the order of migrations on my end was not in order causing the social_provider_users_table to execute before the users migration was actually done

I don't think this is an actual bug in your package but it can lead to frustration for new developers.

I suggest that we can check if the users table exists before setting up a foreign key constraint or simply create it if it doesn't exist and set the foreign key constraint

On Mon, Jun 14, 2021 at 5:14 PM Parables Boltnoel @.***> wrote:

Thank you so much for this amazing package and its a great honour for me to contribute to it. I will do as you have said and report back to you

On Mon, Jun 14, 2021 at 1:11 PM Jose Luis Fonseca < @.***> wrote:

Thanks for addressing the feedback @Parables https://github.com/Parables Unfortunately the check for PHP 8 Laravel 6 is not passing

https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/148/checks?check_run_id=2820018279

The method foreignId was introduced in a later version of laravel and is not compatible with Laravel 6. The reason why we are currently supporting laravel 6 is because is the current LTS.

https://laravel.com/docs/8.x/releases#support-policy

So we need to keep it compatible. Thanks for the PR and I will take a look at the issue you mention to see if it is present in a fresh installation. If you want to update your PR to support Laravel 6 please go ahead and I will take a look again. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/148#issuecomment-860672742, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJDDF3TGIM5SPKVUEN22KYDTSX5YFANCNFSM46TE3DJA .

joselfonseca commented 3 years ago

@Parables Thanks for all the attention to this PR, I am still not sure where the issue is. I'll try to do a fresh installation and see if I run into any issues.

Parables commented 3 years ago

I really appreciate your time and support. This is not an actual issue with this package so please feel free to close this issue.

One more thing, please can you provide a guide on how to get email verification working just like you have a tutorial for auth.

I first used Laravel in V.5 but I didn't continue to learn it until now so I'm not that experienced with the Laravel Ecosystem.

I believe it will be helpful to beginners like me.

On Tue, Jun 15, 2021, 9:44 PM Jose Luis Fonseca @.***> wrote:

@Parables https://github.com/Parables Thanks for all the attention to this PR, I am still not sure where the issue is. I'll try to do a fresh installation and see if I run into any issues.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/joselfonseca/lighthouse-graphql-passport-auth/pull/148#issuecomment-861853715, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJDDF3WMTWDAE2AXGZSQ2H3TS7CSPANCNFSM46TE3DJA .