Performance 4 (plugins) #12228

frankmayer · 2016-09-30T12:55:19Z

[UPDATED (1.6.2017) - see third paragraph]
Performance (and a bit of cleanup) Batch #4

The changes in this batch are all in files under plugins.
This PR modifies code to be a bit more performant and also does some cleanup.

I have mostly done work on low hanging fruit. There are still other ways of improving, but whose involve more deep research in the code and probably more drastic changes in order to be implemented.

[Update 6.1.2017]
In order to lighten this PR up, I have introduced 5 sub-PRs, which are easier to digest and mostly with very specific changes. In order to continue with this one, those ones should be tested/reviewed/merged first. After that, I can resolve the conflicts and there should finally be only a few changes left.

1) Replaced unnecessary double quotes in plugins Replaced unnecessary double quotes in plugins #13211
2) Removed unnecessary parentheses in plugins Removed unnecessary parentheses in plugins #13212
3) Various changes in plugins Various changes in plugins #13213
4) Type safe string comparison of strings in plugins Type safe string comparison of strings in plugins #13271
5) Type safe comparison in plugins - second iteration Type safe comparison in plugins - second iteration #13272

[Update 1.6.2017]
All Sub-PRs of this PR have now been merged. That means a lot less changes to be reviewed 😄
Please review.
Thank you!

andrepereiradasilva · 2016-10-03T22:59:46Z

plugins/captcha/recaptcha/recaptcha.php

@@ -258,14 +258,14 @@ private function _recaptcha_http_post($host, $path, $data, $port = 80)
 		$http_request  = "POST $path HTTP/1.0\r\n";
 		$http_request .= "Host: $host\r\n";
 		$http_request .= "Content-Type: application/x-www-form-urlencoded;\r\n";
-		$http_request .= "Content-Length: " . strlen($req) . "\r\n";
+		$http_request .= 'Content-Length: ' . strlen($req) . "\r\n";


again why use ' when you use " in all others?

andrepereiradasilva · 2016-10-03T23:00:20Z

plugins/captcha/recaptcha/recaptchalib.php

 		{
 			die("To use reCAPTCHA you must get an API key from <a href='"
-				. self::$_signupUrl . "'>" . self::$_signupUrl . "</a>");
+				. self::$_signupUrl . "'>" . self::$_signupUrl . '</a>');


again why use ' when you use " in all others?

andrepereiradasilva · 2016-10-03T23:02:12Z

plugins/content/pagenavigation/pagenavigation.php

@@ -39,7 +39,7 @@ public function onContentBeforeDisplay($context, &$row, &$params, $page = 0)
 			return false;
 		}

-		if (($context == 'com_content.article') && ($view == 'article') && $params->get('show_item_navigation'))
+		if (($context === 'com_content.article') && ($view === 'article') && $params->get('show_item_navigation'))


you could remove the parenthisis here

andrepereiradasilva · 2016-10-03T23:16:30Z

plugins/user/profile/profile.php


-		if (($task == 'register') && ($tosenabled) && ($tosarticle) && ($option == 'com_users'))
+		// Check that the tos is checked.
+		if (($task === 'register') && $tosenabled && $tosarticle && ($option === 'com_users') && !$data['profile']['tos'])


you could remove the parenthisis here

andrepereiradasilva · 2016-10-04T01:36:30Z

I have tested this item ✅ successfully on 42a5a54

code review

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12228.}

# Conflicts: # plugins/content/pagebreak/pagebreak.php # plugins/content/pagenavigation/pagenavigation.php # plugins/system/debug/debug.php # plugins/system/languagefilter/languagefilter.php # plugins/user/profile/field/dob.php # plugins/user/profile/profile.php

frankmayer · 2016-12-27T23:36:50Z

This PR is waiting on merge of sub PR #13272, so I can resolve conflicts here and get this reviewed / tested / merged, too.

wilsonge · 2016-12-29T14:51:30Z

plugins/system/fields/fields.php

@@ -472,6 +472,8 @@ public function onContentPrepare($context, $item)
 		{
 			$item->fields[$field->id] = $field;
 		}
+
+		return;


This isn't needed?

Reverted this one to its prior state, so it won't raise conflicts with @laoneo 's efforts.

frankmayer · 2017-01-06T15:41:20Z

Edited initial post to show which sub-PRs still have to go through, in order to make this one easier to digest.
It would be great if the initial big batch PRs of mine could make it into alpha2. Thanks!

# Conflicts: # plugins/authentication/gmail/gmail.php # plugins/editors-xtd/image/image.php # plugins/editors/tinymce/tinymce.php # plugins/system/debug/debug.php # plugins/system/fields/fields.php # plugins/user/joomla/joomla.php # plugins/user/profile/field/dob.php

frankmayer · 2017-05-31T21:13:26Z

Pls don't review this yet. There was a git problem. Need to resolve first.

# Conflicts: # plugins/captcha/recaptcha/recaptchalib.php # plugins/content/contact/contact.php # plugins/finder/categories/categories.php # plugins/finder/contacts/contacts.php # plugins/finder/newsfeeds/newsfeeds.php # plugins/finder/tags/tags.php

frankmayer · 2017-05-31T22:24:56Z

All Sub-PRs of this PR have now been merged. That means a lot less changes to be reviewed 😄
Please review.
Thank you!

andrepereiradasilva · 2017-05-31T22:38:55Z

next in my list of tests - tomorrow probably

Quy · 2017-06-01T00:28:28Z

