Skip to content

Conversation

@xcopy
Copy link
Contributor

@xcopy xcopy commented Dec 8, 2024

Q A
Is bugfix? ✔️
New feature?
Breaks BC?

Note: this does not solve the whole problem described in the issue #17365, but only part of it.

@codecov
Copy link

codecov bot commented Dec 8, 2024

Codecov Report

Attention: Patch coverage is 18.75000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 63.42%. Comparing base (49cfd3b) to head (52ff9db).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
framework/caching/FileCache.php 25.00% 9 Missing ⚠️
framework/log/FileTarget.php 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #20294      +/-   ##
============================================
- Coverage     64.84%   63.42%   -1.43%     
- Complexity    11412    11417       +5     
============================================
  Files           431      431              
  Lines         37147    37155       +8     
============================================
- Hits          24088    23565     -523     
- Misses        13059    13590     +531     

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

@xcopy
Copy link
Contributor Author

xcopy commented Dec 8, 2024

Btw, can find more similar code-block, i.e.

$error = error_get_last();
Yii::warning("...: {$error['message']}", __METHOD__);

and I guess the same kind of problem might arise with them.

However, I like the approach in the class yii\web\Session:

$error = error_get_last();
$message = isset($error['message']) ? $error['message'] : 'Failed to start session.';
Yii::error($message, __METHOD__);

@samdark samdark added this to the 2.0.52 milestone Dec 8, 2024
@samdark samdark added the type:bug Bug label Dec 8, 2024
Copy link
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

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

👍 Would you please add a line for CHANGELOG? Thanks.

@xcopy
Copy link
Contributor Author

xcopy commented Dec 8, 2024

Btw, can find more similar code-block, i.e.

$error = error_get_last();
Yii::warning("...: {$error['message']}", __METHOD__);

and I guess the same kind of problem might arise with them.

However, I like the approach in the class yii\web\Session:

$error = error_get_last();
$message = isset($error['message']) ? $error['message'] : 'Failed to start session.';
Yii::error($message, __METHOD__);

@samdark how about this one? Should fix 'em all?

@xcopy
Copy link
Contributor Author

xcopy commented Dec 8, 2024

👍 Would you please add a line for CHANGELOG? Thanks.

Done ✅

@samdark
Copy link
Member

samdark commented Dec 8, 2024

Worth fixing all these if you're sure the issue could occur.

@samdark samdark requested review from a team December 8, 2024 13:52
@bizley
Copy link
Member

bizley commented Dec 9, 2024

LGTM although I prefer classic ifs.

@xcopy
Copy link
Contributor Author

xcopy commented Dec 9, 2024

@mtangoo I was thinking about it. Well, there is no standard: somewhere I can see throw new xxx(Yii::t(...)), somewhere just throw new xxx(...).

I can start a separate dedicated PR.

@xcopy
Copy link
Contributor Author

xcopy commented Dec 9, 2024

LGTM although I prefer classic ifs.

Done ✅

@mtangoo
Copy link
Contributor

mtangoo commented Dec 9, 2024

I think we are good to go. @xcopy thanks for your time and effort. If you feel you can tackle another PR, feel free to do so!

@mtangoo mtangoo merged commit 4ea0575 into yiisoft:master Dec 9, 2024
33 of 87 checks passed
@xcopy xcopy deleted the xcopy-patch-1 branch December 9, 2024 15:05
terabytesoftw pushed a commit to terabytesoftw/yii2 that referenced this pull request Apr 25, 2025
terabytesoftw pushed a commit to terabytesoftw/yii2 that referenced this pull request Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants