Joomla! Issue Tracker - CMS

Download
Launch
Feeling Lucky

[#17051] - Fix browser / version detection for firefox

? ? ? Failure

User tests: Successful: Unsuccessful:

avatar matrikular
matrikular
10 Jul 2017

Summary of Changes

Change the regular expression to detect the correct Firefox version.

Testing Instructions

Paste the following code snippet into the index.php of the protostar template and open the page in Firefox:

jimport('joomla.environment.browser');

$navigator = JBrowser::getInstance();

var_dump($navigator->getAgentString(), $navigator->getVersion());

Expected result

The result should show the correct Firefox version e.g.

array (size=2)
  'UserAgent:' => string 'Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0' (length=78)
  'Version:' => string '54.0' (length=4)

Actual result

The version number is "always" 5.0.

array (size=2)
  'UserAgent:' => string 'Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0' (length=78)
  'Version:' => string '5.0' (length=3)

Edit: maybe the agent variable should be changed from "mozilla" to "firefox", too? Can we do this for 3.8 or not until 4.x?

avatar matrikular matrikular - open - 10 Jul 2017
avatar matrikular matrikular - change - 10 Jul 2017
Status New ⇒ Pending
avatar joomla-cms-bot joomla-cms-bot - change - 10 Jul 2017
Category ⇒ Libraries
avatar matrikular matrikular - change - 10 Jul 2017
The description was changed
avatar matrikular matrikular - edited - 10 Jul 2017
avatar matrikular matrikular - change - 10 Jul 2017
The description was changed
avatar matrikular matrikular - edited - 10 Jul 2017
avatar franz-wohlkoenig franz-wohlkoenig - test_item - 10 Jul 2017 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 10 Jul 2017

I have tested this item ✅ successfully on 886dd25
Without PR:
without
With PR:
with

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17051.
avatar gorgonz
gorgonz - comment - 10 Jul 2017

verified successfully with linux, result is
string(68) "Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0" string(3) "5.0"

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 10 Jul 2017

@gorgonz i have altered your Test at Issue Tracker.

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

avatar franz-wohlkoenig franz-wohlkoenig - alter_testresult - 10 Jul 2017 - gorgonz: Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 10 Jul 2017
Status Pending ⇒ Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 10 Jul 2017

RTC after two successful tests.

avatar matrikular
matrikular - comment - 10 Jul 2017

Before this gets merged I would love to get some feedback as to whether or not to change the agent variable to firefox instead of mozilla.

@mbabker @wilsonge thoughts?

avatar rdeutz rdeutz - change - 11 Jul 2017
Milestone Added:
avatar wilsonge
wilsonge - comment - 17 Jul 2017

Are there any user agents with Mozzila in that don't have FF in

avatar mbabker
mbabker - comment - 29 Jul 2017

@matrikular any feedback on George's question?

avatar matrikular
matrikular - comment - 30 Jul 2017

@mbabker As mentioned in the description / comments both here and in your PR, "all" User Agents have got Mozilla in it. For that, our browser class would currently identify every browser as "Mozilla/5.0" (mozilla) if no other (more strict) rule applies before, of course.

If the browser is a clone / derivative of Firefox, you could find something like the following in the User Agent string: "Mozilla/5.0 (Windows NT 5.2; Win64; x64; rv:39.0) Gecko/20100101 Firefox/39.0 Waterfox/39.0".
In this example, adding a regular expression for Firefox alone would match and return "firefox", even though it is Waterfox or another clone / derivative (e.g Pale Moon).

In short; Yes, there are User Agents with Mozilla in that don't have FF in. Sadly, if we want to get it right, it seems that this is not easily covered with adding yet another regular expression / rule. Well, at least not for the long run.

That said, I would like to propose the following changes:

  • Move the regular expression for Mozilla to the very bottom / end
  • Add regular expressions for Firefox and some of the known clones / derivatives
  • Check and re-order the existing regular expressions to make sure that we don't have any "false positives"
  • [...]
avatar mbabker
mbabker - comment - 30 Jul 2017

Those changes sound reasonable to me.

avatar wilsonge
wilsonge - comment - 30 Jul 2017

Me too :)

avatar mbabker
mbabker - comment - 31 Jul 2017

To merge this PR, let's do 3 things:

  1. Re-sync with staging
  2. Re-add the elseif (preg_match('|Mozilla/([0-9.]+)|', $this->agent, $version)) condition as the last condition in the browser check
  3. In JBrowserTest remove lines 154 thru 157 as with this PR the Firefox UAs I added for the tests there should now correctly be parsed

We can work on further improving from here but we really REALLY should (finally) fix Firefox detection.

avatar matrikular matrikular - change - 31 Jul 2017
Labels Added: Conflicting Files
avatar matrikular matrikular - change - 31 Jul 2017
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 31 Jul 2017
Category Libraries ⇒ Libraries Unit Tests
avatar matrikular matrikular - change - 31 Jul 2017
Labels Added: ?
avatar mbabker mbabker - change - 31 Jul 2017
Status Ready to Commit ⇒ Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 ⇒ 2017-07-31 22:31:29
Closed_By ⇒ mbabker
Labels Removed: Conflicting Files
avatar mbabker mbabker - close - 31 Jul 2017
avatar mbabker mbabker - merge - 31 Jul 2017

Add a Comment

Login with GitHub to post a comment