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

Add installation command, fix config overwrites #6649

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

toitzi
Copy link
Contributor

@toitzi toitzi commented Sep 20, 2024

Fix group names for pulishing config and assets.

Closes #6645

Q A
Branch? 4.0
Tickets Closes #6645
License MIT
Doc PR api-platform/docs#1989

@toitzi
Copy link
Contributor Author

toitzi commented Sep 20, 2024

Note: Some first-party packages, such as Laravel Telescope or Laravel Horizon, provide an artisan command like php artisan horizon:install to publish service provider stubs, configs, and assets. If you're interested, I can include a similar command in this PR and update the documentation accordingly to reflect this feature.

Let me know if this is something you'd like to incorporate!

@soyuka
Copy link
Member

soyuka commented Sep 20, 2024

If I understand correctly the configuration won't be copied on package install right? Indeed I think we should introduce such a command and add it in the composer.json scripts so that's there no other step then composer require api-platform/laravel

@toitzi
Copy link
Contributor Author

toitzi commented Sep 20, 2024

Yes, while the config won't be copied by default, however this is actually expected behavior for Laravel packages. Even first-party packages like Telescope, Horizon, etc., do not automatically publish configs and assets upon installation. You can see this reflected in their installation instructions (Telescope, Horizon, Scout). The Laravel documentation on package development also outlines this as standard practice in the Laravel ecosystem.

While I'm not deeply familiar with composer post-install/update scripts, they seem to run every time commands like composer install or composer update are executed, which would risk overwriting configs repeatedly. However, if you prefer handling it that way, I can certainly give it a try.

@soyuka
Copy link
Member

soyuka commented Sep 20, 2024

I was thinking that if we create our own api-platform:install command, we could definitely just check if the file exist and not replace it if it does?

Also, I see by using composer create-project laravel/laravel that some commands are executed by default:

20240920_19h18m28s_grim

see also https://github.com/laravel/laravel/blob/11.x/composer.json#L32-L47

I really think that we should have the less steps possible to start a new api platform project, and I agree that replacing the configuration file is not good. @dunglas any opinion on this?

@dunglas
Copy link
Member

dunglas commented Sep 21, 2024

I'll go for the api-platform:install command. Let's use Laravel conventions as much as possible.

@toitzi
Copy link
Contributor Author

toitzi commented Sep 21, 2024

I added the install command, also updated the documentation PR.

@toitzi toitzi changed the title fix(ServiceProvider) Add installation command, fix config overwrites Sep 21, 2024
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

LGTM

src/Laravel/Console/InstallCommand.php Outdated Show resolved Hide resolved
src/Laravel/Console/InstallCommand.php Outdated Show resolved Hide resolved
src/Laravel/Console/InstallCommand.php Outdated Show resolved Hide resolved
added the installation command, but as a bug fix since configs are currently getting overwritten.

Closes: api-platform#6645
@dunglas dunglas merged commit 88bd8c3 into api-platform:4.0 Sep 21, 2024
60 of 61 checks passed
@dunglas
Copy link
Member

dunglas commented Sep 21, 2024

Thanks!!

@toitzi toitzi deleted the patch-1 branch September 21, 2024 11:08
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