Skip to content

Conversation

@facundocapua
Copy link
Contributor

I have added a new optional parameter so it is possible to define virtual types for the Base logger handler.
With this change now it is possible to do something like this:

<virtualType name="MyCustomLog" type="Magento\Framework\Logger\Handler\Base">
    <arguments>
        <argument name="fileName" xsi:type="string">/var/log/custom.log</argument>
    </arguments>
</virtualType>

I have also tried to modify the System and Debug handler implementation so it can be added in the di.xml. However, there is some information that is logged before di is resolved.

Without this change, adding a custom log implies creating a new handler class just to define the file.

Let me know if something needs further clarification

@MauroNigrele
Copy link

Agree! Log some data shouldn't be a pain in thr back, this feature would help.

@Vinai
Copy link
Contributor

Vinai commented Dec 15, 2015

Thank you for the PR! We are currently waiting for the travis builds to work again, which should happen in a few days. Once that is running and the tests are green we can continue to process your contribution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be the other way around: if (!empty($filename)) { $this->fileName = $fileName }. If I pass a new file path to the class, I'm expecting its state to change, as well, but you're not changing $this->fileName at all.

@adragus-inviqa
Copy link
Contributor

What about sanitization - passing everything but a string? What about passing something like ../../../../../hack_log.log as the file name? What if the passed path is /some/path/ and the name is /debug.log - that would end up as /some/path//debug.log wouldn't it? Some tests would be nice, too.

@vancoz
Copy link

vancoz commented Jan 12, 2016

@fcapua-summa could you plase address comments from code review?
Also place pull latests changes (we have fixed travis build).
+1 to @adragus-inviqa regarding sanitation and tests.

Thank you!

@mazhalai
Copy link
Contributor

@fcapua-summa we are waiting for you to address the comments above and also please pull latest code and re run builds.

@sshrewz
Copy link

sshrewz commented Mar 18, 2016

@fcapua-summa, since we have not heard from you for quite some time, we will go ahead and close this pull request. However, feel free to reopen once you have update for us.

@facundocapua
Copy link
Contributor Author

Sorry for the lack of response, I've been a little bit off in the past months.
I have applied the changes according to the suggestions.

Please let me know if there is something else to modify.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants