? Success

User tests: Successful: Unsuccessful:

avatar okonomiyaki3000
okonomiyaki3000
14 Oct 2016

Pull Request for Issue # .

Summary of Changes

Separated the various parts of a long function into smaller ones. The main purpose being that overriding some small part of this class in a subclass will be a lot simpler.

Avoiding using string concatenation on long strings by pushing to an array and imploding at the end. This is generally better for performance although, in this case, it may be offset somewhat by the overhead of calling additional functions.

Avoid the /> tag ending for void elements when in html5 mode. Just use > instead. Save a whole byte per tag!

Testing Instructions

Load any page that has a 'head'. All of your tags should appear as expected.

Documentation Changes Required

Nope

avatar okonomiyaki3000 okonomiyaki3000 - open - 14 Oct 2016
avatar okonomiyaki3000 okonomiyaki3000 - change - 14 Oct 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 14 Oct 2016
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 14 Oct 2016
Category Libraries
avatar C-Lodder
C-Lodder - comment - 14 Oct 2016

I believe * @since 3.7 will need to be changed to * @since __DEPLOY_VERSION__ in the comment blocks

avatar SharkyKZ
SharkyKZ - comment - 14 Oct 2016

Is there any reason a space is added at the end of the tag?

avatar SharkyKZ
SharkyKZ - comment - 14 Oct 2016

Empty meta tags are added after patch. Should check that content is not empty before adding tags.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 17 Oct 2016

@SharkyKZ Thanks for the tips. The original code had a space before tag endings so I left it in but you're right, it's meaningless. Another byte saved!

avatar okonomiyaki3000 okonomiyaki3000 - change - 2 Nov 2016
The description was changed
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 12 Jan 2017

I have tested this item 🔴 unsuccessfully on 3efe4de

After apply Patch got:
Notice: Undefined index: mime in __/Joomla/libraries/joomla/document/renderer/html/head.php on line 252
Notice: Undefined index: attribs in __/Joomla/libraries/joomla/document/renderer/html/head.php on line 280


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12413.

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 12 Jan 2017 - Tested unsuccessfully
avatar okonomiyaki3000
okonomiyaki3000 - comment - 16 Jan 2017

Should be good to go now.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 21 Jan 2017

@okonomiyaki3000 Installing PR is good now.

Load any page that has a 'head'. All of your tags should appear as expected.

means any Type of Page (Singel Article, Blog, List, …) and tags

, etc.?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 21 Jan 2017

@franz-wohlkoenig Yes. No.

Any page means any page that uses the head renderer. This is the head renderer. It renders most of the contents of the tag on all typical pages (maybe not error pages, for example).

Tags means tags within the head tag which this renderer is responsible for rendering. So not

but more like , <script>, etc.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Jan 2017

@okonomiyaki3000 at type login-form is a difference between

without PR

withoutpr

with PR

withpr
If this is expected, will test further.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Jan 2017

@franz-wohlkoenig This difference is not expected and probably has nothing to do with these changes. It seems your 'without PR' version has some other differences. I believe these additional lines are supposed to be here. This is translation text used by the form when a field is invalid.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Jan 2017

@okonomiyaki3000 tested 10 different types (category-list, singel article, ...), they look similar (different order of <script__, but same number of lines). What you mean " It seems your 'without PR' version has some other differences", go on testing isn't necessary?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 23 Jan 2017

@franz-wohlkoenig Upon closer examination, I now see why these differences happened. Actually, there's nothing wrong with your 'without PR' version. Two things I was doing a little wrong or differently.

First, there's something called 'script options' which just a block of json, not an actual script. This is normally placed ahead of any normal script tags but I had it in the wrong order. Not sure if it would affect anything but now I've put it first just in case.

Second, I don't remember why I did this but for some reason I was explicitly adding the JText translations. This isn't necessary as they are in the script options and get loaded by the core js. I guess I must not have noticed that before. Anyway, if you check again, I expect you'll see fewer (if any) differences before and after the PR.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 23 Jan 2017

@okonomiyaki3000 so all checked types test again?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 23 Jan 2017

@franz-wohlkoenig If you're concerned about the order of script tags, by all means, check again. I expect the order to be the same with or without the PR now.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 23 Jan 2017

@okonomiyaki3000 can you please check before and after PR at example "Single Contact" if its okay to test on:

without PR

withoutpr

with PR

withpr

avatar okonomiyaki3000
okonomiyaki3000 - comment - 23 Jan 2017

@franz-wohlkoenig Thanks! These look pretty much the same to me. I see some whitespace differences which nobody should care about and the order of attributes in some tags which also doesn't matter. Also, you can see that I've removed the / at the end of link tags. This is intentional. So this is working as expected.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 23 Jan 2017

I have tested this item ✅ successfully on eec6774

Tested on:

Single Contact
Contact Categories
News Feed Categories
News Feed Category
Single News Feed
Search
Archived Articles
Single Article
Article Category Blog
Article Category List
Featured Articles
Article Categories
Contact Single Category
Login Form
User Profile
Edit User Profile
Registration Form
Username Reminder Request
Password Reset
Featured Contacts
Smart Search
Compact Tagged
Tagged Items
All Tags

Thanks for help by #7680, @Bakual

Test: Made a Screenshot of ... without PR. Apply PR, clear Cache, see Sourcecode beside Screenshot.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12413.

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 23 Jan 2017 - Tested successfully
avatar okonomiyaki3000
okonomiyaki3000 - comment - 23 Jan 2017
avatar csthomas
csthomas - comment - 11 Feb 2017

Avoiding using string concatenation on long strings by pushing to an array and imploding at the end. This is generally better for performance although, in this case, it may be offset somewhat by the overhead of calling additional functions.

According to:

concatenation is faster.
IMHO each function which you create should create and return string.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 13 Feb 2017

@csthomas Interesting. Goes against what I've always heard on this issue.

avatar mbabker
mbabker - comment - 30 May 2017

In 4.0 we now have 3 separate renderer objects (scripts, styles, meta). Is this something that's still worth finishing up on?

avatar mbabker
mbabker - comment - 30 May 2017

// @dgt41

avatar okonomiyaki3000
okonomiyaki3000 - comment - 30 May 2017

@mbabker Maybe this is not needed then. I'm satisfied if this kind of thing is moving in the right direction even if it might take a little while.

avatar mbabker
mbabker - comment - 31 May 2017

So let's go ahead and close this then.

avatar mbabker mbabker - change - 31 May 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-05-31 08:25:34
Closed_By mbabker
avatar mbabker mbabker - close - 31 May 2017

Add a Comment

Login with GitHub to post a comment