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

[BUG] backpack:build copy found models in other directories (e.g. app/packages) into app/Models . #105

Open
pxpm opened this issue Jan 26, 2021 · 7 comments

Comments

@pxpm
Copy link
Contributor

pxpm commented Jan 26, 2021

Bug report

What I did:

I have some local packages (Models, Controllers etc) inside app/packages folder.

When running php artisan backpack:build it find those models too. Problem is not finding them, is the fact the backpack copy them into app/Models folder.

This makes me also think we should have some key in config where we could add an array of locations to ignore searching for models.

What I expected to happen:

Atmost, find the models and add the CrudTrait. Never copy them!

What happened:

My models were copied into app/Models

What I've already tried to fix it:

Backpack, Laravel, PHP, DB version:

@welcome
Copy link

welcome bot commented Jan 26, 2021

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication mediums:

  • Bug Reports, Feature Requests - Github Issues (here);
  • Quick help (How do I do X) - Gitter Chatroom;
  • Long questions (I have done X and Y and it won't do Z wtf) - Stackoverflow, using the backpack-for-laravel tag;

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

--
Justin Case
The Backpack Robot

@pxpm pxpm added the bug label Jan 26, 2021
@promatik
Copy link
Contributor

Generator will search everything inside \app folder, and every time it finds a file extending a Model, it will add the model name only, to a list of models.

This is the problem, it's finding a Test.php model inside any other folder, and it's not saving the path, but the name Test. And then it creates everything related with that model, Controller, Request ... and Model inside the models folder.

@pxpm you may thought it "copied" the file, but it actually crated a new Model.

I don't think Backpack should be worrying with models inside other folders other than \app\*.php and \app\Models\*.php.

Because if we were going to worry about models in \app\packages\Test\app\Models\Test.php the Controller and Request should be created inside that structure right?

Let me know what you think.

@tabacitu
Copy link
Member

tabacitu commented Feb 2, 2021

Here's what I think we should do, from your conversation:

  • Generator should make a list of model classpaths, not just model names;
  • Generator should add CrudTrait to ANY model, doesn't matter where it is; totally agree, it should NOT re-create the model if it exists (no matter where it is), it should just add CrudTrait to it;

Additionally, to cover a very common use case where you have a BUNCH of models, don't need CRUDs for all of them, but don't want to run a command for each model either, we should let the user choose models to exclude. We should keep in mind that we'll want to use this same command inside DevTools, where we'll have a better interface for this:

  • run "Build all"
  • it'll show a list of Models that don't have CRUDs (all checkboxes are checked)
  • you uncheck which Models you want to exclude
  • submit the form/modal and it'll run THIS command, with a flag (ex: php artisan backpack:build --ignore=App\Models\Product,App\Packages\Something\Models\Thing

To account for that future feature, but also provide a similar feature in the command line, what I propose we do is that we:

  • instead of building all models by default, would do something like this:
> php artisan backpack:build

Found 20 models without a CRUD:
- App\Models\Product
- App\Packages\Something\Models\Something
- ...
- ...
- ...
Would you like to build CRUDs for ALL the models above (Y)? Alternatively, reply (N) and we will ask 
you for each model individually, if it requires a CRUD to be built.

> N

Build CRUD for App\Models\Product? (Y/N - default Y in 15 sec)

> Y

Build CRUD for App\Packages\Something\Models\Something (Y/N - default Y in 15 sec)

> N

What do you guys think?

(notice I've moved this to 4.2 - let's finish the bugs first, and afterwards Generators will be our first focus for improvements)

@promatik
Copy link
Contributor

promatik commented Feb 4, 2021

I think it's the best approach 👍

Maybe we can use Artisan Console choice, there's a allowMultipleSelections option.
It's easier than answering Noon ...build CRUDs for ALL the models above and have to choose one by one if we want the CRUD.

@promatik
Copy link
Contributor

promatik commented Feb 4, 2021

Or maybe something like the package https://github.com/eddiriarte/console-select
Since the default Artisan multiple selection is not that intuitive.

image

@tabacitu
Copy link
Member

tabacitu commented Feb 8, 2021

Console-select looks good indeed, but I see its last update was in 2018... I'm a bit wary about adding a dependency like this...

How does artisan's choice & allowMultipleSelections work differently from this?

@promatik
Copy link
Contributor

promatik commented Feb 8, 2021

@tabacitu, artisan choice is not "interactive", you have the options listed, and you have to manually type your selected options (comma separated I think) 1, 2, 3 ...

[1] App\Models\Article
[2] App\Models\Category
[3] App\Models\Tag
[4] App\Packages\Something\Models\Test
[5] App\Packages\Something\Models\AnotherTest
> 1, 2, 3

For me it's enough, I totally agree in avoiding dependencies 😅 but we may need to add a note to instruct devs on how to use artisan choice, they may not realize they can choose many.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants