Performance 6 (templates) by frankmayer · Pull Request #12233 · joomla/joomla-cms

frankmayer · 2016-09-30T18:30:41Z

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

The changes in this batch are all in files under templates.
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 and cleaning up, but those 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 4 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) Simplify some ternary operations using elvis operator and remove unnecessary parentheses in templates Simplify some ternary operations using elvis operator and remove unnecessary parentheses in templates #13244
2) Replace unnecessary double quotes in templates Replace unnecessary double quotes in templates #13245
3) Type-safe string comparison in site/templates Type-safe string comparison in site/templates #13309
4) More type-safe comparisons in site/templates More type-safe comparisons in site/templates #13310

[Update 10.6.2017]
All Sub-PRs of this PR have now been merged. That means a lot less changes to be reviewed 😄

Attention potential testers: Those changes should only need code review.

Thank you!

…2225)

#12225)

…#12225)

Bakual · 2016-09-30T18:36:24Z

templates/beez3/html/com_content/archive/default_items.php



-<?php if (($params->get('show_author')) or ($params->get('show_parent_category')) or ($params->get('show_category')) or ($params->get('show_create_date')) or ($params->get('show_modify_date')) or ($params->get('show_publish_date'))  or ($params->get('show_hits'))) : ?>
+<?php if ($params->get('show_author') || $params->get('show_parent_category') || $params->get('show_category') || $params->get('show_create_date') || $params->get('show_modify_date') || $params->get('show_publish_date') || $params->get('show_hits')) : ?>


In ~~template~~ layout files (everything within the /tmpl/ folders) we prefer or and and over || and &&. So please leave those alone.

Didn't know that. Why is this?

It's easier to read for people (users) which aren't that familiar with PHP.
Same for the if (...) : ... endif instead of if (...) { ... }

When did that become a thing? Because last I remember that's NOT in our codestyle rules or documented guidelines. Not to mention there are (mostly small) behavioral differences between $foo and $bar versus $foo && $bar.

Is there some kind of system with those? I ask because there are also plenty of && and || in those files.
A quick count revealed
84 x && | 29 x and
49 x || vs x 48 or

When did that become a thing? Because last I remember that's NOT in our codestyle rules or documented guidelines.

Agreed, it's not in the codestyle rules. But I was under the impression that it is usally done like this and I always followed that unspoken rule. Same as the if ... endif stuff which isn't (imho) written in the codestyle rules as well.

brianteeman · 2016-09-30T18:52:28Z

I hadn't heard that either but I can see the logic behind it.

mbabker · 2016-09-30T18:55:54Z

Definitely easier to read but it's not just a 1-for-1 swap between and and && or || and or.

frankmayer · 2016-09-30T19:03:50Z

@mbabker could you elaborate on that?

mbabker · 2016-09-30T19:04:55Z

http://php.net/manual/en/language.operators.logical.php will probably cover it better than I can.

frankmayer · 2016-09-30T19:13:05Z

@mbabker Got it. Thanks.

frankmayer · 2016-09-30T19:21:48Z

@mbabker ok but from what I see, most - if not all of those are swap-able 1-for-1. Why is it preferred then? And why specifically for the tmpl?

Bakual · 2016-09-30T19:38:59Z

The tmpl folder contains the HTML output, which is the stuff users likely want to adjust.
Those files also are easy to override in a template. The files you got in the /html/ folder are actually overrides of the layout files in the /tmpl/ folder.

most - if not all of those are swap-able 1-for-1

