Skip to content

fix: double context on custom taps #4

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

Merged
belisarh merged 1 commit into from
Jul 1, 2025
Merged

fix: double context on custom taps #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion config/logging-monitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
<?php

// Monitor uses Laravel's default logging channel (Log::info(), Log::error(), etc.)
// so it works with whatever you have configured as your default channel.
//
// If you want Monitor logs to go to a separate channel, you can:
// 1. Set a different default channel in your logging.php config, OR
// 2. Create a dedicated channel and manually use it like Log::channel('monitor')->info()
//
// The example below shows how you could configure a dedicated channel with Monitor's
// structured logging tap, but this is completely optional.

return [
'channels' => [
'monitor' => [
'monitor_example' => [
'driver' => 'daily',
'path' => storage_path('logs/monitor.log'),
'level' => 'debug',
Expand Down
14 changes: 1 addition & 13 deletions src/MonitorServiceProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public function register(): void
$this->app->singleton(CircuitBreaker::class, fn () => new CircuitBreaker);
$this->app->singleton(ControlledContext::class, fn () => new ControlledContext);

$this->mergeLoggingChannelsFrom(__DIR__.'/../config/logging-monitor.php', 'logging');
// Remove automatic logging channel merging - users should configure their own channels
}

public function boot(): void
Expand All @@ -45,16 +45,4 @@ public function boot(): void
app(Trace::class)->start();
}
}

protected function mergeLoggingChannelsFrom(string $path, string $key): void
{
$existingChannels = (array) config("{$key}.channels", []);

/** @var array{channels?: array<string, mixed>} $packageChannels */
$packageChannels = require $path;

config([
"{$key}.channels" => array_merge($packageChannels['channels'] ?? [], $existingChannels),
]);
}
}
291 changes: 0 additions & 291 deletions tests/Feature/MonitorServiceProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,89 +40,6 @@ public function test_services_return_correct_types()
->and($this->app->make(LogTimer::class))->toBeInstanceOf(LogTimer::class);
}

public function test_merges_logging_channels_from_package_config()
{
// Set up existing logging config
config(['logging.channels.existing' => ['driver' => 'single']]);

// Re-register the service provider to trigger config merging
$provider = new MonitorServiceProvider($this->app);
$provider->register();

$channels = config('logging.channels');

// Should contain existing channels
expect($channels['existing'])->toBe(['driver' => 'single']);

// Should contain package channels (from logging-monitor.php)
expect($channels)->toHaveKey('monitor');
}

public function test_preserves_existing_logging_channels()
{
// Set up multiple existing channels
config([
'logging.channels.single' => ['driver' => 'single'],
'logging.channels.daily' => ['driver' => 'daily'],
'logging.channels.slack' => ['driver' => 'slack'],
]);

// Re-register to trigger merging
$provider = new MonitorServiceProvider($this->app);
$provider->register();

$channels = config('logging.channels');

// All existing channels should be preserved
expect($channels['single'])->toBe(['driver' => 'single'])
->and($channels['daily'])->toBe(['driver' => 'daily'])
->and($channels['slack'])->toBe(['driver' => 'slack']);
}

public function test_handles_empty_package_channels_config()
{
// Mock the config file to return empty channels
config(['logging.channels.existing' => ['driver' => 'single']]);

// Create a temporary file with empty channels
$tempPath = tempnam(sys_get_temp_dir(), 'test_logging_monitor');
file_put_contents($tempPath, '<?php return ["channels" => []];');

// Use reflection to test the protected method
$provider = new MonitorServiceProvider($this->app);
$reflection = new \ReflectionClass($provider);
$method = $reflection->getMethod('mergeLoggingChannelsFrom');
$method->setAccessible(true);

$method->invoke($provider, $tempPath, 'logging');

// Should preserve existing channels
expect(config('logging.channels.existing'))->toBe(['driver' => 'single']);

unlink($tempPath);
}

