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.

Sign up for GitHub

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

[BugFix] avoid backpack:build to generate interfaces for abstract models and others #146

Merged
merged 6 commits into from
Dec 20, 2022
Merged

[BugFix] avoid backpack:build to generate interfaces for abstract models and others #146

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c134f1
Load models by namespace instead of assuming it's a model just by fil…
promatik Sep 8, 2022
e1f9860
Merge branch 'main' into load-models-by-namespace
promatik Sep 13, 2022
0aacf2c
Used Reflection
promatik Sep 17, 2022
5410827
Apply fixes from StyleCI
StyleCIBot Sep 17, 2022
679eb91
Update src/Console/Commands/BuildBackpackCommand.php
promatik Oct 9, 2022
a7b4306
Apply fixes from StyleCI
StyleCIBot Oct 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions src/Console/Commands/BuildBackpackCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
namespace Backpack\Generators\Console\Commands;

use Illuminate\Console\Command;
use Illuminate\Support\Arr;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Support\Str;

class BuildBackpackCommand extends Command
Expand Down Expand Up @@ -41,7 +41,7 @@ public function handle()
return;
}

foreach ($models as $key => $model) {
foreach ($models as $model) {
$this->call('backpack:crud', ['name' => $model, '--validation' => $this->option('validation')]);
$this->line(' <fg=gray>----------</>');
}
Expand All @@ -63,10 +63,29 @@ private function getModels($path)
if (is_dir($filename)) {
$out = array_merge($out, $this->getModels($filename));
} else {
$file_content = file_get_contents($filename);
if (Str::contains($file_content, 'Illuminate\Database\Eloquent\Model') &&
Str::contains($file_content, 'extends Model')) {
$out[] = Arr::last(explode('/', substr($filename, 0, -4)));
require_once $filename;

// Try to load it by path as namespace
$class = Str::of($filename)
->after(base_path())
->trim('\\/')
pxpm marked this conversation as resolved.
Show resolved Hide resolved
->replace('/', '\\')
->before('.php')
->ucfirst();

if (is_a($class->value(), Model::class, true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When we have the class, why we don't reflect on it ?
If the reflection fails we are sure it's not a model, if the reflection works we can check if it's a $reflection->isSubclassOf() (or something similar), and reflection also let us check if it's abstract with $reflection->isAbstract().

I think using reflection would also remove the need for: // Try to load it form file content scenario.

Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

jackpot-gif-by-syfy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to Reflection 👌
Much better, it also prevents crashes due to errors on the files 👌

Regarding the need of // Try to load it form file content there may be situations where we can't infer class from file path.

  1. Models misplaced (not sure if possible, but it might(?))
  2. Models inside a package, in the packages folder.

In these cases (maybe others), we need to try to load them from parsing the file content right?

Let me know @pxpm 🙌

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this become a non-issue if we have the paths and namespaces configurable?

I think something like this:

'model_containers' => [
  [
    'namespace' => 'App'
    'path' =>app_path(),
    'recursive' => false,
  ],
  [
    'namespace' => 'App\Models'
    'path' =>app_path('Models'),
    'recursive' => true,
  ],
  [
    'namespace' => 'Vendor\Package',
    'path' =>  app_path('packages/mypackage/models'),
    'recursive' => true
  ]
];

This way the config would say:

  • I want to get all models in the "root" app_path() only (not recursively)
  • I want everything inside app_path('Models') and it should use the base namespace App\Models. We should recursively find models in that directiory, eg: App\Models\Auth\User, App\Models\Blog\Post.
  • I have a local package in my app/packages that also have models, I want them, but the base namespace is Vendor/Package.

It would work for everyone and every project by changing the configs ... the defaults could be "permissive", but I could configure it other way if needed.

Let me know what you guys think @promatik @tabacitu

Copy link
Member

@tabacitu tabacitu Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Configurable model paths is beyond the scope of this PR. And if you ask me - beyond the scope of Generators. DevTools? Sure, that's easier to use. But Generators?... nah.

Let's not expand the scope please.

$out[] = $class->afterLast('\\');
continue;
}

// Try to load it from file content
$fileContent = Str::of(file_get_contents($filename));
$namespace = $fileContent->match('/namespace (.*);/')->value();
$classname = $fileContent->match('/class (\w+)/')->value();

if ($namespace && $classname && is_a("$namespace\\$classname", \Illuminate\Database\Eloquent\Model::class, true)) {
$out[] = $classname;
continue;
}
}
}
Expand Down