Skip to content
This repository has been archived by the owner on Jan 25, 2023. It is now read-only.

7.0.0 #78

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

7.0.0 #78

wants to merge 20 commits into from

Conversation

gabrielbull
Copy link
Member

@gabrielbull gabrielbull commented Apr 6, 2017

@TomMettam Continuity of #76

TomMettam and others added 6 commits March 31, 2017 17:07
* Removed $isRobot, setIsRobot(), getIsRobot(), isRobot(), checkBrowserRobot()
* Replaced above with detectScriptedAgent()
* Added a new ScriptedAgent class with detection for bots, spiders, etc
* Added isWebkit(), getIsWebkit(), setIsWebkit()

  NOTE: TODO: Should add proper browser engine detection (Webkit, Gecko, Trident, etc)

* Added $isTwitterWebView, setIsTwitterWebView(), getIsTwitterWebView(), isTwitterWebView();
* Added browser detection for UC Browser
* Added browser detection for NSPlayer (Windows Media Player)
* Added OS detection for NSPlayer (Windows Media Player)
* Added browser detection for Microsoft Office
* Added browser detection for the Apple News app
* Added browser detection for the Dalvik (Android) OS
* Moved wkHTMLtoPDF to scripted agents and removed the test accordingly
* Moved GoogleBot to scripted agents
* Moved Slurp to scripted agents
* Moved W3CValidator to scripted agents
* Moved MSNBot to scripted agents
* Renamed "Navigator" to "Android Navigator" for clarity
* Strip linebreaks in setVersion (fixes a failng test)
* Added .idea to the gitignore (I use PHPstorm, don't be a hater)

TODO: Tests for new class
Instead of: Match everything except ^ ;\),
Match: a-z, A-Z, 0-9, and . only.

Remove dirty fudge added to remove the stray \n

Now passes tests without said fudge
@@ -1,5 +1,9 @@
# CHANGELOG

## 7.0.0 (released 2017-xx-xx)

- ...
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to add changelog

@@ -67,12 +64,12 @@ class Browser
/**
* @var bool
*/
private $isRobot = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to know if is a scripted agent from here, maybe isScriptedAgent

src/Browser.php Outdated
* @param bool $isRobot
* @return ScriptedAgent|bool
*/
public function detectScriptedAgent()
Copy link
Member Author

Choose a reason for hiding this comment

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

Move this to the ScriptedAgent class like Browser, OS, Device, etc

Choose a reason for hiding this comment

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

The reasoning here is that detectScriptedAgent can provide a replacement for $isRobot in this class. This function provides a quick pre-scan to avoid in-depth substring searches in ScriptedAgent if not necessary.

{
public function testExample()
{
$this->assertNotFalse(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to add some tests to assert this is working

Choose a reason for hiding this comment

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

#82

{
public function testExample()
{
$this->assertNotFalse(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to add some tests to assert this is working

Choose a reason for hiding this comment

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

#82

@gabrielbull gabrielbull added this to the 7.0.0 milestone Apr 6, 2017
TomMettam and others added 5 commits April 6, 2017 15:06
…rsion

This adds a lookup table for Edge. This will require some maintenance.

If the version cannot be resolved, the browser name "EdgeHTML" is returned rather than "Edge", so that the version number is not misleading.

Also fixed regex for edge.
stripos($ua, 'msn') !== false ||
stripos($ua, 'Google Web Preview') !== false ||
stripos($ua, 'ips-agent') !== false ||
(stripos($ua, 'Chrome/51.0.2704.103') !== false && !isset($_SERVER['HTTP_UPGRADE_INSECURE_REQUESTS']) && stristr($_SERVER['HTTP_ACCEPT_LANGUAGE'], "ru-RU") !== false) //ICQ Preview
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't those checks be done in the ScriptedAgentDetector class?

Choose a reason for hiding this comment

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

I anticipated this to be called from $browser in order to be a substitute of $isRobot.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could still check from the browser class if it is a scripted agent by checking the result of ScriptedAgentDetector. I think it would be cleaner to separate the two and link them with methods instead of having this check in the browser class.

Choose a reason for hiding this comment

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

Ok, I'm on board with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, do you want to make the change?

src/Browser.php Outdated
@@ -38,6 +38,7 @@ class Browser
const GSA = 'Google Search Appliance';
const YANDEX = 'Yandex';
const EDGE = 'Edge';
const EDGE_HTML = 'EdgeHTML';
Copy link
Member Author

Choose a reason for hiding this comment

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

This could lead to use error, I'd prefer to still have Edge as the browser and the version be unknown if it is unknown.

Choose a reason for hiding this comment

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

Agreed. I am working on engine detection also, so that fits.

if (isset($version[1])) {
self::$browser->setVersion((float)$version[1]);
preg_match('/Edge[\\/ \\(]([a-zA-Z\\d\\.]*)/i', self::$userAgentString, $matches);
if (sizeof($matches)>1) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We should check if the version is between the range and provide the closest version if it is.

Example:

"12.10049" => "0.10.10049",
"12.10051" => "0.11.10051",

If we detect version 12.10050, we should fallback to the latest available version which would be 0.10.10049. This way, while it may not be 100% accurate, it will provide a fallback to the versions missing in the table.

Copy link

@TomMettam TomMettam left a comment

Choose a reason for hiding this comment

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

--

@peter279k
Copy link

It looks like the StyleCI build is failed.

That is, the coding style check is failed, and I think this should be fixed.

*/
public static function checkBrowserUCBrowser()
{
// Navigator
Copy link

Choose a reason for hiding this comment

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

Do you need this comment in each function?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants