Repository navigation
Warning: Trying to access array offset on value of type null #111
Description
When I try to view any article, I receive the following message:
"Warning: Trying to access array offset on value of type null in \plugins\attachments\framework\src\PlgAttachmentsFramework.php on line 966"
Environment:
- Joomla 5.2.3 with PHP 8.2.
- Attachments 4.1.0.
The line 966 in PlgAttachmentsFramework.php:
$attachments_list = $controller->displayString($parent_id, $this->parent_type, $parent_entity, null, true, true, false, $from, $attachment_id[$i] );
In an article without any attachments, the variable $attachment_id is null and $attachment_id[$i] cause this warning, because it isn't an array. This is source of this issue.
The proposal solution is modify the line 917 in \plugins\attachments\framework\src\PlgAttachmentsFramework.php
$attachment_id = null;
for this one:
$attachment_id [] = null;
After applying this change, the warning disappears.
Activity
mckillo commentedon Feb 5, 2025
I see that line 966 in PlgAttachmentsFramework.php has been patched in the master branch with:
$attachments_list = $controller->displayString($parent_id, $this->parent_type, $parent_entity, null, true, true, false, $from, $attachment_id[$i] ?? null);While this patch (?? null) prevents the warning, it is only a workaround if the variable is not properly declared.
I believe the better solution is to correctly initialize the variable by changing:
$attachment_id [] = null;and delete
?? nullat the end of line 966I'll send you a PR with the solution.
Proposal to Improve the Fix - Access array offset on value of type null
Proposal to Improve the Fix - Access array offset on value of type null
parapente commentedon Feb 5, 2025
@mckillo the displayString function expects either a null value or an array. I don't see the reason for changing $attachment_id to an array with a null value inside. The ?? operator helps to avoid doing a double check (if $attachment_id is an array and $attachment_id[$i] exists).
mckillo commentedon Feb 5, 2025
The last parameter of the displayString function is $attachment_id[$i]. If $attachment_id is defined as a variable instead of an array, attempting to access it as an array will trigger a warning when $attachment_id is null, as it is not an array.
It's not the same for a variable to be null as for an array to be null; the variable does not have [0] when $i is 0.
Additionally, in lines 932 and 935, the variable $attachment_id is used as an array.
I believe the variable should be an array, and declaring it correctly resolves the warning issue.
This warning appears when Joomla has Attachments 4.10 installed, and the user is viewing an article or blog.
It is a solution to the error found in version 4.10. The master branch is patched, but it is not the best solution (just a workaround).
parapente commentedon Feb 5, 2025
True
In lines 932 and 935 the code is appending to the array those values. However there is a difference between the case where the $attachmen_id variable is null and [null]. If the $attachment_id variable is null then the result will be an array with the content of those lines. If the $attachment_id variable is [null] and you meet line 935 then the result will be [null, null].
The patch was not a workaround, the $attachment_id variable is either null or an array with values. It used to be either null or an integer but we had to convert it to an array to allow for multiple {attachments id=xxx} tags. The array is the workaround in the specific case and as it is a rather complicated part of the extension I would prefer to avoid unnecessary changes that could break something that is already working.
If there is a use case where the ?? operator breaks something then an appropriate fix is always welcome.
mckillo commentedon Feb 5, 2025
I'm sorry, but I don't agree.
The
$attachment_idvariable is declared asnull:$attachment_id = null;and it cannot be an array containing
[null].The function insertAttachmentsList (line 887) is called to determine whether a Joomla article has attachments. Statistically, most articles do not have attachments. In such cases, the article content does not contain '{attachments', meaning it does not enter the while loop (line 919), and the variable does not change to an array type (line 935).
Therefore, the current declaration of this variable makes no sense. It should be an array so that it can be accessed as an array in line 965. It will always be an array at that point and will never be used as a simple variable.
Moreover, this is an issue in the 4.10 release—it exists and has already been distributed in the update. Users have this issue now. The patch only exists in the master branch and is not included in any released version. So, not fixing it does not prevent anything from breaking; on the contrary, it avoids an error caused by an incorrect declaration for the next release.
I’m trying to explain what I saw, but don’t worry, this was just an attempt to help fix the warning. You can reject the PR and save us both some time.
JLTRY commentedon Feb 6, 2025
@parapente I agree with @mckillo In php we do not have strict types. Nethertheless We should not change the type of a variable
parapente commentedon Feb 6, 2025
"$attachment_id[] =null" is [null]. That is what I said. You can inspect it using print_r if you don't believe me. The warning was found after version 4.1.0 was released and was fixed in PR #103 which will be included in v4.1.1. Thank you for taking the time and effort to report and make the PR but I will have to reject it for now. Feel free to reopen if there is an issue with the current implementation.
mckillo commentedon Feb 6, 2025
"$attachment_id[] =null" is [null]This is not the discussion nor the error. This is the solution that you deny (I didn't expect it, for me I don't have any logic).
The current code tries to read the first value ($i=0) of an array from a simple variable that is not an array.
I may not have explained myself well—I think you misunderstood me. Don't you see that the variable is incorrectly declared? The solution is quite simple.
Ok, thanks for you time, the details are importants.