From c6a60da567d399a7b6392c4229c407a561e1af46 Mon Sep 17 00:00:00 2001 From: Martin Georgiev Date: Wed, 24 Oct 2018 14:42:18 +0100 Subject: [PATCH] Housekeeping fixes (#7) Apply fixes reported by static analysis. Ensure global functions are not overloaded. Decouple controller from ContainerAware. --- composer.json | 24 ++-- phpstan.neon | 2 + phpunit.xml | 5 +- src/Controller/DeviceController.php | 39 +++++-- .../AlphaVisitorTrackingExtension.php | 1 - src/DependencyInjection/Configuration.php | 2 + src/Entity/Seed.php | 10 +- src/Entity/Session.php | 109 +++++------------- .../VisitorTrackingSubscriber.php | 62 ++++------ src/Manager/DeviceFingerprintManager.php | 8 +- src/Resources/config/controllers.yml | 9 ++ src/Resources/config/services.yml | 3 +- .../Manager/DeviceFingerprintManagerTest.php | 9 +- 13 files changed, 128 insertions(+), 155 deletions(-) create mode 100644 phpstan.neon create mode 100644 src/Resources/config/controllers.yml diff --git a/composer.json b/composer.json index 7be45ca..94343b0 100644 --- a/composer.json +++ b/composer.json @@ -29,6 +29,7 @@ "require": { "php": ">=7.1", + "ext-json": "*", "doctrine/doctrine-bundle": "~1.6", "doctrine/orm": "~2.1", "stof/doctrine-extensions-bundle": "*", @@ -41,38 +42,39 @@ "behat/symfony2-extension": "^2.1", "friendsofphp/php-cs-fixer": "*", "jakub-onderka/php-parallel-lint": "^1.0", - "phpstan/phpstan": "^0.10.2", - "phpunit/phpunit": "^5.7", - "satooshi/php-coveralls": "^2.0", + "php-coveralls/php-coveralls": "^2.1", + "phpstan/phpstan": "^0.10.5", + "phpstan/phpstan-phpunit": "^0.10.0", + "phpunit/phpunit": "^7.4", "sensiolabs/security-checker": "^4.1", "symfony/phpunit-bridge": "^3.0|^4.0" }, "scripts": { "check-code-style": [ - "bin/php-cs-fixer fix --config='./.php_cs' --show-progress=none --dry-run --no-interaction --diff -v" + "php-cs-fixer fix --config='./.php_cs' --show-progress=none --dry-run --no-interaction --diff -v" ], "check-security": [ - "bin/security-checker security:check" + "security-checker security:check" ], "fix-code-style": [ - "bin/php-cs-fixer fix --config='./.php_cs' --show-progress=none --no-interaction --diff -v" + "php-cs-fixer fix --config='./.php_cs' --show-progress=none --no-interaction --diff -v" ], "run-static-analysis": [ - "bin/phpstan analyse --level=1 src/" + "phpstan analyse --level=7 src/" ], "run-static-analysis-including-tests": [ "@run-static-analysis", - "bin/phpstan analyse --level=1 tests/" + "phpstan analyse --level=3 tests/" ], "run-tests": [ - "bin/phpunit" + "phpunit" ], "run-tests-with-clover": [ - "bin/phpunit --coverage-clover build/logs/clover.xml" + "phpunit --coverage-clover build/logs/clover.xml" ], "validate-files": [ - "bin/parallel-lint --exclude vendor --exclude bin ." + "parallel-lint --exclude vendor --exclude bin ." ] }, diff --git a/phpstan.neon b/phpstan.neon new file mode 100644 index 0000000..5cba2c4 --- /dev/null +++ b/phpstan.neon @@ -0,0 +1,2 @@ +includes: + - vendor/phpstan/phpstan-phpunit/extension.neon \ No newline at end of file diff --git a/phpunit.xml b/phpunit.xml index ff34496..b265f31 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -6,11 +6,10 @@ convertWarningsToExceptions="true" stopOnFailure="false" bootstrap="vendor/autoload.php" - verbose="true" - syntaxCheck="true"> + verbose="true"> - + ./tests diff --git a/src/Controller/DeviceController.php b/src/Controller/DeviceController.php index 5ba331a..0de7c0a 100644 --- a/src/Controller/DeviceController.php +++ b/src/Controller/DeviceController.php @@ -6,17 +6,38 @@ use Alpha\VisitorTrackingBundle\Entity\Device; use Alpha\VisitorTrackingBundle\Entity\Session; -use Symfony\Bundle\FrameworkBundle\Controller\Controller; +use Alpha\VisitorTrackingBundle\Manager\DeviceFingerprintManager; +use Alpha\VisitorTrackingBundle\Storage\SessionStore; +use Doctrine\ORM\EntityManager; +use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; -class DeviceController extends Controller +class DeviceController { + private $entityManager; + + private $logger; + + private $sessionStore; + + private $deviceFingerprintManager; + + public function __construct( + EntityManager $entityManager, + LoggerInterface $logger, + SessionStore $sessionStore, + DeviceFingerprintManager $deviceFingerprintManager + ) { + $this->entityManager = $entityManager; + $this->logger = $logger; + $this->sessionStore = $sessionStore; + $this->deviceFingerprintManager = $deviceFingerprintManager; + } + public function fingerprintAction(Request $request): Response { - $em = $this->getDoctrine()->getManager(); - - $session = $this->get('alpha.visitor_tracking.storage.session')->getSession(); + $session = $this->sessionStore->getSession(); $device = null; if ($session instanceof Session) { @@ -30,12 +51,12 @@ public function fingerprintAction(Request $request): Response $device->setFingerprint($request->getContent()); $device->setSession($session); - $this->get('alpha.visitor_tracking.manager.device_fingerprint')->generateHashes($device); + $this->deviceFingerprintManager->generateHashes($device); - $em->persist($device); - $em->flush($device); + $this->entityManager->persist($device); + $this->entityManager->flush($device); - $this->get('logger')->debug(sprintf('A new device fingerprint was added with the id %d.', $device->getId())); + $this->logger->debug(\sprintf('A new device fingerprint was added with the id %d.', $device->getId())); return new Response('', 201); } diff --git a/src/DependencyInjection/AlphaVisitorTrackingExtension.php b/src/DependencyInjection/AlphaVisitorTrackingExtension.php index 83b281a..eb9593b 100644 --- a/src/DependencyInjection/AlphaVisitorTrackingExtension.php +++ b/src/DependencyInjection/AlphaVisitorTrackingExtension.php @@ -16,7 +16,6 @@ public function load(array $configs, ContainerBuilder $container): void $configuration = new Configuration(); $config = $this->processConfiguration($configuration, $configs); - $loader = new Loader\YamlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config')); $loader->load('services.yml'); diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 31cb9b8..adf9e96 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -15,6 +15,7 @@ public function getConfigTreeBuilder(): TreeBuilder $treeBuilder = new TreeBuilder(); $rootNode = $treeBuilder->root('alpha_visitor_tracking'); + \assert($rootNode instanceof ArrayNodeDefinition); $rootNode->append($this->createSubscriberNode()); return $treeBuilder; @@ -23,6 +24,7 @@ public function getConfigTreeBuilder(): TreeBuilder private function createSubscriberNode(): ArrayNodeDefinition { $root = (new TreeBuilder())->root('session_subscriber'); + \assert($root instanceof ArrayNodeDefinition); $root->addDefaultsIfNotSet(); $root->children()->arrayNode('firewall_blacklist') diff --git a/src/Entity/Seed.php b/src/Entity/Seed.php index cfeabeb..8abd8ac 100644 --- a/src/Entity/Seed.php +++ b/src/Entity/Seed.php @@ -36,15 +36,11 @@ class Seed /** * @var string + * * @ORM\Column(type="string") */ protected $value; - /** - * @param string $name - * @param int $numberOfValues - * @param array $weights - */ public function __construct(string $name, int $numberOfValues, ?array $weights = null) { $this->name = $name; @@ -102,7 +98,7 @@ public function getValue() private function setValue(int $numberOfValues, ?array $weights = null): void { if ($weights === null) { - $weights = array_fill(1, $numberOfValues, 1); + $weights = \array_fill(1, $numberOfValues, 1); } if (\count($weights) !== $numberOfValues) { @@ -111,7 +107,7 @@ private function setValue(int $numberOfValues, ?array $weights = null): void // This does not need to be cryptographically secure, mt_rand() is roughly 2x faster than random_int(). /** @noinspection RandomApiMigrationInspection */ - $random = mt_rand(1, array_sum($weights)); + $random = \mt_rand(1, (int) \array_sum($weights)); $total = 0; foreach ($weights as $seed => $weight) { diff --git a/src/Entity/Session.php b/src/Entity/Session.php index 2dbe1dc..be7d707 100644 --- a/src/Entity/Session.php +++ b/src/Entity/Session.php @@ -25,7 +25,7 @@ class Session protected $id; /** - * @var Lifetime + * @var Lifetime|null * * @ORM\ManyToOne(targetEntity="Lifetime", inversedBy="sessions") */ @@ -161,11 +161,9 @@ public function getIp() } /** - * @param string $ip - * - * @return Session + * @param string $ip */ - public function setIp($ip) + public function setIp($ip): self { $this->ip = $ip; @@ -181,11 +179,9 @@ public function getReferrer() } /** - * @param string $referrer - * - * @return Session + * @param string $referrer */ - public function setReferrer($referrer) + public function setReferrer($referrer): self { $this->referrer = $referrer; @@ -201,11 +197,9 @@ public function getUserAgent() } /** - * @param string $userAgent - * - * @return Session + * @param string $userAgent */ - public function setUserAgent($userAgent) + public function setUserAgent($userAgent): self { $this->userAgent = $userAgent; @@ -221,11 +215,9 @@ public function getQueryString() } /** - * @param string $queryString - * - * @return Session + * @param string $queryString */ - public function setQueryString($queryString) + public function setQueryString($queryString): self { $this->queryString = $queryString; @@ -241,11 +233,9 @@ public function getUtmSource() } /** - * @param string $utmSource - * - * @return Session + * @param string $utmSource */ - public function setUtmSource($utmSource) + public function setUtmSource($utmSource): self { $this->utmSource = $utmSource; @@ -261,11 +251,9 @@ public function getUtmMedium() } /** - * @param string $utmMedium - * - * @return Session + * @param string $utmMedium */ - public function setUtmMedium($utmMedium) + public function setUtmMedium($utmMedium): self { $this->utmMedium = $utmMedium; @@ -281,11 +269,9 @@ public function getUtmCampaign() } /** - * @param string $utmCampaign - * - * @return Session + * @param string $utmCampaign */ - public function setUtmCampaign($utmCampaign) + public function setUtmCampaign($utmCampaign): self { $this->utmCampaign = $utmCampaign; @@ -301,11 +287,9 @@ public function getUtmTerm() } /** - * @param string $utmTerm - * - * @return Session + * @param string $utmTerm */ - public function setUtmTerm($utmTerm) + public function setUtmTerm($utmTerm): self { $this->utmTerm = $utmTerm; @@ -321,11 +305,9 @@ public function getUtmContent() } /** - * @param string $utmContent - * - * @return Session + * @param string $utmContent */ - public function setUtmContent($utmContent) + public function setUtmContent($utmContent): self { $this->utmContent = $utmContent; @@ -341,11 +323,9 @@ public function getLoanTerm() } /** - * @param string $loanTerm - * - * @return Session + * @param string $loanTerm */ - public function setLoanTerm($loanTerm) + public function setLoanTerm($loanTerm): self { $this->loanTerm = $loanTerm; @@ -361,11 +341,9 @@ public function getRepApr() } /** - * @param string $repApr - * - * @return Session + * @param string $repApr */ - public function setRepApr($repApr) + public function setRepApr($repApr): self { $this->repApr = $repApr; @@ -380,12 +358,7 @@ public function getCreated() return $this->created; } - /** - * @param \DateTime $created - * - * @return Session - */ - public function setCreated(\DateTime $created) + public function setCreated(\DateTime $created): self { $this->created = $created; @@ -393,35 +366,25 @@ public function setCreated(\DateTime $created) } /** - * @return Lifetime + * @return Lifetime|null */ public function getLifetime() { return $this->lifetime; } - /** - * @param Lifetime $lifetime - * - * @return Session - */ - public function setLifetime(?Lifetime $lifetime = null) + public function setLifetime(?Lifetime $lifetime = null): self { $this->lifetime = $lifetime; - if ($lifetime instanceof Lifetime && !$this->lifetime->getSessions()->contains($this)) { + if ($lifetime instanceof Lifetime && $this->lifetime instanceof Lifetime && !$this->lifetime->getSessions()->contains($this)) { $this->lifetime->addSession($this); } return $this; } - /** - * @param PageView $pageView - * - * @return Session - */ - public function addPageView(PageView $pageView) + public function addPageView(PageView $pageView): self { if ($pageView->getSession() !== $this) { $pageView->setSession($this); @@ -434,19 +397,14 @@ public function addPageView(PageView $pageView) return $this; } - /** - * @param PageView $pageViews - * - * @return $this - */ - public function removePageView(PageView $pageViews) + public function removePageView(PageView $pageViews): self { $this->pageViews->removeElement($pageViews); return $this; } - public function addDevice(Device $device) + public function addDevice(Device $device): self { if ($device->getSession() !== $this) { $device->setSession($this); @@ -459,12 +417,7 @@ public function addDevice(Device $device) return $this; } - /** - * @param Device $device - * - * @return $this - */ - public function removeDevice(Device $device) + public function removeDevice(Device $device): self { $this->devices->removeElement($device); diff --git a/src/EventListener/VisitorTrackingSubscriber.php b/src/EventListener/VisitorTrackingSubscriber.php index 370ccad..9b4b362 100644 --- a/src/EventListener/VisitorTrackingSubscriber.php +++ b/src/EventListener/VisitorTrackingSubscriber.php @@ -39,29 +39,21 @@ class VisitorTrackingSubscriber implements EventSubscriberInterface protected const COOKIE_SESSION_TTL = '+2 years'; - /** - * @var EntityManager - */ - protected $em; - - /** - * @var SessionStore - */ + private $entityManager; + private $sessionStore; - /** - * @var array - */ private $firewallBlacklist; - /** - * @var FirewallMap - */ private $firewallMap; - public function __construct(EntityManager $em, SessionStore $sessionStore, array $firewallBlacklist, FirewallMap $firewallMap) - { - $this->em = $em; + public function __construct( + EntityManager $entityManager, + SessionStore $sessionStore, + array $firewallBlacklist, + FirewallMap $firewallMap + ) { + $this->entityManager = $entityManager; $this->sessionStore = $sessionStore; $this->firewallBlacklist = $firewallBlacklist; $this->firewallMap = $firewallMap; @@ -84,7 +76,7 @@ public function onKernelRequest(GetResponseEvent $event): void } if ($request->cookies->has(self::COOKIE_SESSION)) { - $session = $this->em->getRepository(Session::class)->find($request->cookies->get(self::COOKIE_SESSION)); + $session = $this->entityManager->getRepository(Session::class)->find($request->cookies->get(self::COOKIE_SESSION)); if ($session instanceof Session && (!$this->requestHasUTMParameters($request) || $this->sessionMatchesRequestParameters($request))) { $this->sessionStore->setSession($session); @@ -112,6 +104,7 @@ public function onKernelResponse(FilterResponseEvent $event): void $response = $event->getResponse(); if (!$request->cookies->has(self::COOKIE_LIFETIME)) { + \assert($session->getLifetime() instanceof Lifetime); $response->headers->setCookie(new Cookie(self::COOKIE_LIFETIME, $session->getLifetime()->getId(), new \DateTime('+2 years'), '/', null, false, false)); } @@ -127,17 +120,7 @@ public function onKernelResponse(FilterResponseEvent $event): void $pageView->setUrl($request->getUri()); $session->addPageView($pageView); - $this->em->flush($session); - } - - /** - * @deprecated Use the SessionStore directly. - * - * @return Session|null - */ - public function getSession(): ?Session - { - return $this->sessionStore->getSession(); + $this->entityManager->flush($session); } protected function requestHasUTMParameters(Request $request): bool @@ -163,25 +146,28 @@ protected function setUTMSessionCookies(Request $request, Response $response): v private function generateSessionAndLifetime(Request $request): void { - $lifetime = false; + $lifetime = null; if ($request->cookies->has(self::COOKIE_LIFETIME)) { - $lifetime = $this->em->getRepository(Lifetime::class)->find($request->cookies->get(self::COOKIE_LIFETIME)); + $lifetime = $this->entityManager->getRepository(Lifetime::class)->find($request->cookies->get(self::COOKIE_LIFETIME)); } - if (!$lifetime) { + if (!$lifetime instanceof Lifetime) { $lifetime = new Lifetime(); - $this->em->persist($lifetime); + $this->entityManager->persist($lifetime); + $this->entityManager->flush($lifetime); } $session = new Session(); - $this->em->persist($session); + $this->entityManager->persist($session); $session->setIp($request->getClientIp() ?: ''); - $session->setReferrer($request->headers->get('Referer') ?: ''); - $session->setUserAgent($request->headers->get('User-Agent') ?: ''); + $referer = $request->headers->get('Referer'); + $session->setReferrer(\is_string($referer) ? $referer : ''); + $userAgent = $request->headers->get('User-Agent'); + $session->setUserAgent(\is_string($userAgent) ? $userAgent : ''); $session->setQueryString($request->getQueryString() ?: ''); $session->setLoanTerm($request->query->get('y') ?: ''); - $session->setRepApr($request->query->has('r') ? hexdec($request->query->get('r')) / 100 : ''); + $session->setRepApr($request->query->has('r') ? (string) (\hexdec($request->query->get('r')) / 100) : ''); foreach (self::UTM_CODES as $code) { $method = 'set'.Inflector::classify($code); @@ -190,7 +176,7 @@ private function generateSessionAndLifetime(Request $request): void $lifetime->addSession($session); - $this->em->flush(); + $this->entityManager->flush($session); $this->sessionStore->setSession($session); } diff --git a/src/Manager/DeviceFingerprintManager.php b/src/Manager/DeviceFingerprintManager.php index 1860e20..0f3af49 100644 --- a/src/Manager/DeviceFingerprintManager.php +++ b/src/Manager/DeviceFingerprintManager.php @@ -20,12 +20,12 @@ class DeviceFingerprintManager public function generateHashes(Device $device): Device { - $data = json_decode($device->getFingerprint(), true); + $data = \json_decode($device->getFingerprint(), true); - if (!empty($data) && JSON_ERROR_NONE === json_last_error()) { + if (\is_array($data) && \JSON_ERROR_NONE === \json_last_error()) { foreach (self::HASHES as $field) { - $method = 'set'.ucfirst($field); - $value = array_key_exists($field, $data) ? md5(serialize($data[$field])) : null; + $method = 'set'.\ucfirst($field); + $value = \array_key_exists($field, $data) ? \md5(\serialize($data[$field])) : null; $device->$method($value); } } diff --git a/src/Resources/config/controllers.yml b/src/Resources/config/controllers.yml new file mode 100644 index 0000000..cc289c0 --- /dev/null +++ b/src/Resources/config/controllers.yml @@ -0,0 +1,9 @@ +services: + _defaults: + public: true # Controllers need to be public + autowire: true + tags: + - 'controller.service_arguments' + + Alpha\VisitorTrackingBundle\Controller\: + resource: '../../Controller/*' \ No newline at end of file diff --git a/src/Resources/config/services.yml b/src/Resources/config/services.yml index bab332f..7759779 100644 --- a/src/Resources/config/services.yml +++ b/src/Resources/config/services.yml @@ -1,4 +1,5 @@ -parameters: +import: + - 'controllers.yml' services: alpha.visitor_tracking_subscriber: diff --git a/tests/Manager/DeviceFingerprintManagerTest.php b/tests/Manager/DeviceFingerprintManagerTest.php index 1411c4b..36aaf34 100644 --- a/tests/Manager/DeviceFingerprintManagerTest.php +++ b/tests/Manager/DeviceFingerprintManagerTest.php @@ -1,11 +1,14 @@ generateHashes($device); - $this->assertSame(md5(serialize('valid')), $device->getCanvas()); - $this->assertSame(md5(serialize([3, 4])), $device->getScreen()); + $this->assertSame(\md5(\serialize('valid')), $device->getCanvas()); + $this->assertSame(\md5(\serialize([3, 4])), $device->getScreen()); $this->assertNull($device->getFonts()); $this->assertNull($device->getNavigator()); $this->assertNull($device->getPlugins());