-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Updated monolog/monolog to 2.3.1 #33497
Updated monolog/monolog to 2.3.1 #33497
Conversation
Hi @andrewbess. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Static Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Unit Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
ab6ef75
to
249d587
Compare
@magento run Static Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
if ($configValue === null) { | ||
$isEnabled = $this->state->getMode() !== State::MODE_PRODUCTION; | ||
} else { | ||
$isEnabled = (bool)$configValue; | ||
$isEnabled = (bool) $configValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$isEnabled = (bool) $configValue; | |
$isEnabled = (bool)$configValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ihor-sviziev.
Thank you for your suggestion, but space symbol is exist in examples from PSR-12. Also codesniffer proposes to have space symbol here.
Thank you again )))
public function addRecord( | ||
int $level, | ||
string $message, | ||
array $context = [] | ||
): bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not spend space vertically, when it's not needed. Before it looked much better.
public function addRecord( | |
int $level, | |
string $message, | |
array $context = [] | |
): bool { | |
public function addRecord(int $level, string $message, array $context = []): bool | |
{ |
public function __construct( | ||
$name, | ||
array $handlers = [], | ||
array $processors = [] | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function __construct( | |
$name, | |
array $handlers = [], | |
array $processors = [] | |
) { | |
public function __construct($name, array $handlers = [], array $processors = [] ) | |
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ihor-sviziev
Thank you for your suggestion.
I strive for perfection and try to write my code according to PSR-12 (p.2.3).
This line contains more than 80 symbols.
throw new InvalidArgumentException( | ||
'Filename expected to be a string' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new InvalidArgumentException( | |
'Filename expected to be a string' | |
); | |
throw new InvalidArgumentException('Filename expected to be a string'); |
use Magento\Framework\Filesystem\DriverInterface; | ||
use Magento\Framework\Logger\Handler\Exception as ExceptionHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to name it HandlerException, as ExceptionHandler means that it will handle some exception(s).
use Magento\Framework\Logger\Handler\Exception as ExceptionHandler; | |
use Magento\Framework\Logger\Handler\Exception as HandlerException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ihor-sviziev
Thank you for your suggestion.
The property of this class has already been named $exceptionHandler
. Why do we need to change the style of the alias?
Also, the style of alias naming like xxxxxHandler
has already existed in Magento.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ihor-sviziev I think ExceptionHandler is the better name, as this class is a handler, not an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sivaschenko @andrewbess it definitely make sense. From the class name I just thought it’s opposite
$message = $message instanceof Exception | ||
? $message->getMessage() | ||
: $message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$message = $message instanceof Exception | |
? $message->getMessage() | |
: $message; | |
$message = $message instanceof Exception ? $message->getMessage() : $message; |
$this->exceptionHandlerMock = $this->getMockBuilder( | ||
ExceptionHandler::class | ||
)->disableOriginalConstructor()->getMock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace it with the following?
$this->exceptionHandlerMock = $this->getMockBuilder( | |
ExceptionHandler::class | |
)->disableOriginalConstructor()->getMock(); | |
$this->exceptionHandlerMock = $this->createMock(ExceptionHandler::class); |
$this->filesystemMock = $this->getMockBuilder(DriverInterface::class) | ||
->getMockForAbstractClass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->filesystemMock = $this->getMockBuilder(DriverInterface::class) | |
->getMockForAbstractClass(); | |
$this->filesystemMock = $this->createMock(DriverInterface::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ihor-sviziev
Thank you for your notice.
I fixed it.
'datetime' => DateTime::createFromFormat( | ||
'U.u', | ||
sprintf('%.6F', microtime(true)) | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'datetime' => DateTime::createFromFormat( | |
'U.u', | |
sprintf('%.6F', microtime(true)) | |
), | |
'datetime' => DateTime::createFromFormat('U.u', sprintf('%.6F', microtime(true))), |
int $level = Logger::WARNING, | ||
string $message = 'test', | ||
array $context = [] | ||
):array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
):array { | |
): array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ihor-sviziev
Thank you for your notice.
I fixed it.
@magento run Unit Tests, WebAPI Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
1 similar comment
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
fc32670
to
4eca2e2
Compare
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
Hi @andrewbess, thank you for your contribution! |
Description (*)
This PR (pull request) updates the monolog/monolog library to the latest version 2.3.1.
Also, it fixes the codebase to take compatibility after updating the library.
Related Pull Requests
https://github.com/magento/partners-magento2ee/pull/561
Fixed Issues (if relevant)
Questions or comments
Contribution checklist (*)