Skip to content

Conversation

@artengin
Copy link
Contributor

@artengin artengin commented Oct 6, 2025

Refs: #42

@coveralls
Copy link

coveralls commented Oct 6, 2025

Pull Request Test Coverage Report for Build 18334096819

Details

  • 21 of 21 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 97.973%

Totals Coverage Status
Change from base Build 18301590721: 0.06%
Covered Lines: 290
Relevant Lines: 296

💛 - Coveralls

@DenTray DenTray requested a review from Copilot October 6, 2025 18:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes database environment variable configuration during the initialization process by replacing direct Artisan calls with shell_exec commands to ensure proper environment setup for migrations.

  • Replaces Artisan::call('migrate') with a new runMigrations() method that configures database environment variables before running migrations via shell_exec
  • Changes Artisan::call('vendor:publish') to shell_exec in the publishWebLogin() method for consistency
  • Updates test expectations to include the new shell_exec calls and removes the assertWebLoginPublished() method calls from test assertions

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/Commands/InitCommand.php Adds database configuration setup in new runMigrations() method and switches to shell_exec for vendor:publish command
tests/InitCommandTest.php Updates test expectations to match new shell_exec approach and removes web login assertions
tests/TestCase.php Removes unused assertFileEqualsFixture() method

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

]);
}

shell_exec("php artisan migrate");
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

Using shell_exec without error handling could mask migration failures. Consider checking the return value or using a more robust command execution method that can capture and handle errors.

Suggested change
shell_exec("php artisan migrate");
$output = [];
$exitCode = 0;
exec("php artisan migrate", $output, $exitCode);
if ($exitCode !== 0) {
$this->error("Migration failed with exit code {$exitCode}.\nOutput:\n" . implode("\n", $output));
throw new \RuntimeException("Migration failed. See error output above.");
}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@artengin I not sure, could you please check it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method works,
but the shell command also outputs error messages from the migrations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exec() :
image

shell_exec:

$output = shell_exec('php artisan migrate --ansi 2>&1');
echo $output;
image image

]);
}

shell_exec("php artisan migrate");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@artengin I not sure, could you please check it?

@DenTray DenTray assigned artengin and unassigned DenTray Oct 7, 2025
@artengin
Copy link
Contributor Author

artengin commented Oct 7, 2025

The following errors:

PHP Fatal error: Declaration of Monolog\Logger::emergency(Stringable|string $message, array $context = []): void must be compatible with Psr\Log\LoggerInterface::emergency($message, array $context = []) in /var/www/test/test-laravel/vendor/monolog/monolog/src/Monolog/Logger.php on line 683
PHP Fatal error: Uncaught Error: Class "Monolog\Logger" not found in /var/www/test/test-laravel/vendor/monolog/monolog/src/Monolog/Handler/AbstractHandler.php:60

did not occur because of our package, but due to an internal issue in Laravel.

Reference to the related issue in laravel/framefork #57295

As a result,, using --no-scripts is no longer necessary, because it was causing other problems for us.

@artengin
Copy link
Contributor Author

artengin commented Oct 7, 2025

Tested on the laravel-empty-project — everything works.
image

@DenTray DenTray merged commit eec24b3 into main Oct 8, 2025
3 checks passed
@DenTray DenTray deleted the update-pgsql-migrations-if-not-set branch October 8, 2025 04:48
@artengin
Copy link
Contributor Author

artengin commented Oct 8, 2025

If we add an echo to all the commands to display a message about the execution result, it will look like this:
image
image
image
image
image

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