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

Not work artisan after install this package #131

Closed
JeRabix opened this issue Apr 28, 2022 · 11 comments
Closed

Not work artisan after install this package #131

JeRabix opened this issue Apr 28, 2022 · 11 comments

Comments

@JeRabix
Copy link

JeRabix commented Apr 28, 2022

After install this package, artisan stops its work (any of artisan commands has no effect) and keep working after uninstall this package

php artisan *

image

@rubenvanassche
Copy link
Member

Which php version + Laravel version are you using? + could you run php artisan -vvv?

@jpswade
Copy link

jpswade commented May 24, 2022

I'm also seeing this issue.

Essentially after doing composer require spatie/laravel-settings which brings in "spatie/laravel-settings": "^2.4" into composer.json.

Once installed, it seems to try and include every php file when you run php artisan. In my example, it tries to run a provisioning script which has a die in it, and so dies with that message.

If I put dd(count(get_included_files())); in the file where it dies, I get 883, when I remove spatie/laravel-settings, that doesn't even get hit.

It's as if it is trying to load every PHP file.

% cat composer.json | grep laravel/framework
        "laravel/framework": "^8.0",

% php -r "echo phpversion();"
8.1.5%                                          

Edit: I tried to roll back to an older version, but it seemed to exist in every version. I couldn't install 2.1.3 or below, due to a conflict with spatie/data-transfer-object v3, so I've not been able to narrow down when this issue would have been introduced.

Edit: I've narrowed it down to the discovery method here: https://github.com/spatie/laravel-settings/blob/main/src/Support/DiscoverSettings.php#L55

@rubenvanassche
Copy link
Member

The settings package indeed checks each file in the discover paths for settings classes so they can be registered correctly in the container. We filter out any files which will break because they are invalid:

try {
return is_subclass_of($settingsClass, Settings::class) &&
(new ReflectionClass($settingsClass))->isInstantiable();
} catch (Throwable $e) {
return false;
}

So normally this process cannot crash. Could you check which line makes artisan crash (adding a dd() in each step and see where it breaks). When we make this process a little less brittle artisan should work as expected.

Another solution is disabling auto discovery and manually adding your settings classes to the config file.

In production we do not want to run this auto discovery, because it will slowly load all your php code files. That's why you should always cache the file discovery when running this package in production.

@jpswade
Copy link

jpswade commented Jun 17, 2022

Hey @rubenvanassche ,

Thanks, as far as I could tell the auto discovery process tries to include and load every single php file in your application.

For us, and presumably others, when loading this into a mature application that could have unknown, unexpected and potentially detrimental effects, albeit locally.

For that reason disabling auto discovery by default is the recommended solution as it does resolve this issue, unless there can be a more reliable (less fragile) way to "discover" settings files. If nothing else, it will save others in a similar situation repeatedly debugging this same issue.

In our case I found that it was trying to load a php file that contained an exit and that was causing artisan to always exit.

@rubenvanassche
Copy link
Member

In our case I found that it was trying to load a php file that contained an exit and that was causing artisan to always exit.

That's kinda strange since the discover process only reflects files and does not require or include them. An exit shouldn't be executed then. Any idea how this could have happened in your codebase?

We do have a process where we remove settings migrations from a loaded schema:

$files = Finder::create()
->files()
->ignoreDotFiles(true)
->in($this->resolveMigrationPaths())
->depth(0);
$migrations = collect(iterator_to_array($files))
->mapWithKeys(function (SplFileInfo $file) {
preg_match('/class\s*(\w*)\s*extends/', file_get_contents($file->getRealPath()), $found);
if (empty($found)) {
return null;
}
require_once $file->getRealPath();
return [$file->getBasename('.php') => $found[1]];
})
->filter(fn (string $migrationClass) => is_subclass_of($migrationClass, SettingsMigration::class))
->keys();
$event->connection
->table(config()->get('database.migrations'))
->useWritePdo()
->whereIn('migration', $migrations)
->delete();

This one will require files and in such case an exit could stop artisan. But the files included are only the settings migrations so I wouldn't expect an exit over there.

@nick322
Copy link

nick322 commented Jul 15, 2022

I had a similar problem, just not as severe as @JeRabix.
I removed the non-existing class and reinstalled the package.
I would suggest putting the "auto_discover_settings" preset in a path like app/settings.
Avoid scanning the entire app folder

    /*
      * The package will look for settings in these paths and automatically
      * register them.
      */
     'auto_discover_settings' => [
         app()->path('Settings'),
     ],

Finally, a configuration (php artisan vendor:publish --provider="Spatie\LaravelSettings\LaravelSettingsServiceProvider" --tag="settings") file is generated, which allows engineers to decide the path by themselves.

another thing
You can add settings:discover in composer.json

  "post-autoload-dump": [
             "Illuminate\\Foundation\\ComposerScripts::postAutoloadDump",
             "@php artisan package:discover",
            -> "@php artisan settings:discover"
         ],

The settings can be loaded automatically at deployment time.

@rubenvanassche
Copy link
Member

Updating the default would be a breaking change since not everybody is publishing the config file. So, we're not going to do that. That being said, this code should be more robust.

