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

Added default for missing User model #90

Closed
wants to merge 6 commits into from

Conversation

kevinmeijer97
Copy link
Member

The added User model is the complete User model with the Statamic changes added to it as well.

@@ -186,6 +189,18 @@ public function bootPublishables() : self
__DIR__.'/../config/rapidez/statamic.php' => config_path('rapidez/statamic.php'),
], 'config');

if (!class_exists('\App\Models\User::class') && $userModel = $this->prepareUserModel()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this always place a user model in your storage directory? Even when not publishing? Maybe the publishing functionality is not exactly the way to go here, because we can also just use the User model from the module, doesn't that work too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes i agree, this was not the way to go.
Changed it so that it's loaded from the module now!

BobWez98
BobWez98 previously approved these changes Nov 18, 2024
config(['auth.providers.users.model' => User::class]);
}

if (!Schema::hasTable('users') && !file_exists(database_path('migrations/0001_01_01_000000_create_users_table.php'))) {
Copy link
Member

Choose a reason for hiding this comment

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

You don't want to have database actions within the service provider as this runs even when you didn't have this setup yet for example with composer

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the hasTable check, it's not really needed for this anyways. Checking if the migration file exists is enough.

BobWez98
BobWez98 previously approved these changes Dec 4, 2024
Copy link
Member

@royduin royduin left a comment

Choose a reason for hiding this comment

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

What about these steps? https://statamic.dev/tips/storing-users-in-a-database#in-an-existing-laravel-app, do we also need to do something with that here?

Copy link
Member

Choose a reason for hiding this comment

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

Instead keeping a copy within this repo we could get the content from https://github.com/laravel/laravel/blob/11.x/database/migrations/0001_01_01_000000_create_users_table.php? Maybe just a file_get_contents() on the publish?

@kevinmeijer97
Copy link
Member Author

Instead keeping a copy within this repo we could get the content from https://github.com/laravel/laravel/blob/11.x/database/migrations/0001_01_01_000000_create_users_table.php? Maybe just a file_get_contents() on the publish?

Changed this around to get a copy from the repo instead.

What about these steps? https://statamic.dev/tips/storing-users-in-a-database#in-an-existing-laravel-app, do we also need to do something with that here?

These steps are in the docs at this moment, only doing this automatically right now because the user model + migration is needed for the install of this addon.

@BobWez98
Copy link
Collaborator

Conflicts

$userTable = file_get_contents('https://raw.githubusercontent.com/laravel/laravel/refs/heads/11.x/database/migrations/0001_01_01_000000_create_users_table.php');

if (!$userTable) {
throw new Exception('Failed to download the migration file.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

So if laravel moves this file, we'll get exceptions on every production site that includes this code? 😲

Copy link
Member Author

Choose a reason for hiding this comment

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

No, as this will be caught and logged in the catch.
Also will only happen if there is no existing migration, which should never be on a production environment.

@royduin
Copy link
Member

royduin commented Dec 19, 2024

I'm not really sure about this PR, downloading from a boot method; having the user model within here as fallback. Maybe we've to talk this through later.

@BobWez98
Copy link
Collaborator

I'm not really sure about this PR, downloading from a boot method; having the user model within here as fallback. Maybe we've to talk this through later.

Agreed

@royduin
Copy link
Member

royduin commented Jan 24, 2025

Closing in favor of #111

@royduin royduin closed this Jan 24, 2025
@royduin royduin deleted the feature/default-user-model branch January 24, 2025 08:41
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.

3 participants