public function test_handles_missing_channels_key_in_package_config()
{
config(['logging.channels.existing' => ['driver' => 'single']]);

// Create config file without 'channels' key
$tempPath = tempnam(sys_get_temp_dir(), 'test_logging_monitor');
file_put_contents($tempPath, '<?php return ["other_config" => "value"];');

$provider = new MonitorServiceProvider($this->app);
$reflection = new \ReflectionClass($provider);
$method = $reflection->getMethod('mergeLoggingChannelsFrom');
$method->setAccessible(true);

$method->invoke($provider, $tempPath, 'logging');

// Should preserve existing channels
expect(config('logging.channels.existing'))->toBe(['driver' => 'single']);

unlink($tempPath);
}

public function test_publishes_config_files()
{
// Test that publishing is registered
Expand Down Expand Up @@ -247,22 +164,6 @@ public function test_boot_console_logic(): void
expect($trace->hasNotStarted())->toBeTrue();
}

public function test_merge_logging_channels_handles_no_existing_config()
{
// Clear any existing logging config
config(['logging.channels' => null]);

// Re-register to trigger merging
$provider = new MonitorServiceProvider($this->app);
$provider->register();

$channels = config('logging.channels');

// Should have package channels even with no existing config
expect($channels)->toBeArray()
->and($channels)->toHaveKey('monitor');
}

public function test_app_binding_works_through_helper()
{
// Test that app() helper works with our registered services
Expand Down Expand Up @@ -292,28 +193,6 @@ public function test_boot_method_integration()
expect($exception)->toBeNull();
}

public function test_config_merging_edge_cases()
{
// Test merging with existing null config
config(['logging.channels' => null]);

$provider = new MonitorServiceProvider($this->app);
$reflection = new \ReflectionClass($provider);
$method = $reflection->getMethod('mergeLoggingChannelsFrom');
$method->setAccessible(true);

// Create a temp config file
$tempPath = tempnam(sys_get_temp_dir(), 'test_logging_monitor');
file_put_contents($tempPath, '<?php return ["channels" => ["test" => ["driver" => "single"]]];');

$method->invoke($provider, $tempPath, 'logging');

// Should set channels even with null existing config
expect(config('logging.channels'))->toBe(['test' => ['driver' => 'single']]);

unlink($tempPath);
}

public function test_service_provider_properties()
{
$provider = new MonitorServiceProvider($this->app);
Expand All @@ -323,25 +202,6 @@ public function test_service_provider_properties()
->and(get_class($provider))->toBe(MonitorServiceProvider::class);
}

public function test_merge_logging_channels_with_invalid_file_path()
{
$provider = new MonitorServiceProvider($this->app);
$reflection = new \ReflectionClass($provider);
$method = $reflection->getMethod('mergeLoggingChannelsFrom');
$method->setAccessible(true);

// This should test the error handling for non-existent files
// The method might fail gracefully or throw an exception
try {
$method->invoke($provider, '/non/existent/path.php', 'logging');
// If it doesn't throw, that's also a valid test result
expect(true)->toBeTrue();
} catch (\Exception $e) {
// If it throws, that's expected behavior for invalid paths
expect($e)->toBeInstanceOf(\Exception::class);
}
}

public function test_register_is_complete()
{
// Test the registration process itself by checking that register() method works
Expand All @@ -359,156 +219,5 @@ public function test_register_is_complete()
expect($monitor1)->toBe($monitor2)
->and($trace1)->toBe($trace2)
->and($timer1)->toBe($timer2);

// Verify config merging happened during registration
expect(config('logging.channels'))->toHaveKey('monitor');
}