The plan is actually to move all the code for discovering classes within Spatie packages to: https://github.com/spatie/laravel-auto-discoverer. I'm planning the move for settings next week or the week after that.

Could you describe me what actually went wrong within your application and why our package crashed, which class caused the crash. The package we're building above should be able to scan the app folder without problems so I want to make it as robust as possible.

@nick322
Copy link

nick322 commented Jul 18, 2022

My project itself does not use Auth for some reasons.
After laravel upgrade, some Auth related classes have not been removed.
I got this error when installing the package. As follows
In the end, I just deleted it and reinstalled it successfully.

❯ composer require spatie/laravel-settings
Using version ^2.4 for spatie/laravel-settings
./composer.json has been updated
Running composer update spatie/laravel-settings
Loading composer repositories with package information
Updating dependencies                                 
Lock file operations: 3 installs, 0 updates, 0 removals
  - Locking spatie/data-transfer-object (2.8.4)
  - Locking spatie/laravel-settings (2.4.2)
  - Locking spatie/temporary-directory (1.3.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 3 installs, 0 updates, 0 removals
  - Installing spatie/temporary-directory (1.3.0): Extracting archive
  - Installing spatie/data-transfer-object (2.8.4): Extracting archive
  - Installing spatie/laravel-settings (2.4.2): Extracting archive
1 package suggestions were added by new dependencies, use `composer suggest` to see details.
Package box/spout is abandoned, you should avoid using it. No replacement was suggested.
Package swiftmailer/swiftmailer is abandoned, you should avoid using it. Use symfony/mailer instead.
Package ukfast/laravel-health-check is abandoned, you should avoid using it. Use ans-group/laravel-health-check instead.
Generating optimized autoload files
> Illuminate\Foundation\ComposerScripts::postAutoloadDump
> @php artisan package:discover

   Symfony\Component\ErrorHandler\Error\FatalError 

  Trait 'Illuminate\Foundation\Auth\SendsPasswordResetEmails' not found

  at app/Http/Controllers/Auth/ForgotPasswordController.php:8
      4▕ 
      5▕ use App\Http\Controllers\Controller;
      6▕ use Illuminate\Foundation\Auth\SendsPasswordResetEmails;
      7▕ 
  ➜   8▕ class ForgotPasswordController extends Controller
      9▕ {
     10▕     /*
     11▕     |--------------------------------------------------------------------------
     12▕     | Password Reset Controller

      +23 vendor frames 
  24  artisan:37
      App\Console\Kernel::handle("class Symfony\Component\Console\Input\ArgvInput { private $tokens = [0 => 'package:discover']; private $parsed = NULL; protected $definition = class Symfony\Component\Console\Input\InputDefinition { private $arguments = [...]; private $requiredCount = 0; private $lastArrayArgument = NULL; private $lastOptionalArgument = NULL; private $options = [...]; private $negations = [...]; private $shortcuts = [...] }; protected $stream = NULL; protected $options = []; protected $arguments = []; protected $interactive = TRUE }", "class Symfony\Component\Console\Output\ConsoleOutput { private $stderr = class Symfony\Component\Console\Output\StreamOutput { private $stream = resource(3) of type (stream); private ${Symfony\Component\Console\Output\Output}verbosity = 32; private ${Symfony\Component\Console\Output\Output}formatter = class Symfony\Component\Console\Formatter\OutputFormatter { ... } }; private $consoleSectionOutputs = []; private ${Symfony\Component\Console\Output\StreamOutput}stream = resource(2) of type (stream); private ${Symfony\Component\Console\Output\Output}verbosity = 32; private ${Symfony\Component\Console\Output\Output}formatter = class Symfony\Component\Console\Formatter\OutputFormatter { private $decorated = TRUE; private $styles = [...]; private $styleStack = class Symfony\Component\Console\Formatter\OutputFormatterStyleStack { ... } } }")

  25  artisan:0
      {main}()

   Whoops\Exception\ErrorException 

  Trait 'Illuminate\Foundation\Auth\SendsPasswordResetEmails' not found

  at app/Http/Controllers/Auth/ForgotPasswordController.php:8
      4▕ 
      5▕ use App\Http\Controllers\Controller;
      6▕ use Illuminate\Foundation\Auth\SendsPasswordResetEmails;
      7▕ 
  ➜   8▕ class ForgotPasswordController extends Controller
      9▕ {
     10▕     /*
     11▕     |--------------------------------------------------------------------------
     12▕     | Password Reset Controller

      +1 vendor frames 
  2   [internal]:0
      Whoops\Run::handleShutdown()
Script @php artisan package:discover handling the post-autoload-dump event returned with error code 255

@rubenvanassche
Copy link
Member

Hmmm seems that creating a ReflectionClass from a class using an unknown trait will cause a fatal error which is uncatchable (thanks PHP). Not sure how we're going to solve this problem 🤔

@jpswade
Copy link

jpswade commented Jul 18, 2022

I think a breaking change of moving all settings to a settings directory is a good idea in the long run, if people don’t want that they can simplify modify their configuration or stay on an older version.

@rubenvanassche
Copy link
Member

Yeah let's make this a default in the next major version of the package.

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

No branches or pull requests

5 participants