Fix for Undefined offset in Content History preview popup #12791
Conversation
| else | ||
| { | ||
| $valueText = null; | ||
| } |
There was a problem hiding this comment.
Can we add a clean line after the if?
What about removing the else completly?
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
how about:
$valueText = null;
if (is_array($optionFieldArray) && count($optionFieldArray))
{
$valueText = trim((string) $optionFieldArray[0]);
}
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
The code sniffer doesn't detect all the style issues. There is supposed to be a line break after an if/else block.
| if (false === $this->item) | ||
| { | ||
| $language = JFactory::getLanguage(); | ||
| $language->load('com_content', JPATH_SITE, null, true); |
There was a problem hiding this comment.
Can we remover the language var here?
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Honestly, we shouldn't be importing strings from com_content here (especially as history can be used with more than articles). But for some asinine reason there isn't a generic "Item not found" string in the global language files.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
I have tested this item ✅ successfully on 3a466a2 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12791. |
|
I have tested this item ✅ successfully on 3a466a2 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12791. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12791. |
These are all reasons why I'm trying to get a PHPCS2 version of our code standards tested and verified so we have automatic fixers to correct most of the general code style issues. |
* staging: (98 commits) Coding style. PHP constants true, false, and null MUST be in lower case. (joomla#13010) Removing duplicated AS in sql query (joomla#13006) Fixed typo in comment (joomla#12992) Correcting strings in TFA Google plugin (joomla#12980) code style changes (joomla#12986) Error in sr-YU installation ini file (joomla#12984) New DateTime picker (replaces calendar) (joomla#11138) Export of Banners Tracks Does Not Export the Banner Name fix rues get data (joomla#12763) Added Feature items filter to mod_articles_news (joomla#12547) fix them all (joomla#12943) a11y regression fix (joomla#12935) Set correct component id for system links (joomla#12938) Fix for Undefined offset in Content History preview popup (joomla#12791) remove tab on meta charset (joomla#12895) JSession patched to set session _state to 'inactive' when session is closed. (joomla#12928) [JHtmlNumber::bytes] Format number according to language (joomla#12929) Update edit.php (joomla#12818) Update default.xml (joomla#12917) Adding the ability to use the global value for character count in newsfeeds (joomla#12869) ...
Set error reporting to E_ALL (Like it should be for development)
Article Manager -> Edit Getting Started -> Versions -> Select a version -> popup url is like:
http://example.com/administrator/index.php?option=com_contenthistory&view=preview&layout=preview&tmpl=component&b05a6963679e395c78f4b883e2c00b56=1&version_id=2
repeated errors on page of
Notice: Undefined offset: 0 in example.com/administrator/components/com_contenthistory/helpers/contenthistory.php on line 123Summary of Changes
Better checking of the array before attempting to access its members
Testing Instructions
Do above - replicate Notices, apply patch, see they disappear...
Documentation Changes Required
None
EDIT
So like a prat I committed my next fix to this branch too... so this PR now contains that too.
The problem that solves is that if you fake a non-existing version ID in the popup with a url like:
http://127.0.0.1:8000/administrator/index.php?option=com_contenthistory&view=preview&layout=preview&tmpl=component&b05a6963679e395c78f4b883e2c00b56=1&version_id=212312Then you get
This PR also fixes that now...