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 --force argument to php artisan backpack:crud #187

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

karandatwani92
Copy link
Contributor

WHY

To address this #186

AFTER - What is happening after this PR?

Added --force argument to php artisan backpack:crud

Is it a breaking change or non-breaking change?

Non-breaking

How can we test the before & after?

php artisan backpack:crud item
php artisan backpack:crud item --force

@tabacitu
Copy link
Member

Awesome! Is this ready @karandatwani92 ? If so, please assign @pxpm to review, test and merge.

@karandatwani92
Copy link
Contributor Author

Yes @tabacitu , this is ready for review.

Hey @pxpm, Please give it a read and test? Thanks 🙃

Copy link
Contributor

@pxpm pxpm left a comment

Choose a reason for hiding this comment

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

Hey @karandatwani92 thanks for the clean solution.
I've just added one suggestion to make it more similar to what we already have, small detail, nothing special.
Everything seems to be working as expected, good job 🙏

I tried the same with php artisan backpack:model SomeModel expecting it would also work. Sadly it doesn't.
image

I think if we are adding the --force option we should do it consistently.
If you can force a model to regenerate during a :crud command, you should also be able to force generation when using it the single command :model. Same for :request and :controller.

At least those because are the ones that we are introducing the --force option via :crud command.

What do you think ?

Cheers 🙏

@@ -52,14 +52,14 @@ public function handle()
}

// Create the CRUD Model and show output
$this->call('backpack:crud-model', ['name' => $name]);
$this->call('backpack:crud-model', ['name' => $nameTitle, '--force' => $this->option('force')]);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably store the --force option up close where we init other command properties like $name, $nameTitle etc. We then re-use the variable across the command 👍 This is just personal preference, it's not that is bad implemented.

Suggested change
$this->call('backpack:crud-model', ['name' => $nameTitle, '--force' => $this->option('force')]);
$this->call('backpack:crud-model', ['name' => $nameTitle, '--force' => $force]);

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also take a look at this @karandatwani92 let me know if you think we shouldn't do this, otherwise please do it 👍

@pxpm pxpm assigned karandatwani92 and unassigned pxpm Mar 13, 2023
@karandatwani92
Copy link
Contributor Author

@pxpm,

I didn't knew backpack has following signatures:
php artisan backpack:model
php artisan backpack:request

And this doesn't exists
php artisan backpack:controller

Docs only shows the following which i have already covered:

Command Description.
php artisan backpack:crud-controller Generate a Backpack CRUD controller.
php artisan backpack:crud-model Generate a Backpack CRUD model
php artisan backpack:crud-request Generate a Backpack CRUD request
php artisan backpack:crud-operation Generate a custom Backpack CRUD operation trait

@karandatwani92
Copy link
Contributor Author

I found these two were unmaintained and replaced by newer commands.
So, as a final solution i have refactored these two to call newer backpack commands to keep up with newer functionalities.

php artisan backpack:model
php artisan backpack:request

@pxpm
Copy link
Contributor

pxpm commented Apr 24, 2023

I found these two were unmaintained and replaced by newer commands. So, as a final solution i have refactored these two to call newer backpack commands to keep up with newer functionalities.

php artisan backpack:model
php artisan backpack:request

I think this is not correct. backpack:model had a fix not so much time ago: 42d2304 so there may be people still using that command.

If we plan do remove it in favor of the new command it need to be on a new version with a breaking change.
Since we have only the "new commands" documented, and they do the same (or better) than the old ones, we can just add the --force to the new ones and leave the old ones untouched.
Is there something that backpack:model does that backpack:crud-model don't ? what are the differences between them?

Cheers

@pxpm pxpm assigned karandatwani92 and unassigned pxpm Apr 24, 2023
@karandatwani92
Copy link
Contributor Author

Hi @pxpm
I rechecked, both used to do the same thing with no differences. So I think you can merge.

@karandatwani92 karandatwani92 assigned pxpm and unassigned karandatwani92 May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants