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

Move Monolog to dev #660

Closed
wants to merge 1 commit into from
Closed

Move Monolog to dev #660

wants to merge 1 commit into from

Conversation

mostafasoufi
Copy link
Contributor

@mostafasoufi mostafasoufi commented Jan 8, 2024

Regarding PR #465, why isn't MonoLog included in the development environment when it's used in the Commands classes for Unit tests?

Best

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (003b4d2) 36.26% compared to head (184d27d) 36.26%.

Additional details and impacted files
@@            Coverage Diff            @@
##              7.5.x     #660   +/-   ##
=========================================
  Coverage     36.26%   36.26%           
  Complexity      286      286           
=========================================
  Files            27       27           
  Lines          1183     1183           
=========================================
  Hits            429      429           
  Misses          754      754           
Flag Coverage Δ
phpunit 36.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mimmi20
Copy link
Member

mimmi20 commented Jan 8, 2024

@mostafasoufi Monolog is used in the File src/Helper/LoggerHelper.php. This Helper is used in all Commands, not just in the Tests. If you want to make Monolog optional, you have to change that file too, and maybe all places where this Helper is used.

@mostafasoufi
Copy link
Contributor Author

They are only uses in /bin/browscap-php which is not the main functionality of Browscap, isn't it?

@mimmi20 mimmi20 requested review from asgrim and mimmi20 January 8, 2024 19:01
@mimmi20
Copy link
Member

mimmi20 commented Jan 8, 2024

Browscap can be used in two ways:

  1. as Component for other projects, where it is installed via composer
  2. as Standalone Project

In the second Way, you can use these Commands. I do not know, if anyone uses Browscap this way.

If Monolog is transfered to the dev-section, these Commands will break. This means that will be a breaking change.

Copy link
Member

@mimmi20 mimmi20 left a comment

Choose a reason for hiding this comment

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

Please add also an Comment in the Readme (where these Commands are listed), that the User has to install Monolog.

@@ -43,6 +42,7 @@
"require-dev": {
"doctrine/coding-standard": "^12.0.0",
"mikey179/vfsstream": "^1.6.11",
"monolog/monolog": "^3.5.0",
Copy link
Member

Choose a reason for hiding this comment

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

Please add "monolog/monolog" also in the suggest section with an comment, that this is required for the console commands.

@asgrim
Copy link
Member

asgrim commented Jan 8, 2024

I would suggest this is not mergable at least until the next major, since it would indeed break this functionality. It would help if we knew if people use this functionality or not 🤷

@asgrim asgrim self-assigned this Jan 8, 2024
@asgrim asgrim added the wontfix label Jan 8, 2024
@asgrim
Copy link
Member

asgrim commented Jan 8, 2024

@mostafasoufi thank you for the PR, but since this is functionality people may use, which would break, this wouldn't be acceptable until a major release really. I'd suggest making an issue to see if others have feedback about it and we can discuss what the migration path might look like. Thank you though, we really appreciate it!

@asgrim asgrim closed this Jan 8, 2024
@mostafasoufi
Copy link
Contributor Author

I inquired because I'm planning to upgrade Browscap for WordPress plugins like WP Statistics and SlimStat Analytics. However, since the pull request (PR) is closed, it looks like I'll need to fork the project and work on my own repository.

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.

4 participants