Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: Fix undefined offset: 1 #17139

Merged
merged 4 commits into from Aug 12, 2017
Merged

Regression: Fix undefined offset: 1 #17139

merged 4 commits into from Aug 12, 2017

Conversation

Quy
Copy link
Contributor

@Quy Quy commented Jul 15, 2017

Pull Request for Issue #16970 .
Fix regression for Issue #15228 .

Summary of Changes

Revert to previous individual preg_match rather than combine them into 1.

Testing Instructions

Check your PHP error log.

or

Create the following PHP script and run it. 1st preg_match( (previous) outputs no error. 2nd preg_match( (current) outputs error.

<?php

$agent = "Mozilla/5.0 (iPad; CPU OS 10_3_2 like Mac OS X) AppleWebKit/603.1.30 (KHTML, like Gecko) CriOS/59.0.3071.102 Mobile/14F89 Safari/602.1";

echo 'v3.6.5 ';
preg_match('|CriOS[/ ]([0-9.]+)|', $agent, $version);
var_dump($version);
list ($majorVersion, $minorVersion) = explode('.', $version[1]);

echo 'v3.7.3 ';
preg_match('/Chrome[\/ ]([0-9.]+)|CrMo[\/ ]([0-9.]+)|CriOS[\/ ]([0-9.]+)/i', $agent, $version);
var_dump($version);
list ($majorVersion, $minorVersion) = explode('.', $version[1]);

?>

results (1st array is v3.6.5 and 2nd array is v3.7.3):

v3.6.5 array(2) {
  [0]=>
  string(19) "CriOS/59.0.3071.102"
  [1]=>
  string(13) "59.0.3071.102"
}
v3.7.3 array(4) {
  [0]=>
  string(19) "CriOS/59.0.3071.102"
  [1]=>
  string(0) ""
  [2]=>
  string(0) ""
  [3]=>
  string(13) "59.0.3071.102"
}
<br />
<b>Notice</b>:  Undefined offset: 1 in <b>C:\xampp\htdocs\agent.php</b> on line <b>14</b><br />

Expected result

No PHP Notice

Actual result

PHP message: PHP Notice: Undefined offset: 1 in /libraries/joomla/environment/browser.php on line 282\n

Documentation Changes Required

None

@Quy Quy changed the title Fix undefined offset: 1 Regression: Fix undefined offset: 1 Jul 15, 2017
Copy link
Contributor

@photodude photodude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an appropriate correction. On code review and preg_match testing

@Quy
Copy link
Contributor Author

Quy commented Jul 16, 2017

@rdeutz I have two v3.7.3 installs where the second install has this PR. The first install is littered with Fix undefined offset: 1 daily in the PHP error log file and not in the second install. This is a regression from v.3.7.2.

@photodude
Copy link
Contributor

photodude commented Jul 16, 2017

@Quy we should add new UA to the unit tests for the noted regression(s) here. (UA's listed in the previous version of this PR #16989)

This is a merge by code review / unit tests pass. No easy way to user test the browser cases.

@mbabker
Copy link
Contributor

mbabker commented Jul 16, 2017

There are no unit tests for JBrowser yet (#17140 starts it but it's a very limited subset). If someone wants to generate a list, we can add them as part of that PR or later on after that one's merged.

@Quy
Copy link
Contributor Author

Quy commented Jul 16, 2017

@photodude I don't know how to do it. Would you mind doing it? Thanks.

@photodude
Copy link
Contributor

@mbabker I'll put a list together for that PR.

@photodude
Copy link
Contributor

@Quy can you fix the merge conflict

@Brian5600
Copy link

Hello
I tried to change browser.php with the changes about Chrome today like shown in "Files changed", and the error went away from Chrome browser (ver. 60.x) on Ipad (Ios 10.3.2). I am on Joomla 3.7.4.
I have not tested regarding to IE. I am on Win7, IE 11, and don't see any issues with "default" code.
Hope it will be fixed in Joomla.
Thanks @Brian5600.

@Quy
Copy link
Contributor Author

Quy commented Aug 10, 2017

@Brian5600 Please mark it a successful test here: https://issues.joomla.org/tracker/joomla-cms/17139

@ghost
Copy link

ghost commented Aug 10, 2017

@Quy i altered successfully Test for @Brian5600

@Brian5600
Copy link

@franz-wohlkoenig @Quy thanks.
I also tested with success in Chrome installed on Win8 today.


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

@photodude
Copy link
Contributor

@mbabker @wilsonge Could either of you, or someone else, consider marking RTC for this since there are two reported successful tests (also drone seems to need a restart here).

@ghost
Copy link

ghost commented Aug 12, 2017

@photodude who is second successfully Test?

@photodude
Copy link
Contributor

@franz-wohlkoenig I guess I miss read it, I thought you had done a test.

Really this is a "by code review" item as there is no good/easy way to test this and there are not really any unit tests for it either.

I'll mark approved by code review.

Copy link
Contributor

@photodude photodude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved by code review

@ghost
Copy link

ghost commented Aug 12, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 12, 2017
@ghost
Copy link

ghost commented Aug 12, 2017

@photodude i altered your Review as successfully Test at Issue Tracker.

@mbabker mbabker added this to the Joomla 3.8.0 milestone Aug 12, 2017
@mbabker mbabker merged commit 4a5a043 into joomla:staging Aug 12, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 12, 2017
@Quy Quy deleted the browser2 branch August 12, 2017 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants