Skip to content

Type safe comparison in libraries/cms - second iteration #13276

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jun 13, 2017
Merged

Type safe comparison in libraries/cms - second iteration #13276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
579bacd
### Summary of Changes
frankmayer Dec 19, 2016
06b1743
Merge branch 'staging' into tye-safety-in-libraries-cms-2
frankmayer Apr 26, 2017
15ff27e
Merge branch 'staging' into tye-safety-in-libraries-cms-2
rdeutz May 27, 2017
15a6212
Merge branch 'staging' into tye-safety-in-libraries-cms-2
frankmayer May 29, 2017
76a417a
Merge remote-tracking branch 'origin/tye-safety-in-libraries-cms-2' i…
frankmayer May 29, 2017
6bca581
Merge branch 'staging' into tye-safety-in-libraries-cms-2
frankmayer May 29, 2017
8e5d494
Changes according to reviewer's comments + more type safety + some co…
frankmayer Jun 12, 2017
f5fe9bf
Merge branch 'staging' into tye-safety-in-libraries-cms-2
frankmayer Jun 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libraries/cms/form/rule/password.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public function test(SimpleXMLElement $element, $value, $group = null, Registry
// Set a variable to check if any errors are made in password
$validPassword = true;

if (strlen($valueTrim) != $valueLength)
if (strlen($valueTrim) !== $valueLength)
{
JFactory::getApplication()->enqueueMessage(
JText::_('COM_USERS_MSG_SPACES_IN_PASSWORD'),
Expand Down
8 changes: 4 additions & 4 deletions libraries/cms/html/html.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ protected static function extract($key)
// Check to see whether we need to load a helper file
$parts = explode('.', $key);

$prefix = (count($parts) == 3 ? array_shift($parts) : 'JHtml');
$file = (count($parts) == 2 ? array_shift($parts) : '');
$func = array_shift($parts);
$prefix = count($parts) === 3 ? array_shift($parts) : 'JHtml';
$file = count($parts) === 2 ? array_shift($parts) : '';
$func = array_shift($parts);

return array(strtolower($prefix . '.' . $file . '.' . $func), $prefix, $file, $func);
}
Expand All @@ -97,7 +97,7 @@ public static function _($key)
if (array_key_exists($key, static::$registry))
{
$function = static::$registry[$key];
$args = func_get_args();
$args = func_get_args();

// Remove function name from arguments
array_shift($args);
Expand Down
4 changes: 2 additions & 2 deletions libraries/cms/html/select.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public static function genericlist($data, $name, $attribs = null, $optKey = 'val
// Set default options
$options = array_merge(JHtml::$formatOptions, array('format.depth' => 0, 'id' => false));

if (is_array($attribs) && func_num_args() == 3)
if (is_array($attribs) && func_num_args() === 3)
{
// Assume we have an options array
$options = array_merge($options, $attribs);
Expand Down Expand Up @@ -354,7 +354,7 @@ public static function integerlist($start, $end, $inc, $name, $attribs = null, $
// Set default options
$options = array_merge(JHtml::$formatOptions, array('format.depth' => 0, 'option.format' => '', 'id' => null));

if (is_array($attribs) && func_num_args() == 5)
if (is_array($attribs) && func_num_args() === 5)
{
// Assume we have an options array
$options = array_merge($options, $attribs);
Expand Down
10 changes: 5 additions & 5 deletions libraries/cms/html/string.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ abstract class JHtmlString
public static function truncate($text, $length = 0, $noSplit = true, $allowHtml = true)
{
// Assume a lone open tag is invalid HTML.
if ($length == 1 && substr($text, 0, 1) == '<')
if ($length === 1 && substr($text, 0, 1) === '<')
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure lenght is an int?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks to not having the ability to do scalar typehinting, no, we aren't. That said, any dev passing a string into a method declaring a parameter as an integer is just asking for trouble as newer PHP releases come out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I agree. Still to keep b/c maybe is better to (int) the var....

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a B/C break if the code enforces correct typing of a parameter. Otherwise either we bump to PHP 7 and typehint everything or remove all typehints and we're stuck running gettype() and is_a() or instanceof checks on everything because the documentation is unreliable.

Sooner or later the mentality of "well just pass anything in any ol' format" has to be broken. There are far too many issues dealing with something like 0 == '0' == false == null.

Copy link
Contributor

@andrepereiradasilva andrepereiradasilva Dec 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

OK. If is not considered a b/c break by the project fine. Just raising the question. For the rest obvisualy i agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree with @mbabker. 👍

{
return '...';
}
Expand Down Expand Up @@ -105,7 +105,7 @@ public static function truncate($text, $length = 0, $noSplit = true, $allowHtml
$numOpened = count($openedTags);

// Not all tags are closed so trim the text and finish.
if (count($closedTags) != $numOpened)
if (count($closedTags) !== $numOpened)
{
// Closing tags need to be in the reverse order of opening tags.
$openedTags = array_reverse($openedTags);
Expand Down Expand Up @@ -170,7 +170,7 @@ public static function truncateComplex($html, $maxLength = 0, $noSplit = true)
$baseLength = strlen($html);

// If the original HTML string is shorter than the $maxLength do nothing and return that.
if ($baseLength <= $maxLength || $maxLength == 0)
if ($baseLength <= $maxLength || $maxLength === 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure maxlenght is an int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving as is, because the variable is defined as a parameter to the function, with type integer.

{
return $html;
}
Expand All @@ -182,7 +182,7 @@ public static function truncateComplex($html, $maxLength = 0, $noSplit = true)
}

// Deal with maximum length of 1 where the string starts with a tag.
if ($maxLength == 1 && substr($html, 0, 1) == '<')
if ($maxLength === 1 && substr($html, 0, 1) === '<')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving as is, because the variable is defined as a parameter to the function, with type integer.

{
$endTagPos = strlen(strstr($html, '>', true));
$tag = substr($html, 1, $endTagPos);
Expand All @@ -204,7 +204,7 @@ public static function truncateComplex($html, $maxLength = 0, $noSplit = true)
$ptString = JHtml::_('string.truncate', $html, $maxLength, $noSplit, $allowHtml = false);

// It's all HTML, just return it.
if (strlen($ptString) == 0)
if (strlen($ptString) === 0)
{
return $html;
}
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/installer/helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ public static function unpack($p_filename, $alwaysReturnArray = false)
*/
$dirList = array_merge((array) JFolder::files($extractdir, ''), (array) JFolder::folders($extractdir, ''));

if (count($dirList) == 1)
if (count($dirList) === 1)
{
if (JFolder::exists($extractdir . '/' . $dirList[0]))
{
Expand Down
8 changes: 4 additions & 4 deletions libraries/cms/installer/installer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ public function parseQueries(SimpleXMLElement $element)
// Get the array of query nodes to process
$queries = $element->children();

if (count($queries) == 0)
if (count($queries) === 0)
{
// No queries to process
return 0;
Expand Down Expand Up @@ -987,7 +987,7 @@ public function parseSQLFiles($element)
// Create an array of queries from the sql file
$queries = JDatabaseDriver::splitSql($buffer);

if (count($queries) == 0)
if (count($queries) === 0)
{
// No queries to process
return 0;
Expand Down Expand Up @@ -1177,7 +1177,7 @@ public function parseSchemaUpdates(SimpleXMLElement $schema, $eid)
// Create an array of queries from the sql file
$queries = JDatabaseDriver::splitSql($buffer);

if (count($queries) == 0)
if (count($queries) === 0)
{
// No queries to process
continue;
Expand Down Expand Up @@ -1757,7 +1757,7 @@ public function removeFiles($element, $cid = 0)
// Get the array of file nodes to process
$files = $element->children();

if (count($files) == 0)
if (count($files) === 0)
{
// No files to process
return true;
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/language/associations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public static function getAssociations($extension, $tablename, $context, $id, $p
foreach ($items as $tag => $item)
{
// Do not return itself as result
if ((int) $item->{$pk} != $id)
if ((int) $item->{$pk} !== $id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure id is an int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving as is, because the variable is defined as a parameter to the function, with type integer.

{
$multilanguageAssociations[$queryKey][$tag] = $item;
}
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/module/helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public static function &getModules($position)
}
}

if (count($result) == 0)
if (count($result) === 0)
{
if ($input->getBool('tp') && JComponentHelper::getParams('com_templates')->get('template_positions_display'))
{
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/plugin/helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public static function importPlugin($type, $plugin = null, $autocreate = true, J
// Check for the default args, if so we can optimise cheaply
$defaults = false;

if (is_null($plugin) && $autocreate == true && is_null($dispatcher))
if (is_null($plugin) && $autocreate === true && is_null($dispatcher))
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure autocreate is a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving as is, because the variable is defined as a parameter to the function, with type boolean.

{
$defaults = true;
}
Expand Down
4 changes: 2 additions & 2 deletions libraries/cms/router/site.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ protected function parseRawRoute(&$uri)
$this->setVar('Itemid', $this->app->input->getInt('Itemid', null));

// Only an Itemid OR if filter language plugin set? Get the full information from the itemid
if (count($this->getVars()) == 1 || ($this->app->getLanguageFilter() && count($this->getVars()) == 2))
if (count($this->getVars()) === 1 || ($this->app->getLanguageFilter() && count($this->getVars()) === 2))
{
$item = $this->menu->getItem($this->getVar('Itemid'));

Expand Down Expand Up @@ -620,7 +620,7 @@ protected function processBuildRules(&$uri, $stage = self::PROCESS_DURING)
$query = $uri->getQuery(true);
if ($this->_mode != 1
&& isset($query['Itemid'])
&& (count($query) == 2 || (count($query) == 3 && isset($query['lang']))))
&& (count($query) === 2 || (count($query) === 3 && isset($query['lang']))))
{
// Get the active menu item
$itemid = $uri->getVar('Itemid');
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/table/contenthistory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ public function deleteOldVersions($maxVersions)
$idsToSave = $db->loadColumn(0);

// Don't process delete query unless we have at least the maximum allowed versions
if (count($idsToSave) == (int) $maxVersions)
if (count($idsToSave) === (int) $maxVersions)
{
// Delete any rows not in our list and and not flagged to keep forever.
$query = $db->getQuery(true);
Expand Down
16 changes: 8 additions & 8 deletions libraries/cms/table/corecontent.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,21 +89,21 @@ public function bind($array, $ignore = '')
*/
public function check()
{
if (trim($this->core_title) == '')
if (trim($this->core_title) === '')
{
$this->setError(JText::_('JLIB_CMS_WARNING_PROVIDE_VALID_NAME'));

return false;
}

if (trim($this->core_alias) == '')
if (trim($this->core_alias) === '')
{
$this->core_alias = $this->core_title;
}

$this->core_alias = JApplicationHelper::stringURLSafe($this->core_alias);

if (trim(str_replace('-', '', $this->core_alias)) == '')
if (trim(str_replace('-', '', $this->core_alias)) === '')
{
$this->core_alias = JFactory::getDate()->format('Y-m-d-H-i-s');
}
Expand Down Expand Up @@ -278,8 +278,8 @@ public function store($updateNulls = false)
protected function storeUcmBase($updateNulls = false, $isNew = false)
{
// Store the ucm_base row
$db = $this->getDbo();
$query = $db->getQuery(true);
$db = $this->getDbo();
$query = $db->getQuery(true);
$languageId = JHelperContent::getLanguageId($this->core_language);

// Selecting "all languages" doesn't give a language id - we can't store a blank string in non mysql databases, so save 0 (the default value)
Expand Down Expand Up @@ -331,9 +331,9 @@ public function publish($pks = null, $state = 1, $userId = 0)
$k = $this->_tbl_key;

// Sanitize input.
$pks = ArrayHelper::toInteger($pks);
$pks = ArrayHelper::toInteger($pks);
$userId = (int) $userId;
$state = (int) $state;
$state = (int) $state;

// If there are no primary keys set check to see if the instance key is set.
if (empty($pks))
Expand Down Expand Up @@ -388,7 +388,7 @@ public function publish($pks = null, $state = 1, $userId = 0)
}

// If checkin is supported and all rows were adjusted, check them in.
if ($checkin && (count($pks) == $this->_db->getAffectedRows()))
if ($checkin && count($pks) === $this->_db->getAffectedRows())
{
// Checkin the rows.
foreach ($pks as $pk)
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/toolbar/button/popup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public function fetchButton($type = 'Modal', $name = '', $text = '', $url = '',
$onClose = '', $title = '', $footer = null)
{
// If no $title is set, use the $text element
if (strlen($title) == 0)
if (strlen($title) === 0)
{
$title = $text;
}
Expand Down