Skip to content

issue77#78

Merged
arhimede merged 4 commits intodotkernel:5.0from
Jurj-Bogdan:issue77
Feb 24, 2025
Merged

issue77#78
arhimede merged 4 commits intodotkernel:5.0from
Jurj-Bogdan:issue77

Conversation

@Jurj-Bogdan
Copy link
Copy Markdown
Member

Removed src/Exception/InvalidArgumentException.php and replaced it's usage with Psr\Log\InvalidArgumentException.

Closes #77

Signed-off-by: Jurj-Bogdan <bogdanjurj11@gmail.com>
@arhimede arhimede requested a review from alexmerlin February 21, 2025 13:12
@Jurj-Bogdan
Copy link
Copy Markdown
Member Author

Jurj-Bogdan commented Feb 21, 2025

To adhere to PSR-3, only the log method from Logger.php must throw Psr\Log\InvalidArgumentException, but it felt confusing to keep the older one as well. Let me know if that would've been preferable though

Copy link
Copy Markdown
Member

@alexmerlin alexmerlin left a comment

Choose a reason for hiding this comment

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

IMO, I would keep src/Exception/InvalidArgumentException.php, because this was the purpose of having our own version of an exception: we can introduce modifications in the package, but the implementors do not need to adapt their code, they can continue using Dot\Log\Exception\InvalidArgumentException.
Keeping that file, you can undo almost the entire PR and you'll probably have end up having one single file modified.

This reverts commit af1076c.
Signed-off-by: Jurj-Bogdan <bogdanjurj11@gmail.com>
Copy link
Copy Markdown
Member

@alexmerlin alexmerlin left a comment

Choose a reason for hiding this comment

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

Also, modify src/Exception/InvalidArgumentException.php:

<?php

declare(strict_types=1);

namespace Dot\Log\Exception;

class InvalidArgumentException extends \Psr\Log\InvalidArgumentException
{
}

This is what I meant in my previous review.
Now that Dot\Log\Exception\InvalidArgumentException extends Psr\Log\InvalidArgumentException, which extends \InvalidArgumentException we can use our own InvalidArgumentException everywhere because it is an instance of all the exteded classes so it will be accepted everywhere.

Comment thread src/Logger.php Outdated
Comment thread src/Logger.php Outdated
Comment thread test/LoggerTest.php Outdated
Comment thread test/LoggerTest.php
Signed-off-by: Jurj-Bogdan <bogdanjurj11@gmail.com>
@alexmerlin
Copy link
Copy Markdown
Member

As I mentioned in my first review:

Keeping that file, you can undo almost the entire PR and you'll probably have end up having one single file modified.

👍

@arhimede arhimede merged commit e8f2a06 into dotkernel:5.0 Feb 24, 2025
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.

Replace Dot\Log\Exception\InvalidArgumentException

3 participants