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 "Trying to access array offset on null" warning #20294

Merged
merged 4 commits into from
Dec 9, 2024

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.

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
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