[#12233] - Performance 6 (templates)
- Fixed in Code Base
- 25 Jul 2017
- Medium
- Build: staging
- # 12233
- Diff
- frankmayer:Performance_6
- Success hound No violations found. Woof! Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
- Pending continuous-integration/jenkins/pr-merge This commit is being built Details
User tests: Successful: Unsuccessful:
[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 #13244
- 2) Replace unnecessary double quotes in templates #13245
- 3) Type-safe string comparison in site/templates #13309
- 4) 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!
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Front End Templates (site) |
| Labels |
Added:
?
|
||
Definitely easier to read but it's not just a 1-for-1 swap between and and && or || and or.
http://php.net/manual/en/language.operators.logical.php will probably cover it better than I can.
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.
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.
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
in this one i'm just not sure of the mantainers position in using the JS/CSS HEREDOC for inline js/css in the beez3 template files. I'm not in favour of it, but that is just me.
for the rest it seems ok
No issues, it's just a proposal that I thought of, when I saw it. I can remove it, if it's not wanted.
However, IMHO it looks cleaner that way, as the variables are defined and the HEREDOC is easier to read and modify if needed.
again, IMHO, the usage of HEREDOC in CSS and JS (or even HTML) is a thing that needs some Code Style mantainers decision. IMHO it should be decided if the code should use HEREDOC in all css/js/html across the code or not.
And that change would be another PR, not this one. But that's my opinion.
Without the HEREDOC part, as said before, for me is ok on code review.
@andrepereiradasilva I will remove the heredoc changes from this PR, then, so it can move on.
see comment #12233 (comment)
I have tested this item
code review
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12233.
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!
| Category | Front End Templates (site) | ⇒ | Administration Templates (admin) Front End Templates (site) |
[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!
I have tested this item
code review
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12233.
I have tested this item
Code review.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12233.
RTC please, thank you
| Status | Pending | ⇒ | Ready to Commit |
RTC after two successful tests.
| Milestone |
Added: |
||
| Milestone |
Added: |
||
| Milestone |
Removed: |
||
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.
That'd be helpful.
Base changed to 3.8-dev without conflicts on this PR.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-07-25 23:46:54 |
| Closed_By | ⇒ | mbabker |
I hadn't heard that either but I can see the logic behind it.