In the layout files, they usually are interchangeable. It only starts to matter if you have them mixed in a assignments and ternaries (see http://php.net/manual/en/language.operators.precedence.php). But there I prefer to use () around the logical blocks anyway to make it simpler to read.

frankmayer · 2016-09-30T19:44:00Z

Ok, I see, but still it's a mix of both styles, with the && and || having the most occurrences. That can be even more confusing to "users".
I would suggest to choose one way and go with it.

Bakual · 2016-09-30T19:48:55Z

I would suggest to choose one way and go with it.

Agreed, but please don't do it in this PR. If we want to choose one way, the proper way is to define a codestyle rule and then apply it. Otherwise you're going to change it now and the next guy wants to change it back 😄

frankmayer · 2016-09-30T19:55:11Z

😄 Yes, of course! I have already reversed those changes.

andrepereiradasilva · 2016-10-03T22:26:13Z

templates/beez3/html/com_content/archive/default_items.php

 	<dd class="createdby">
 		<?php $author = $item->author; ?>
-		<?php $author = ($item->created_by_alias ? $item->created_by_alias : $author);?>
+		<?php $author = ($item->created_by_alias ?: $author);?>


like the other PR just

<?php $author = $item->created_by_alias ?: $item->author; ?>

andrepereiradasilva · 2016-10-03T22:27:24Z

templates/beez3/html/com_content/article/default.php

 	<dd class="createdby">
 		<?php $author = $this->item->author; ?>
-		<?php $author = ($this->item->created_by_alias ? $this->item->created_by_alias : $author);?>
+		<?php $author = ($this->item->created_by_alias ?: $author);?>


again, just

<?php $author = $this->item->created_by_alias ?: $this->item->author; ?>

andrepereiradasilva · 2016-10-03T22:29:02Z

templates/beez3/html/com_content/category/blog.php

 <?php endif; ?>
 <?php
-	$introcount = (count($this->intro_items));
+	$introcount = count($this->intro_items);


actually this seems to be only used once ahead so it should only be there.

see ahead

<?php if (($rowcount == $this->columns) or ($counter == $introcount)) : ?>

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

templates/beez3/html/com_content/category/blog_item.php

 	<dd class="createdby">
 		<?php $author = $this->item->author; ?>
-		<?php $author = ($this->item->created_by_alias ? $this->item->created_by_alias : $author);?>
+		<?php $author = ($this->item->created_by_alias ?: $author);?>


again, just

<?php $author = $this->item->created_by_alias ?: $this->item->author; ?>

andrepereiradasilva · 2016-10-03T22:30:51Z

templates/beez3/html/com_content/category/default_articles.php

 						<?php if (!empty($article->author) || !empty($article->created_by_alias)) : ?>
 							<?php $author = $article->author ?>
-							<?php $author = ($article->created_by_alias ? $article->created_by_alias : $author); ?>
+							<?php $author = ($article->created_by_alias ?: $author); ?>


again, just

<?php $author = $article->created_by_alias ?: $article->author; ?>

andrepereiradasilva · 2016-10-03T22:31:54Z

templates/beez3/html/com_content/featured/default_item.php

 	<dd class="createdby">
 		<?php $author = $this->item->author; ?>
-		<?php $author = ($this->item->created_by_alias ? $this->item->created_by_alias : $author);?>
+		<?php $author = ($this->item->created_by_alias ?: $author);?>


again, just

<?php $author = $this->item->created_by_alias ?: $this->item->author; ?>

andrepereiradasilva · 2016-10-03T22:35:17Z

templates/beez3/jsstrings.php

-	var bildauf    = '" . $this->baseurl . "/templates/" . $this->template . "/images/plus.png';
-	var bildzu     = '" . $this->baseurl . "/templates/" . $this->template . "/images/minus.png';
+	var bildauf    = '" . $this->baseurl . '/templates/' . $this->template . "/images/plus.png';
+	var bildzu     = '" . $this->baseurl . '/templates/' . $this->template . "/images/minus.png';


dont't understand why this change, before the baseurl var you have " and after '. IMHO or we use always ' or we use always "

Thanks, this was an automated change which I obviously reviewed poorly... 😄

Actually this was supposed to be a HEREDOC.. Forgot to implement...

@frankmayer please review this

andrepereiradasilva · 2016-10-03T22:35:56Z

templates/beez3/index.php

-	}");
+		background: " . $this->params->get('backgroundcolor') . ';
+	}'
+	);


dont't understand why this change, before you have " and after '. IMHO or we use always ' or we use always "

Quy · 2017-06-11T23:04:19Z

templates/beez3/html/com_content/category/blog_item.php


 <?php if ($this->item->state == 0 || strtotime($this->item->publish_up) > strtotime(JFactory::getDate())
-	|| ((strtotime($this->item->publish_down) < strtotime(JFactory::getDate())) && $this->item->publish_down != JFactory::getDbo()->getNullDate())) : ?>
+	|| ((strtotime($this->item->publish_down) < strtotime(JFactory::getDate())) && $this->item->publish_down !== JFactory::getDbo()->getNullDate())) : ?>


Remove parentheses.

Quy · 2017-06-11T23:06:24Z

templates/beez3/html/com_content/category/default.php

