-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
define $options as an array if its not already an array - the code is expecting an array. #15641
define $options as an array if its not already an array - the code is expecting an array. #15641
Conversation
if (!is_array($options)) | ||
{ | ||
$options = array(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding stylewise, I think you need to add a return after }
.
I don't know whether adding a return directly after } is a safe thing to do - there are lines below it (624-627) that may still need to be executed. My amend is simply ensuring that $options is an Array before it gets to the code below it so that the warnings about it not being an array are suppressed. I don't know if there are any knock on consequences of this amend - I just know that in my installation it was the only way that I found to suppress the frontend warnings without breaking the admin area as well. |
We need a proper test case for this and not just using review here. How is |
I also like Michael think we should find where the warning origins, because apparently there is code somewhere which calls that method wrong. |
I'm getting this issue appearing following a 3.6.5 to 3.7.1 upgrade. |
a) submit a pr
|
Yep, that would be very interesting if you can get that information. |
OK. So I've traced the issue to three calls to style sheets: 2. tooltips/style.min.css $options was empty 3. $file was empty but $options contained: "templates/system/css/system.css" |
OK now found source for 3 is JA Elastica Template: file /templates/ja_elastica/blocks/head.php lines #32 and 33 Bug report here: https://www.joomlart.com/forums/topic/joomla-3-7-update-status/page/8/#post-1035193 So I don't think there is a fault in the core calling the function, but there is clearly a number of "dodgy" calls in older code. SO I think putting the fix in would make sense. But with a depreciation error |
I have tested this item ✅ successfully on b34f620 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15641. |
I tend to say this PR isn't something we should do, as it is only a bandaid for faulty code in 3rd party extensions. It should be fixed in the respective extensions. If we're going to do something like this, then it has to be deprecated right away with at least a log message. But then, the current warning is fine as the code still works and the extension developer gets notified of his faulty code right away. On productive servers, those warnings should be disabled anyway. |
Here is a call from Joomlart's Elastica template in Joomla! 3.4.8: |
@Quy That one is actually fine and still works. It has been deprecated with J3.7.0 and will not supported anymore with J4.0. Currently it logs a deprecation message if you use it that way. |
set Status on "Needs Review" cause of above Comment. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15641. |
Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/15641 |
closed as stated above. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15641. |
Many months later I actually discovered that it was the Joooid plugin that http://www.joooid.com/ that caused this issue. Once disabled the warnings went away. It may have been updated now but if anyone sees this issue again, try disabling the Joooid plugin. |
Pull Request relating to Issue #15548 and #15600.
'stylesheet' function in /libraries/cms/html/html.php takes an $options which may or may not be an array. After upgrading to Joomla 3.7 I was seeing 'Cannot use a scalar value warning' warnings displayed in the frontend website relating to lines 620, 621, 622 and 623. There have been some changes to this function in the last couple of days - I don't know whether there has been an unintended consequence as a result.
Summary of Changes
In the section that produced warnings, the $options variable is expected to be an array. I've added a check to say that if it's not an array then define it as an empty array. If it is already an array then there is no impact. The code is expecting it to be an array.
Testing Instructions
Just tested in the browser. No automated tests added.
Expected result
No 'Cannot use a scalar value warning' warnings
Actual result
Documentation Changes Required
None.