public function test_merge_preserves_complex_channel_structures()
{
// Set up complex existing config with nested arrays
config([
'logging.channels.complex' => [
'driver' => 'stack',
'channels' => ['daily', 'slack'],
'processors' => ['web' => 'App\\Processors\\WebProcessor'],
'tap' => ['App\\Logging\\CustomizeFormatter'],
],
]);

$provider = new MonitorServiceProvider($this->app);
$provider->register();

$channels = config('logging.channels');

// Complex structure should be preserved exactly
expect($channels['complex'])->toBe([
'driver' => 'stack',
'channels' => ['daily', 'slack'],
'processors' => ['web' => 'App\\Processors\\WebProcessor'],
'tap' => ['App\\Logging\\CustomizeFormatter'],
]);
}

public function test_console_boot_conditions_comprehensive()
{
// Test all combinations of console auto-trace conditions
$testCases = [
['console' => true, 'testing' => false, 'started' => false, 'should_start' => true],
['console' => true, 'testing' => true, 'started' => false, 'should_start' => false],
['console' => false, 'testing' => false, 'started' => false, 'should_start' => false],
['console' => true, 'testing' => false, 'started' => true, 'should_start' => false],
];

foreach ($testCases as $case) {
$trace = new Trace;
if ($case['started']) {
$trace->start();
}

$this->app->instance(Trace::class, $trace);

// Create custom provider to test the exact boolean logic
$provider = new class($this->app) extends MonitorServiceProvider
{
public function test_console_logic(bool $console, bool $testing, bool $hasStarted): void
{
if ($console && ! $testing && ! $hasStarted) {
app(Trace::class)->start();
}
}
};

$originalStarted = $trace->hasStarted();
$provider->test_console_logic($case['console'], $case['testing'], $case['started']);

if ($case['should_start']) {
expect($trace->hasStarted())->toBeTrue(
'Trace should be started for case: '.json_encode($case)
);
} else {
expect($trace->hasStarted())->toBe($originalStarted,
'Trace state should be unchanged for case: '.json_encode($case)
);
}
}
}

public function test_console_auto_trace_executes_with_testing_enabled()
{
// Enable console auto-trace in testing environment
config(['monitor.console_auto_trace.enable_in_testing' => true]);

// Create a fresh trace that hasn't started
$trace = new Trace;
expect($trace->hasNotStarted())->toBeTrue();

// Bind to container
$this->app->instance(Trace::class, $trace);

// Create the actual service provider and boot it
$provider = new MonitorServiceProvider($this->app);
$provider->boot();

// The trace should now be started because we enabled it in testing
expect($trace->hasStarted())->toBeTrue();
}

public function test_console_auto_trace_respects_disabled_config()
{
// Disable console auto-trace entirely
config(['monitor.console_auto_trace.enabled' => false]);

// Create a fresh trace
$trace = new Trace;
expect($trace->hasNotStarted())->toBeTrue();

$this->app->instance(Trace::class, $trace);

// Boot the provider
$provider = new MonitorServiceProvider($this->app);
$provider->boot();

// Trace should NOT be started because auto-trace is disabled
expect($trace->hasNotStarted())->toBeTrue();
}

public function test_console_auto_trace_respects_already_started_trace()
{
// Enable auto-trace in testing
config(['monitor.console_auto_trace.enable_in_testing' => true]);

// Create and start a trace
$trace = new Trace;
$trace->start();
$originalId = $trace->id();

$this->app->instance(Trace::class, $trace);

// Boot the provider
$provider = new MonitorServiceProvider($this->app);
$provider->boot();

// Trace should remain the same (not restarted)
expect($trace->hasStarted())->toBeTrue()
->and($trace->id())->toBe($originalId);
}

public function test_console_auto_trace_config_defaults()
{
// Test that default config values work as expected
$trace = new Trace;
expect($trace->hasNotStarted())->toBeTrue();

$this->app->instance(Trace::class, $trace);

// Clear any existing config to test defaults
config(['monitor.console_auto_trace' => []]);

$provider = new MonitorServiceProvider($this->app);
$provider->boot();

// With defaults, auto-trace is enabled but not in testing
expect($trace->hasNotStarted())->toBeTrue();
}
}