-<?php if ($this->params->get('show_page_heading') and ($this->params->get('show_category_title') or $this->params->get('page_subheading'))) : ?>
+<?php
+if ($this->params->get('show_page_heading')) : ?>
+<?php if ($this->params->get('show_page_heading') and ($showCategoryTitle === true or $pageSubHeading)) : ?>


Redundant checking with the previous line $this->params->get('show_page_heading'.

andrepereiradasilva

two doubts

andrepereiradasilva · 2017-06-13T18:28:45Z

templates/beez3/html/com_content/article/default.php

 		</h2>
 <?php endif; ?>
-<?php if ($this->params->get('show_page_heading') and $params->get('show_title')) :?>
+<?php if ($showPageHeading and $params->get('show_title')) :?>


are you sure $showPageHeading defined at this time?

It is defined in the first if() statement at line 30. Or, did I miss something?

right sorry ... tought this instruction was inside an else

No issue, thought I had messed up :)

andrepereiradasilva · 2017-06-13T18:31:30Z

templates/beez3/html/com_content/category/blog.php

 		</article>
 		<?php $counter++; ?>
-		<?php if (($rowcount == $this->columns) or ($counter == $introcount)) : ?>
+		<?php if ($rowcount === $this->columns or $counter === $introcount) : ?>


are you sure $this->columns is an integer ? (see line above echo (int) $this->columns;)

Thank you. Somehow got that wrong...

Quy · 2017-06-13T18:52:24Z

templates/beez3/html/com_content/article/default.php

-		<?php $author = ($this->item->created_by_alias ?: $author);?>
-		<?php if (!empty($this->item->contact_link ) &&  $params->get('link_author') == true) : ?>
+		<?php $author = $this->item->created_by_alias ?: $this->item->author; ?>
+		<?php if (!empty($this->item->contact_link) &&  $params->get('link_author') == true) : ?>


Remove extra space

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

I have tested this item ✅ successfully on 4f3c91f

code review

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

Quy · 2017-06-13T22:16:36Z

I have tested this item ✅ successfully on 4f3c91f

Code review.

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

frankmayer · 2017-06-15T08:59:52Z

RTC please, thank you

ghost · 2017-06-15T12:51:20Z

RTC after two successful tests.

yvesh · 2017-06-15T15:53:33Z

templates/beez3/html/com_content/article/default.php

+<?php if ($showPageHeading = $this->params->get('show_page_heading')) : ?>

-<?php if ($this->params->get('show_page_heading') and $params->get('show_title')) :?>
+<?php if ($showPageHeading and $params->get('show_title')) :?>


Is there any reason we are using and here? :-)

Since this really big one is already RTC, and those seem to be no show-stoppers, do you mind marking those reviews as non-stoppers? It took a long time to go RTC for those bug ones :).
I will happily open a new PR for those ones after this is merged.

Also for this one and the one after the next, those are mixed files, and there has been lot of discussion on that in one of my other PR's.. in the code-standards Repo. Still ongoing I think.

yvesh · 2017-06-15T15:57:18Z

templates/beez3/html/com_content/category/blog_item.php


 <?php if ($this->item->state == 0 || strtotime($this->item->publish_up) > strtotime(JFactory::getDate())
-	|| ((strtotime($this->item->publish_down) < strtotime(JFactory::getDate())) && $this->item->publish_down != JFactory::getDbo()->getNullDate())) : ?>
+	|| (strtotime($this->item->publish_down) < strtotime(JFactory::getDate()) && $this->item->publish_down !== JFactory::getDbo()->getNullDate())) : ?>


instead of calling strtotime on JFactory::getDate you can use JFactory::getDate()->getTimestamp();

yvesh · 2017-06-15T15:58:02Z

templates/beez3/html/com_content/category/default.php

 <?php endif; ?>

-<?php if ($this->params->get('show_category_title') or $this->params->get('page_subheading')) : ?>
+<?php if ($showCategoryTitle === true or $pageSubHeading) : ?>


Why are we using or here? :-)

yvesh · 2017-06-15T15:58:49Z

templates/beez3/html/com_newsfeeds/category/default_children.php

 <?php foreach ($this->children[$this->category->id] as $id => $child) : ?>
 	<?php
-	if ($this->params->get('show_empty_categories') || $child->numitems || count($child->getChildren())) :
+	if ($child->numitems || $this->params->get('show_empty_categories') || count($child->getChildren())) :


!empty is faster than count :-)

yvesh · 2017-06-15T16:00:17Z

templates/beez3/html/mod_breadcrumbs/default.php

 	// Make a link if not the last item in the breadcrumbs
 	$show_last = $params->get('showLast', 1);