plugins/captcha/recaptcha/recaptcha.php

@@ -268,7 +268,7 @@ private function _recaptcha_http_post($host, $path, $data, $port = 80)

 		$response = '';

-		if (($fs = @fsockopen($host, $port, $errno, $errstr, 10)) == false )
+		if (($fs = @fsockopen($host, $port, $errno, $errstr, 10)) === false )


Remove space after false.

Quy · 2017-06-01T00:29:59Z

plugins/content/pagenavigation/pagenavigation.php

@@ -39,7 +39,7 @@ public function onContentBeforeDisplay($context, &$row, &$params, $page = 0)
 			return false;
 		}

-		if (($context === 'com_content.article') && ($view === 'article') && $params->get('show_item_navigation'))
+		if (($context === 'com_content.article') && $view === 'article' && $params->get('show_item_navigation'))


Remove parenthesis ($context === 'com_content.article')

Quy · 2017-06-01T00:35:53Z

plugins/system/debug/debug.php

@@ -1110,7 +1106,7 @@ protected function displayQueries()
 					$title = '<span class="dbg-noprofile">' . $title . '</span>';
 				}

-				$htmlProfile = ($info[$id]->profile ? $info[$id]->profile : JText::_('PLG_DEBUG_NO_PROFILE'));
+				$htmlProfile = ($info[$id]->profile ?: JText::_('PLG_DEBUG_NO_PROFILE'));



Remove outside parenthesis (ignore if done in a different PR).

Don't remember if that is in any other PR. There are so many PR's still open, that some may overlap. I'll just do that here and move on 😄
Thanks for reviewing!

Quy · 2017-06-01T01:04:27Z

plugins/user/profile/profile.php

@@ -384,13 +384,10 @@ public function onUserBeforeSave($user, $isnew, $data)
 		$tosarticle = $this->params->get('register_tos_article');
 		$tosenabled = ($this->params->get('register-require_tos', 0) == 2);

-		if (($task === 'register') && $tosenabled && $tosarticle && ($option === 'com_users'))
+		// Check that the tos is checked.
+		if (($task === 'register') && $tosenabled && $tosarticle && $option === 'com_users' && !$data['profile']['tos'])


Remove parenthesis ($task === 'register').

frankmayer · 2017-06-01T16:07:08Z

@Quy would you be so kind and check the changes of your reviews for this one, too? So maybe we can get this RTC as soon as @andrepereiradasilva also has time to check it?
Thanks guys!!!

Quy · 2017-06-01T16:15:03Z

I have tested this item ✅ successfully on 475b880

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12228.}

ghost · 2017-06-01T17:05:20Z

RTC after two successful tests.

andrepereiradasilva

some minor issues/sugestions

andrepereiradasilva · 2017-06-01T11:51:17Z

plugins/authentication/ldap/ldap.php

@@ -72,7 +72,7 @@ public function onUserAuthenticate($credentials, $options, &$response)
 			{
 				// Bind using Connect Username/password
 				// Force anon bind to mitigate misconfiguration like [#7119]
-				if (strlen($this->params->get('username')))
+				if ($this->params->get('username'))


strict? $this->params->get('username', '') !== ''

Yes.. thanks

andrepereiradasilva · 2017-06-01T11:52:48Z

plugins/captcha/recaptcha/recaptcha.php

@@ -40,7 +40,7 @@ public function onInit($id = 'dynamic_recaptcha_1')
 	{
 		$pubkey = $this->params->get('public_key', '');

-		if ($pubkey == null || $pubkey == '')
+		if ($pubkey === null || $pubkey === '')


question: can pubkey be null? when in line above default to '' if doesn't exist

I think it can't... changing

andrepereiradasilva · 2017-06-01T11:53:38Z

plugins/captcha/recaptcha/recaptcha.php

@@ -117,13 +117,13 @@ public function onCheckAnswer($code = null)
 			case '1.0':
 				$challenge = $input->get('recaptcha_challenge_field', '', 'string');
 				$response  = $input->get('recaptcha_response_field', '', 'string');
-				$spam      = ($challenge == null || strlen($challenge) == 0 || $response == null || strlen($response) == 0);
+				$spam      = ($challenge === null || $challenge === '' || $response === null || $response === '');


same as above

andrepereiradasilva · 2017-06-01T11:54:42Z

plugins/captcha/recaptcha/recaptcha.php

 				break;
 			case '2.0':
 				// Challenge Not needed in 2.0 but needed for getResponse call
 				$challenge = null;
 				$response  = $input->get('g-recaptcha-response', '', 'string');
-				$spam      = ($response == null || strlen($response) == 0);
+				$spam      = ($response === null || $response === '');


same as above

andrepereiradasilva · 2017-06-01T11:56:24Z

plugins/captcha/recaptcha/recaptchalib.php

@@ -131,7 +131,7 @@ public function verifyResponse($remoteIp, $response)
 		$answers = json_decode($getResponse, true);
 		$recaptchaResponse = new JReCaptchaResponse();

-		if (trim($answers['success']) == true)
+		if (trim($answers['success']))


strict? trim($answers['success']) !== ''

andrepereiradasilva · 2017-06-01T12:04:21Z

plugins/finder/categories/categories.php

 		{
 			return;
 		}

 		// Check if the extension that owns the category is also enabled.
-		if (JComponentHelper::isEnabled($item->extension) === false)
+		if (!JComponentHelper::isEnabled($item->extension))
