Skip to content

Conversation

W0rma
Copy link
Contributor

@W0rma W0rma commented May 20, 2022

Fixes #1318

@W0rma W0rma marked this pull request as ready for review May 20, 2022 09:02
@W0rma W0rma force-pushed the monolog3 branch 4 times, most recently from b66cc7f to aca1945 Compare May 20, 2022 18:39
@W0rma
Copy link
Contributor Author

W0rma commented May 20, 2022

I'm a bit unsure on how to fix the phpstan and psalm pipeline.

psalm seems to analyse the code based on PHP 7.2 and I don't know how to change that.

The phpstan errors are related to the BC layer which makes sure that the handler can be used in monolog v2 and v3. Should I add the reported "errors" to the baseline file?

@ste93cry ste93cry added this to the 3.6 milestone May 20, 2022
Copy link
Contributor

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. Since this is not a bug fix, please target the develop branch.

psalm seems to analyse the code based on PHP 7.2 and I don't know how to change that.

Technically speaking, it's correct that Psalm complains because the code should run on any PHP version down to 7.2. I think that the only viable solution is to update the baseline and ignore the errors. A more appropriate way could be to run the analysis more than once, one for PHP 7.2 and one for PHP 8.0 though, but it would be a bigger change and I wouldn't tackle it here

The phpstan errors are related to the BC layer which makes sure that the handler can be used in monolog v2 and v3. Should I add the reported "errors" to the baseline file?

The error is because PHPStan does not understand that the Logger::API constant can change depending on which version of the package is installed. It should be enough to instruct it that the constant is dynamic by using the dynamicConstantNames option.

@W0rma W0rma changed the base branch from master to develop May 23, 2022 05:12
@W0rma
Copy link
Contributor Author

W0rma commented May 23, 2022

@ste93cry Thank you for the hints! I have changed the target branch to develop and addressed all your suggestions.

@W0rma W0rma force-pushed the monolog3 branch 2 times, most recently from 977d64f to 850b0e4 Compare May 23, 2022 08:48
Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Good work! Please add a changelog entry.

@W0rma
Copy link
Contributor Author

W0rma commented May 23, 2022

Please add a changelog entry.

@Jean85 Done

@W0rma W0rma requested review from Jean85 and ste93cry May 23, 2022 12:50
Copy link
Contributor

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

I've left a few more comments, but they are just nitpick. You should also rebase the PR to solve the issues with PHPStan errors. The only concern I have, but to which I don't think there is solution that can be easily taken here, is that it looks weird to see errors related to the vendor folder in the Psalm baseline 😞

Copy link
Contributor

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

Thank you for the hard work!

@ste93cry ste93cry merged commit 5f9c011 into getsentry:develop May 27, 2022
@W0rma W0rma deleted the monolog3 branch May 27, 2022 08:04
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.

Monolog handler is not compatible with monolog v3
3 participants