Performance 2 (libraries/legacy) by frankmayer · Pull Request #12220 · joomla/joomla-cms

frankmayer · 2016-09-30T11:11:49Z

[UPDATED (6.1.2017) - see second paragraph]
Performance (and a bit of cleanup) Batch #2

The changes in this batch are all in files under libraries/legacy.
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 6 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) Replace unnecessary double quotes in libraries/legacy Replace unnecessary double quotes in libraries/legacy #13239
2) Simplify some ternary operations using elvis operator and remove unnecessary parentheses in libraries/legacy Simplify some ternary operations using elvis operator and remove unnecessary parentheses in libraries/legacy #13240
3) Various changes in libraries legacy Various changes in libraries legacy #13241

sovainfo · 2016-09-30T16:49:12Z

Could you explain when '== 0' is oke and when it needs '=== 0' ?

mbabker · 2016-09-30T16:52:35Z

=== is more strict than ==, it will validate that something matches both the value and data type (so in that case 0 === '0' would fail but 0 == '0' is OK).

It's useful with some of the string functions where you can have a boolean false and an integer zero return. With == the values are compared to represent the same thing but with === they aren't.

sovainfo · 2016-09-30T17:03:16Z

libraries/legacy/categories/categories.php

 					// If the node's parent id is not in the _nodes list and the node is not root (doesn't have parent_id == 0),
 					// then remove the node from the list
-					if (!(isset($this->_nodes[$result->parent_id]) || $result->parent_id == 0))
+					if (!($result->parent_id == 0 || isset($this->_nodes[$result->parent_id])))


Shouldn't this be === 0 ?

I didn't strict type those comparisons where it was not clear if the value is really strict. In that case it could be an integer or a string with a number. So, not to break BC I left those alone.

You could use (int) and them do the strict comparation .

Yes, but they also do involve a lot more research. It's not always just doing some casting.
I thought that those kind of changes would/could be done later by me or someone else.
Don't these batches already have enough changes?

mahagr · 2016-10-03T09:31:36Z

@sovainfo The main issue with == comparison is that sometimes it casts the values in very odd way, for example:

    echo (0 == 'asd') ? 'true' : 'false'; // true

Which is just wrong. On the other hand you really need to be careful with === as if you're comparing different types, it always returns false. This is true especially with all (integer) values coming from DB, which are always strings, but usually integers if you happen to update value of the object:

    echo (0 === '0') ? 'true' : 'false'; // false

sovainfo · 2016-10-03T16:24:32Z

Thank you all for the feedback. For some reason thought that 'null' would never compare equal to anything, not even 'null'. Maybe that only applies to SQL, requiring you to use 'IS NULL'.

Concerning values coming from the database, expect the driver to make sure the right datatype to be returned. Apparently it doesn't.

- Codestyle

andrepereiradasilva · 2017-06-13T18:24:26Z

I have tested this item ✅ successfully on 0148c52

code review

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

Quy · 2017-06-13T18:34:06Z

