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

Fix call to logger warn method. #13252

Merged
merged 2 commits into from Jan 22, 2024
Merged

Fix call to logger warn method. #13252

merged 2 commits into from Jan 22, 2024

Conversation

AdamBark
Copy link

warn is not a method, warning appears to be correct.

Q A
Bug fix? (use the a.b branch) [yes ]
New feature/enhancement? (use the a.x branch) [ no]
Deprecations? [ None]
BC breaks? (use the c.x branch) [ None?]
Automated tests included? [ not applicable]

Description:

When running bin/console mautic:iplookup:download the following error is thrown without this bugfix if you have no credentials for the Maxmind service.

                                                                                                
  [Symfony\Component\ErrorHandler\Error\UndefinedMethodError]                                   
  Attempted to call an undefined method named "warn" of class "Symfony\Bridge\Monolog\Logger".  
  Did you mean to call "warning"?

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Run php bin/console mautic:iplookup:download

kuzmany
kuzmany previously approved these changes Jan 19, 2024
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Code looks good 🫰

@kuzmany kuzmany added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Jan 19, 2024
Copy link
Contributor

@mollux mollux left a comment

Choose a reason for hiding this comment

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

@AdamBark while your change fixes that specific error on that specific place, there are 3 other occurrences around that code that use the same non-existing method.
Can you please address those too?

Screenshot 2024-01-20 at 10 16 40

@mollux mollux added this to the 5.0.3 milestone Jan 20, 2024
Copy link

codecov bot commented Jan 20, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (bee6b30) 58.60% compared to head (45ec17c) 58.60%.
Report is 4 commits behind head on 5.0.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.0   #13252      +/-   ##
============================================
- Coverage     58.60%   58.60%   -0.01%     
  Complexity    32985    32985              
============================================
  Files          2183     2183              
  Lines         98756    98756              
============================================
- Hits          57877    57876       -1     
- Misses        40879    40880       +1     
Files Coverage Δ
...ndles/CoreBundle/IpLookup/IP2LocationAPILookup.php 0.00% <0.00%> (ø)
...dles/CoreBundle/IpLookup/MaxmindDownloadLookup.php 21.21% <0.00%> (ø)
...ndles/CoreBundle/IpLookup/IP2LocationBinLookup.php 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Jan 20, 2024
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Good 👍

Copy link
Contributor

@mollux mollux left a comment

Choose a reason for hiding this comment

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

Thanks for the additional changes!

@mollux mollux changed the base branch from 5.x to 5.0 January 21, 2024 13:32
@mollux
Copy link
Contributor

mollux commented Jan 21, 2024

@AdamBark I just noticed you made the PR against 5.x, and it should be 5.0, as it's a bugfix.

Can you rebase your changes based on 5.0? then this PR can be merged

@AdamBark
Copy link
Author

@AdamBark I just noticed you made the PR against 5.x, and it should be 5.0, as it's a bugfix.

Can you rebase your changes based on 5.0? then this PR can be merged

Will do. I was a bit confused by the various documentations on which branch it should be on so thanks for the clarification. Perhaps this line "All PRs are made against the c.x branch in the first instance (e.g. 5.x)" should be amended?

warn is not a method, warning appears to be correct.
@mollux mollux merged commit c08052b into mautic:5.0 Jan 22, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs pending-feedback PR's and issues that are awaiting feedback from the author pending-test-confirmation PR's that require one test before they can be merged
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants