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

new provider mimmi20 #111

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

new provider mimmi20 #111

wants to merge 2 commits into from

Conversation

ThaDafinser
Copy link
Owner

@ThaDafinser ThaDafinser commented May 11, 2017

@mimmi20 can u do a review, if i map the correct fields?

'model' => true,
'brand' => true,
'type' => true,
'isMobile' => false,
Copy link

Choose a reason for hiding this comment

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

this should be true.

'bot' => [
'isBot' => true,
'name' => true,
'type' => false,
Copy link

Choose a reason for hiding this comment

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

this should be true


'renderingEngine' => [
'name' => true,
'version' => false,
Copy link

Choose a reason for hiding this comment

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

this should be true

private function hydrateBot(Model\Bot $bot, BrowserInterface $browserRaw)
{
$bot->setIsBot(true);
$bot->setName($this->getRealResult($browserRaw->getName()));
Copy link

Choose a reason for hiding this comment

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

add this: $bot->setType($this->getRealResult($browserRaw->getType()->getName()));

Copy link
Owner Author

Choose a reason for hiding this comment

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

shouldnt this be $browserRaw->getType()->getType() ? The result there is e.g. bot-syndication-reader

Copy link

Choose a reason for hiding this comment

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

Please use getName. getType is used for the caching


if ($deviceRaw->getPointingMethod() === 'touchscreen') {
$device->setIsTouch(true);
}
Copy link

Choose a reason for hiding this comment

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

add this: $device->setIsMobile($deviceRaw->getType()->isMobile())

/*
* Bot detection
*/
if ($parser->getBrowser()->getType()->getType() === 'bot') {
Copy link

Choose a reason for hiding this comment

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

please use this: if ($parser->getBrowser()->getType()->isBot()) {

@ThaDafinser
Copy link
Owner Author

@mimmi20 got all things covered now, will update the PR tomorrow 👍

@ThaDafinser
Copy link
Owner Author

@mimmi20 can u do a last review?

The question is now...

  • creating v2.x (and do maintenance for 1.x)
  • or just create a new v1.x with higher requirements...
  • or leave 5.6 requirement (since it will fail, when your package is required)

Need to think about that a bit

@mimmi20
Copy link

mimmi20 commented May 19, 2017

@ThaDafinser I do not have the time to maintain two versions. I am still working on the complexity, and I am still adding devices and browsers which are not detected yet. I dont want to add PHP 5.6 to my supported PHP versions because I want to add PHP 7+ features in the future.

@ThaDafinser
Copy link
Owner Author

@mimmi20 leave your package as php 7+.

it was more about this package ;-)

@mimmi20
Copy link

mimmi20 commented Dec 4, 2017

@ThaDafinser Any news here?

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.

2 participants