-	if ($key != $last_item_key)
+	if ($key !== $last_item_key)


new line missing before (yep i know it has been like that)

rdeutz · 2017-07-05T15:35:18Z

I am not going to merge these types of PR into 3.7.4, we have seen that chances are good to miss something and then we introduce a bug as we have seen in the last release. Make the software more solid and stable is more important.

frankmayer · 2017-07-20T13:22:05Z

@mbabker should I change the base of this PR and of #12220 to 3.8-dev and resolve any conflicts if there are any?

mbabker · 2017-07-20T13:24:58Z

That'd be helpful.

frankmayer · 2017-07-20T13:27:26Z

Base changed to 3.8-dev without conflicts on this PR.

frankmayer · 2017-07-20T13:56:28Z

@mbabker Base changed to 3.8-dev on this PR. Conflicts resolved.

@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 13 commits September 29, 2016 16:47

Type-safe string comparisons

4eba2c8

More type-safe comparisons

991054e

Replace is_null with ===

d8def77

Use Elvis instead of complete ternary

7059c08

Merge nested if

2eae3b4

Replace single quotes with double quotes

c7a2763

Remove unnecessary parentheses

1e7efe8

Optimize non-optimal if conditions.

e9a2501

Reverted some condition flips as requested in changes of other PR (#1…

91bfc5d

…2225)

Reverted some more condition flips as requested in changes of other PR (

1ef7b96

#12225)

Reverted even more condition flips as requested in changes of other PR (

6e1fe57

#12225)

Reverted one more condition flips as requested in changes of other PR (…

a6488fb

…#12225)

Replaced and, or with &&, ||

baded6c

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

Bakual reviewed Sep 30, 2016

View reviewed changes

Comply to change request in PR discussion: Leave 'or' and 'and' alone

2f71dcf

Missed a spot...

7fbd96f

andrepereiradasilva reviewed Oct 3, 2016

View reviewed changes

frankmayer added 2 commits October 4, 2016 02:25

This way, this JS is much easier to read...

db7c623

Another HEREDOC

2b7a9aa

Quy reviewed Jun 11, 2017

View reviewed changes

frankmayer added 2 commits June 13, 2017 18:46

Merge branch 'staging' into Performance_6

aba53b7

Changes according to reviewer's comments

2841cf2

andrepereiradasilva reviewed Jun 13, 2017

View reviewed changes

Changes according to reviewer's comment

9a1502d

Quy reviewed Jun 13, 2017

View reviewed changes

Changes according to reviewer's comment

1908a16

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

yvesh reviewed Jun 15, 2017

View reviewed changes

rdeutz added this to the Joomla 3.7.4 milestone Jun 21, 2017

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

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

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 25, 2017

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



		<?php if (($params->get('show_author')) or ($params->get('show_parent_category')) or ($params->get('show_category')) or ($params->get('show_create_date')) or ($params->get('show_modify_date')) or ($params->get('show_publish_date')) or ($params->get('show_hits'))) : ?>
		<?php if ($params->get('show_author') \|\| $params->get('show_parent_category') \|\| $params->get('show_category') \|\| $params->get('show_create_date') \|\| $params->get('show_modify_date') \|\| $params->get('show_publish_date') \|\| $params->get('show_hits')) : ?>

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!

Bakual Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

brianteeman commented Sep 30, 2016 via email

Uh oh!

mbabker commented Sep 30, 2016

Uh oh!

frankmayer commented Sep 30, 2016

Uh oh!

mbabker commented Sep 30, 2016

Uh oh!

frankmayer commented Sep 30, 2016

Uh oh!

frankmayer commented Sep 30, 2016

Uh oh!

Bakual commented Sep 30, 2016

Uh oh!

frankmayer commented Sep 30, 2016

Uh oh!

Bakual commented Sep 30, 2016

Uh oh!

frankmayer commented Sep 30, 2016

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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 left a comment

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!

frankmayer commented Sep 30, 2016 •

edited by joomla-cms-bot

Loading

Bakual Sep 30, 2016 •

edited

Loading

andrepereiradasilva Oct 3, 2016 •

edited

Loading

andrepereiradasilva Oct 3, 2016 •

edited

Loading

andrepereiradasilva Oct 3, 2016 •

edited

Loading

yvesh Jun 15, 2017 •

edited

Loading

yvesh Jun 15, 2017 •

edited

Loading