libraries/legacy/application/application.php

 	public function isSSLConnection()
 	{
-		return (isset($_SERVER['HTTPS']) && ($_SERVER['HTTPS'] == 'on')) || getenv('SSL_PROTOCOL_VERSION');
+		return (isset($_SERVER['HTTPS']) && ($_SERVER['HTTPS'] === 'on')) || getenv('SSL_PROTOCOL_VERSION');


Remove parentheses ($_SERVER['HTTPS'] === 'on')

Quy · 2017-06-13T18:35:59Z

libraries/legacy/controller/admin.php


 		$ids = $this->input->post->get('cid', array(), 'array');
-		$inc = ($this->getTask() == 'orderup') ? -1 : 1;
+		$inc = ($this->getTask() === 'orderup') ? -1 : 1;


Remove parentheses

Quy · 2017-06-13T18:38:45Z

libraries/legacy/model/list.php

 		$input     = $app->input;
 		$old_state = $app->getUserState($key);
-		$cur_state = (!is_null($old_state)) ? $old_state : $default;
+		$cur_state = ($old_state !== null) ? $old_state : $default;


Remove parentheses

Quy · 2017-06-13T18:39:25Z

libraries/legacy/table/menu.php


 		// Verify that the default home menu set to "all" languages" is not unset
-		if ($this->home == '1' && $this->language == '*' && ($array['language'] != '*'))
+		if ($this->home == '1' && $this->language === '*' && ($array['language'] != '*'))


Remove parentheses

andrepereiradasilva · 2017-06-13T19:27:26Z

I have tested this item ✅ successfully on 0148c52

code review

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

Quy · 2017-06-13T20:43:36Z

libraries/legacy/table/menu.php

 	{
 		// Verify that the default home menu is not unset
-		if ($this->home == '1' && $this->language == '*' && ($array['home'] == '0'))
+		if ($this->home == '1' && $this->language === '*' && ($array['home'] == '0'))


Remove parentheses

andrepereiradasilva · 2017-06-13T21:50:03Z

I have tested this item ✅ successfully on 0148c52

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

Quy · 2017-06-13T22:04:19Z

I have tested this item ✅ successfully on 0148c52

Code review.

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

frankmayer · 2017-06-15T09:00:17Z

RTC please, thank you

ghost · 2017-06-15T12:50:11Z

RTC after two successful tests.

ghost · 2017-06-15T12:50:44Z

Thanks for Hint, @frankmayer

frankmayer · 2017-06-28T13:53:28Z

Merge pls?

# Conflicts: # libraries/legacy/categories/categories.php # libraries/loader.php # libraries/src/Joomla/CMS/Controller/Controller.php # libraries/src/Joomla/CMS/Controller/Form.php # libraries/src/Joomla/CMS/Model/Model.php

@Hoffi1

* staging: (274 commits) Add JCryptCipherSodium to support libsodium (joomla#16754) Performance 2 (libraries/legacy) (joomla#12220) Performance 6 (templates) (joomla#12233) Fixed typehint (joomla#16425) Fix for: Repeatable field is no longer rendered with Chosen layout (joomla#16471) Fix the path for the ajax-loader.gif (joomla#16701) Menu items list parent filter (joomla#17060) Text Filters layout (joomla#17113) mod_login showon option (joomla#17153) com_banners incorret tooltip (joomla#17157) fix joomla.content.options_default (joomla#17123) remove the never working limitstart call (joomla#17184) Update phpDocumentor build set 3.8.0 Dev State Prepare 3.7.4 Stable Release fixed a logic change in joomla#12294, thanks @Hoffi1 Update sv-SE.ini Update pt-BR.ini Update lv-LV.ini Update fa-IR.ini ...

frankmayer added 23 commits September 29, 2016 03:03

Fixed misuses of array_push()

5a2c4bc

Replace is_null() with ... === null

985d3c4

Shortened syntax for applied operations

42b577b

Don't use strlen() to check if string is empty.

5537d30

Replace some cases of substr() with strpos()

427ff04

Merge unset() calls

cd706f1

Remove unnecessary reference operator

541e5da

Remove useless return

088ed41

Optimize non-optimal if conditions.

7965a58

Inline one-time use variables

6b18202

Removed unnecessary ternary operators

d24aedb

Ternary to Elvis..

f2622f9

Optimize non-optimal if conditions.

5c3415e

Replace doublequotes with quotes

e692032

Remove unnecessary parentheses

4b0bddb

Compare strings type-safely

8cdd32a

Optimize non-optimal if conditions.

3c1d4bf

Removed unnecessary ternary operators

7da90c4

Compare strings type-safely

c06fb90

Replaced non-optimal regular expression

edbefe0

Removed unused variable

cdcfef6

Fix formating

974c28f

Fix strpos inversion mistake

96ee0ac

joomla-cms-bot added the PR-staging label Sep 30, 2016

Fix codesniffer complaints

696a82b

sovainfo reviewed Sep 30, 2016

View reviewed changes

- Remove some parentheses

0148c52

- Codestyle

Quy reviewed Jun 13, 2017

View reviewed changes

Changes according to reviewer's comment + a few more changes

4da2433

Quy reviewed Jun 13, 2017

View reviewed changes

Changes according to reviewer's comment

f0ad609

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 15, 2017

rdeutz modified the milestone: Joomla 3.7.4 Jun 21, 2017

rdeutz modified the milestones: Joomla 3.8.0, Joomla 3.7.4 Jul 5, 2017

frankmayer mentioned this pull request Jul 20, 2017

Performance 6 (templates) #12233

Merged

4 tasks

frankmayer changed the base branch from staging to 3.8-dev July 20, 2017 13:26

Merge branch '3.8-dev' into Performance_2

263b8d8

# Conflicts: # libraries/legacy/categories/categories.php # libraries/loader.php # libraries/src/Joomla/CMS/Controller/Controller.php # libraries/src/Joomla/CMS/Controller/Form.php # libraries/src/Joomla/CMS/Model/Model.php

joomla-cms-bot added PR-3.8-dev and removed RTC This Pull Request is Ready To Commit labels Jul 20, 2017

frankmayer deleted the Performance_2 branch August 23, 2017 19:09

This was referenced Sep 21, 2017

Side effects Revert Pull? #18055

Closed

[Regression] - Set value as string when doing a strict comparison. #18061

Merged

Uh oh!

Conversation

frankmayer commented Sep 30, 2016 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sovainfo commented Sep 30, 2016

Uh oh!

mbabker commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahagr commented Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sovainfo commented Oct 3, 2016

Uh oh!

andrepereiradasilva commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Jun 13, 2017

Uh oh!

Quy commented Jun 13, 2017

Uh oh!

frankmayer commented Jun 15, 2017

Uh oh!

ghost commented Jun 15, 2017

Uh oh!

ghost commented Jun 15, 2017

Uh oh!

frankmayer commented Jun 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

frankmayer commented Sep 30, 2016 •

edited by joomla-cms-bot

Loading

mahagr commented Oct 3, 2016 •

edited

Loading