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

Fixes #2725 : move logging directory path config to CodeIgniter\Log\Handlers\FileHandler config #2726

Merged
merged 10 commits into from
Mar 22, 2020

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Mar 20, 2020

Fixes #2725 . The Logger::$path config never get as it tried to access the path config in "CodeIgniter\Log\Handlers\FileHandler" config under Logger::$handlers property. Also, the ?? parameter make test for nullable/not isset, while it check the empty string value. Now, to avoid BC break, I use the following flow:

note: $config is a configuration under Logger::$handlers['CodeIgniter\Log\Handlers\FileHandler'], not Logger class properties

  • set $config['path'] to $config['path'] if isset, fallback to WRITEPATH . 'logs/';
  • $this->path to $config['path'] if not empty string, fallback to WRITEPATH . 'logs/';

Checklist:

  • Securely signed commits
  • Unit testing, with >80% coverage

@samsonasik
Copy link
Member Author

Got error in test : "fopen(vfs://root/log-2020-03-20.log): failed to open stream: "org\bovigo\vfs\vfsStreamWrapper::stream_open" call failed". It seems the vfsStream() put -> read requires tweak.

@samsonasik
Copy link
Member Author

The test has been tweaked for writing file with vfsStream

@samsonasik
Copy link
Member Author

travis build green 🎉

@MGatner
Copy link
Member

MGatner commented Mar 20, 2020

This looks good to me. There should be no need for path in the base config since it is handled-specific. Since it’s a change to app I’d like @lonnieezell to take a look before merging. Also @dafriend has spent a lot of time with logging recently and may have some input.
Thanks for the work, these have been very solid PRs.

@MGatner MGatner requested a review from lonnieezell March 20, 2020 13:21
app/Config/Logger.php Outdated Show resolved Hide resolved
@samsonasik
Copy link
Member Author

I close re-open the PR to re-trigger travis build

@samsonasik
Copy link
Member Author

@dafriend I applied your suggestion, travis build green 🎉

@lonnieezell
Copy link
Member

I think this change makes sense. My initial reaction was that it was a BC change that would cause a new MAJOR release, which we don't want to do. But I think another PR just removed that anyway, since it wasn't being used, and this doesn't affect the API so I think we're OK. Will merge.

@lonnieezell lonnieezell merged commit dbc06d3 into codeigniter4:develop Mar 22, 2020
@samsonasik samsonasik deleted the fix-2725 branch March 22, 2020 02:50
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.

Bug: Logger path property ignored
4 participants