From 04b398ca7222adb8a3da08d1f6372319be6209a3 Mon Sep 17 00:00:00 2001 From: Daniel Bosen Date: Tue, 25 Oct 2016 13:28:46 +0200 Subject: [PATCH] Feature/2820072 phpcs eslint (#11) * automated fixes * fix phpcs Drupal warnings * fix drupal best practice warnings * fix eslint * fix schema * depend on breakpoint_js_settings --- .github/PULL_REQUEST_TEMPLATE.md | 10 ++ .travis.yml | 124 ++++++++++++++++++++++++ README.md | 2 +- ad_integration.info.yml | 1 + config/schema/ad_integration.schema.yml | 2 +- js/adHelper.js | 12 +-- js/adIntegration.js | 6 +- scripts/travis/test-source-code.sh | 23 +++++ src/AdIntegration.php | 4 - src/AdIntegrationInterface.php | 2 +- src/AdIntegrationLookup.php | 34 +++++-- src/AdIntegrationLookupInterface.php | 6 +- src/Form/SettingsForm.php | 48 ++++----- src/Plugin/Block/AdvertisingSlot.php | 4 +- tests/src/Kernel/AdIntegrationTest.php | 9 +- 15 files changed, 228 insertions(+), 59 deletions(-) create mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 .travis.yml create mode 100755 scripts/travis/test-source-code.sh diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000..f2be175 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,10 @@ +Make sure these boxes are checked before submitting your pull request - thank you! + +- [ ] All coding styles are fulfilled. ([How to check for cs issues?](https://github.com/BurdaMagazinOrg/thunder-dev-tools/blob/master/README.md#code-style-guidelines)) +- [ ] All tests are running locally. ([How to run the test?](https://github.com/BurdaMagazinOrg/thunder-distribution/blob/8.x-1.x/docs/development.md#how-to-run-the-tests)) +- [ ] Necessary update hooks are provided. +- [ ] User roles have correct access for new introduced permission. +- [ ] Every thunder module has a README.md in its root. Follow [this guidelines](https://www.drupal.org/node/2181737), but we don't need every topic. +- [ ] Code is covered with well-balanced amount of inline comments. + +If you are really awesome, then your feature is covered by additional tests. Well done! diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..65936f6 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,124 @@ +# @file +# .travis.yml - Drupal for Travis CI Integration +# +# Template provided by https://github.com/LionsAd/drupal_ti. + +language: php + +sudo: false + +php: + - 7 + - 5.6 + +addons: + apt: + packages: + - nodejs +branches: + only: + - /^8\.([0-9]+|x)\-[0-9]+\.([0-9]+|x)$/ + +env: + global: + # add composer's global bin directory to the path + # see: https://github.com/drush-ops/drush#install---composer + - PATH="$PATH:$HOME/.composer/vendor/bin" + + # Configuration variables. + - DRUPAL_TI_MODULE_NAME="ad_integration" + - DRUPAL_TI_SIMPLETEST_GROUP=$DRUPAL_TI_MODULE_NAME + + # Define runners and environment vars to include before and after the + # main runners / environment vars. + #- DRUPAL_TI_SCRIPT_DIR_BEFORE="./drupal_ti/before" + #- DRUPAL_TI_SCRIPT_DIR_AFTER="./drupal_ti/after" + + # The environment to use, supported are: drupal-7, drupal-8 + - DRUPAL_TI_ENVIRONMENT="drupal-8" + + # The installation profile to use: + #- DRUPAL_TI_INSTALL_PROFILE="testing" + + # Drupal specific variables. + - DRUPAL_TI_DB="drupal_travis_db" + - DRUPAL_TI_DB_URL="mysql://root:@127.0.0.1/drupal_travis_db" + # Note: Do not add a trailing slash here. + - DRUPAL_TI_WEBSERVER_URL="http://127.0.0.1" + - DRUPAL_TI_WEBSERVER_PORT="8080" + + # Simpletest specific commandline arguments, the DRUPAL_TI_SIMPLETEST_GROUP is appended at the end. + - DRUPAL_TI_SIMPLETEST_ARGS="--verbose --color --concurrency 4 --url $DRUPAL_TI_WEBSERVER_URL:$DRUPAL_TI_WEBSERVER_PORT" + + # === Behat specific variables. + # This is relative to $TRAVIS_BUILD_DIR + - DRUPAL_TI_BEHAT_DIR="./tests/behat" + # These arguments are passed to the bin/behat command. + - DRUPAL_TI_BEHAT_ARGS="" + # Specify the filename of the behat.yml with the $DRUPAL_TI_DRUPAL_DIR variables. + - DRUPAL_TI_BEHAT_YML="behat.yml.dist" + # This is used to setup Xvfb. + - DRUPAL_TI_BEHAT_SCREENSIZE_COLOR="1280x1024x16" + # The version of selenium that should be used. + - DRUPAL_TI_BEHAT_SELENIUM_VERSION="2.48.2" + # Set DRUPAL_TI_BEHAT_DRIVER to "selenium" to use "firefox" or "chrome" here. + - DRUPAL_TI_BEHAT_DRIVER="phantomjs" + - DRUPAL_TI_BEHAT_BROWSER="firefox" + + # PHPUnit specific commandline arguments. + - DRUPAL_TI_PHPUNIT_ARGS="--verbose --debug" + # Specifying the phpunit-core src/ directory is useful when e.g. a vendor/ + # directory is present in the module directory, which phpunit would then + # try to find tests in. This option is relative to $TRAVIS_BUILD_DIR. + #- DRUPAL_TI_PHPUNIT_CORE_SRC_DIRECTORY="./tests/src" + + # Code coverage via coveralls.io + - DRUPAL_TI_COVERAGE="satooshi/php-coveralls:0.6.*" + # This needs to match your .coveralls.yml file. + - DRUPAL_TI_COVERAGE_FILE="build/logs/clover.xml" + + # Debug options + #- DRUPAL_TI_DEBUG="-x -v" + # Set to "all" to output all files, set to e.g. "xvfb selenium" or "selenium", + # etc. to only output those channels. + #- DRUPAL_TI_DEBUG_FILE_OUTPUT="selenium xvfb webserver" + + matrix: + # [[[ SELECT ANY OR MORE OPTIONS ]]] + #- DRUPAL_TI_RUNNERS="phpunit" + #- DRUPAL_TI_RUNNERS="simpletest" + #- DRUPAL_TI_RUNNERS="behat" + #- DRUPAL_TI_RUNNERS="phpunit simpletest behat" + # Use phpunit-core to test modules with phpunit with Drupal 8 core. + - DRUPAL_TI_RUNNERS="phpunit-core" + +mysql: + database: drupal_travis_db + username: root + encoding: utf8 + +before_install: + - composer self-update + + # Code Checks + - bash -x -e ./scripts/travis/test-source-code.sh + + # Continue with Drupal Tests + - cd ./tests + - composer global require "lionsad/drupal_ti:1.*" + - drupal-ti before_install + +install: + - drupal-ti install + +before_script: + - drupal-ti before_script + +script: + - drupal-ti script + +after_script: + - drupal-ti after_script + +notifications: + email: false \ No newline at end of file diff --git a/README.md b/README.md index d03b428..5813e92 100644 --- a/README.md +++ b/README.md @@ -4,5 +4,5 @@ This modules aims to be a base module for the integration of different ad provid ## Usage After installing the module visit /admin/config/system/ads to configure it. -First select the ad provider, currently ForwardAd Group (fag) and orbyd are possible selections. When selection "fag" you have to provide a container tag url and the used fag ad engine you want to use. +First select the ad provider, currently ForwardAd Group (fag) and orbyd are possible selections. When selection "fag" you have to provide a container tag url and the fag ad engine you want to use. diff --git a/ad_integration.info.yml b/ad_integration.info.yml index 9bb21e4..48fcd81 100644 --- a/ad_integration.info.yml +++ b/ad_integration.info.yml @@ -6,3 +6,4 @@ core: 8.x dependencies: - token + - breakpoint_js_settings diff --git a/config/schema/ad_integration.schema.yml b/config/schema/ad_integration.schema.yml index 9d37821..eee551e 100644 --- a/config/schema/ad_integration.schema.yml +++ b/config/schema/ad_integration.schema.yml @@ -1,5 +1,5 @@ ad_integration.settings: - type: mapping + type: config_object label: 'Ad integration settings' mapping: ad_provider: diff --git a/js/adHelper.js b/js/adHelper.js index 0edd34c..9d14166 100644 --- a/js/adHelper.js +++ b/js/adHelper.js @@ -19,20 +19,20 @@ function getDeviceType() { return 'desktop'; } -function deviceIsMobile() { +window.deviceIsMobile = function () { 'use strict'; return (getDeviceType() === 'smartphone'); -} +}; -function deviceIsTablet() { +window.deviceIsTablet = function () { 'use strict'; return (getDeviceType() === 'tablet'); -} +}; -function deviceIsDesktop() { +window.deviceIsDesktop = function () { 'use strict'; return (getDeviceType() === 'desktop'); -} +}; diff --git a/js/adIntegration.js b/js/adIntegration.js index 886c52d..cbac8c2 100644 --- a/js/adIntegration.js +++ b/js/adIntegration.js @@ -17,8 +17,10 @@ } for (var slot_html_id in settings.AdvertisingSlots) { - var tag = settings.AdvertisingSlots[slot_html_id][adsc_device]; - $('#' + slot_html_id).html(''); + if (settings.AdvertisingSlots.hasOwnProperty(slot_html_id)) { + var tag = settings.AdvertisingSlots[slot_html_id][window.adsc_device]; + $('#' + slot_html_id).html(''); + } } } diff --git a/scripts/travis/test-source-code.sh b/scripts/travis/test-source-code.sh new file mode 100755 index 0000000..3354d60 --- /dev/null +++ b/scripts/travis/test-source-code.sh @@ -0,0 +1,23 @@ +# remove xdebug to make php execute faster +phpenv config-rm xdebug.ini + +# globally require drupal coder for code tests +composer global require drupal/coder + +# run phpcs +phpcs --config-set installed_paths ~/.composer/vendor/drupal/coder/coder_sniffer +phpcs --standard=Drupal --report=summary -p . +phpcs --standard=DrupalPractice --report=summary -p . + +# JS ESLint checking +mv ~/.nvm ~/.nvm-backup +git clone https://github.com/creationix/nvm.git ~/.nvm +(cd ~/.nvm && git checkout `git describe --abbrev=0 --tags`) +set -x +source ~/.nvm/nvm.sh +set +x +nvm install 4 +npm install -g eslint +eslint . +rm -rf ~/.nvm +mv ~/.nvm-backup ~/.nvm \ No newline at end of file diff --git a/src/AdIntegration.php b/src/AdIntegration.php index 7ba1aba..c311ebc 100644 --- a/src/AdIntegration.php +++ b/src/AdIntegration.php @@ -3,10 +3,6 @@ namespace Drupal\ad_integration; use Drupal\Core\Config\ConfigFactoryInterface; -use Drupal\Core\Entity\EntityTypeManagerInterface; -use Drupal\Core\Entity\Query\QueryFactory; -use Drupal\Core\Path\PathMatcher; -use Drupal\Core\Routing\CurrentRouteMatch; use Drupal\Core\Utility\Token; /** diff --git a/src/AdIntegrationInterface.php b/src/AdIntegrationInterface.php index d36c83f..11c0beb 100644 --- a/src/AdIntegrationInterface.php +++ b/src/AdIntegrationInterface.php @@ -44,7 +44,7 @@ public function getAdEngine(); * ['entity'] ContentEntityInterface - instance of entity. * ['term'] TermInterface - instance of term. * - * @return string The ad unit 1. + * @return string * The ad unit 1. */ public function getAdUnit1($data = array()); diff --git a/src/AdIntegrationLookup.php b/src/AdIntegrationLookup.php index 58d7484..6674a7f 100644 --- a/src/AdIntegrationLookup.php +++ b/src/AdIntegrationLookup.php @@ -18,7 +18,7 @@ */ class AdIntegrationLookup implements AdIntegrationLookupInterface { - const supportedEntityParameters = ['node', 'taxonomy_term']; + const SUPPORTED_ENTITY_PARAMETERS = ['node', 'taxonomy_term']; protected $currentRouteMatch; protected $config; @@ -28,8 +28,11 @@ class AdIntegrationLookup implements AdIntegrationLookupInterface { * AdIntegrationLookup constructor. * * @param \Drupal\Core\Routing\RouteMatchInterface $currentRouteMatch + * The route matcher. * @param \Drupal\Core\Config\ConfigFactoryInterface $configFactory + * The configuration. * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager + * The entity type manager. */ public function __construct(RouteMatchInterface $currentRouteMatch, ConfigFactoryInterface $configFactory, EntityTypeManagerInterface $entityTypeManager) { $this->currentRouteMatch = $currentRouteMatch; @@ -59,7 +62,8 @@ public function byCurrentRoute($name, $termsOnly = FALSE) { * @param string $name * The name of the Ad property to look up. * @param \Drupal\Core\Routing\RouteMatchInterface $routeMatch - * The route matching the entity (node, term) on which to look up properties. + * The route matching the entity (node, term) on which to look up + * properties. * @param bool $termsOnly * If set to TRUE, skips lookup on node settings. * @@ -69,7 +73,7 @@ public function byCurrentRoute($name, $termsOnly = FALSE) { public function byRoute($name, RouteMatchInterface $routeMatch, $termsOnly = FALSE) { $entity = NULL; - foreach (static::supportedEntityParameters as $parameter) { + foreach (static::SUPPORTED_ENTITY_PARAMETERS as $parameter) { if ($entity = $routeMatch->getParameter($parameter)) { if (is_numeric($entity)) { $entity = Node::load($entity); @@ -145,9 +149,8 @@ protected function searchEntity($name, ContentEntityInterface $entity, $termsOnl } } - // Check for fallback categories if no ad_integration_setting is found. - if (!isset($termOverride) && $fieldType === 'entity_reference' && $fieldDefinition->getSetting('target_type') === 'taxonomy_term') { + if ($fieldType === 'entity_reference' && $fieldDefinition->getSetting('target_type') === 'taxonomy_term') { $fieldName = $fieldDefinition->getName(); if ($tid = $entity->$fieldName->target_id) { if ($term = Term::load($tid)) { @@ -167,9 +170,15 @@ protected function searchEntity($name, ContentEntityInterface $entity, $termsOnl } /** + * Search for a property value in a term. + * * @param string $name + * The property name. * @param \Drupal\taxonomy\Entity\TermInterface $term + * The term in which to search for the value. + * * @return string|null + * The found value. */ protected function searchTerm($name, TermInterface $term) { foreach ($term->getFieldDefinitions() as $fieldDefinition) { @@ -192,11 +201,17 @@ protected function searchTerm($name, TermInterface $term) { } /** - * @param $name - * @param $fieldDefinition - * @param $entity + * Retrieve overriden setting. + * + * @param string $name + * The name of the setting. + * @param FieldDefinitionInterface $fieldDefinition + * The Ad Settings field. + * @param ContentEntityInterface $entity + * The entity to search for overrides in. * * @return string|null + * The overridden value of null if none is found. */ protected function getOverriddenAdSetting($name, FieldDefinitionInterface $fieldDefinition, ContentEntityInterface $entity) { if ($fieldDefinition->getType() === 'ad_integration_settings') { @@ -220,4 +235,5 @@ protected function getOverriddenAdSetting($name, FieldDefinitionInterface $field private function defaults($name) { return $this->config->get($name . '_default'); } -} \ No newline at end of file + +} diff --git a/src/AdIntegrationLookupInterface.php b/src/AdIntegrationLookupInterface.php index 20d6761..4505a72 100644 --- a/src/AdIntegrationLookupInterface.php +++ b/src/AdIntegrationLookupInterface.php @@ -32,7 +32,8 @@ public function byCurrentRoute($name, $termsOnly = FALSE); * @param string $name * The name of the Ad property to look up. * @param \Drupal\Core\Routing\RouteMatchInterface $routeMatch - * The route matching the entity (node, term) on which to look up properties. + * The route matching the entity (node, term) on which to look up + * properties. * @param bool $termsOnly * If set to TRUE, skips lookup on node settings. * @@ -68,4 +69,5 @@ public function byEntity($name, ContentEntityInterface $entity, $termsOnly = FAL * The property value. */ public function byTerm($name, TermInterface $term); -} \ No newline at end of file + +} diff --git a/src/Form/SettingsForm.php b/src/Form/SettingsForm.php index 21a2b9f..2cc339a 100644 --- a/src/Form/SettingsForm.php +++ b/src/Form/SettingsForm.php @@ -10,7 +10,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface; /** - * Defines a form that configures ivw settings. + * Defines a form that configures ad settings. */ class SettingsForm extends ConfigFormBase { /** @@ -63,14 +63,14 @@ public function buildForm(array $form, FormStateInterface $form_state) { $form['site_settings'] = array( '#type' => 'details', - '#title' => t('Site settings'), + '#title' => $this->t('Site settings'), '#open' => TRUE, '#group' => 'ad_settings', ); $form['default_values'] = [ '#type' => 'details', - '#title' => t('Default values'), + '#title' => $this->t('Default values'), '#open' => FALSE, '#group' => 'ad_settings', ]; @@ -79,13 +79,13 @@ public function buildForm(array $form, FormStateInterface $form_state) { $form['site_settings']['ad_provider'] = array( '#type' => 'select', '#options' => $provider_options, - '#title' => t('Ad provider'), + '#title' => $this->t('Ad provider'), '#default_value' => $settings->get('ad_provider'), ); $form['site_settings']['adsc_container_tag'] = array( '#type' => 'textfield', - '#title' => t('Container tag url'), + '#title' => $this->t('Container tag url'), '#default_value' => $settings->get('adsc_container_tag'), '#states' => array( 'visible' => array( @@ -96,9 +96,9 @@ public function buildForm(array $form, FormStateInterface $form_state) { $form['site_settings']['adsc_fia_tag'] = array( '#type' => 'textfield', - '#title' => t('FB Instant Articles: Ad-Tag'), + '#title' => $this->t('FB Instant Articles: Ad-Tag'), '#default_value' => $settings->get('adsc_fia_tag'), - '#description' => t('The Ad-Tag for Facebook Instant Articles provided by Orbyd'), + '#description' => $this->t('The Ad-Tag for Facebook Instant Articles provided by Orbyd'), '#states' => array( 'visible' => array( ':input[name=ad_provider]' => array('value' => 'orbyd'), @@ -108,43 +108,43 @@ public function buildForm(array $form, FormStateInterface $form_state) { $form['site_settings']['adsc_ad_engine'] = array( '#type' => 'textfield', - '#title' => t('Ad engine'), + '#title' => $this->t('Ad engine'), '#default_value' => $settings->get('adsc_ad_engine'), ); $adsc_unit2_values = $settings->get('adsc_unit2_values'); $form['site_settings']['adsc_unit2_values'] = array( '#type' => 'textarea', - '#title' => t('First hierarchy level values'), + '#title' => $this->t('First hierarchy level values'), '#default_value' => !empty($adsc_unit2_values) ? Tags::implode($adsc_unit2_values) : '', - '#description' => t('Comma separated list of possible values for first hierarchy level'), + '#description' => $this->t('Comma separated list of possible values for first hierarchy level'), ); $adsc_unit3_values = $settings->get('adsc_unit3_values'); $form['site_settings']['adsc_unit3_values'] = array( '#type' => 'textarea', - '#title' => t('Second hierarchy level values'), + '#title' => $this->t('Second hierarchy level values'), '#default_value' => !empty($adsc_unit3_values) ? Tags::implode($adsc_unit3_values) : '', - '#description' => t('Comma separated list of possible values for second hierarchy level'), + '#description' => $this->t('Comma separated list of possible values for second hierarchy level'), ); $form['default_values']['adsc_unit1_default'] = [ - '#title' => t('Ad level 1'), + '#title' => $this->t('Ad level 1'), '#type' => 'textfield', '#default_value' => $settings->get('adsc_unit1_default'), - '#description' => t('First hierarchical level. This is the name of the Website'), + '#description' => $this->t('First hierarchical level. This is the name of the Website'), ]; $form['default_values']['adsc_unit1_overridable'] = [ '#type' => 'checkbox', - '#title' => t('Adsc Unit 1 is overrideable'), + '#title' => $this->t('Adsc Unit 1 is overrideable'), '#default_value' => $settings->get('adsc_unit1_overridable'), ]; $form['default_values']['adsc_unit2_default'] = [ - '#title' => t('Ad level 2'), + '#title' => $this->t('Ad level 2'), '#default_value' => $settings->get('adsc_unit2_default'), - '#description' => t('Second hierarchical level'), + '#description' => $this->t('Second hierarchical level'), ]; if (empty($adsc_unit2_values)) { @@ -157,14 +157,14 @@ public function buildForm(array $form, FormStateInterface $form_state) { $form['default_values']['adsc_unit2_overridable'] = [ '#type' => 'checkbox', - '#title' => t('Ad level 2 is overrideable'), + '#title' => $this->t('Ad level 2 is overrideable'), '#default_value' => $settings->get('adsc_unit2_overridable'), ]; $form['default_values']['adsc_unit3_default'] = [ - '#title' => t('Ad level 3'), + '#title' => $this->t('Ad level 3'), '#default_value' => $settings->get('adsc_unit3_default'), - '#description' => t('Third hierarchical level'), + '#description' => $this->t('Third hierarchical level'), ]; if (empty($adsc_unit3_values)) { @@ -177,17 +177,17 @@ public function buildForm(array $form, FormStateInterface $form_state) { $form['default_values']['adsc_unit3_overridable'] = [ '#type' => 'checkbox', - '#title' => t('Ad level 3 is overrideable'), + '#title' => $this->t('Ad level 3 is overrideable'), '#default_value' => $settings->get('adsc_unit3_overridable'), ]; $modes = ['full' => 'full', 'infinite' => 'infinite']; $form['default_values']['adsc_mode_default'] = [ - '#title' => t('Adsc mode'), + '#title' => $this->t('Adsc mode'), '#type' => 'select', '#options' => $modes, '#default_value' => $settings->get('adsc_mode_default'), - '#description' => t('Adsc mode, fag provides a special mode for infinite scrolling.'), + '#description' => $this->t('Adsc mode, fag provides a special mode for infinite scrolling.'), '#states' => array( 'visible' => array( ':input[name=ad_provider]' => array('value' => 'fag'), @@ -197,7 +197,7 @@ public function buildForm(array $form, FormStateInterface $form_state) { $form['default_values']['adsc_mode_overridable'] = [ '#type' => 'checkbox', - '#title' => t('Adsc mode is overrideable'), + '#title' => $this->t('Adsc mode is overrideable'), '#default_value' => $settings->get('adsc_mode_overridable'), ]; diff --git a/src/Plugin/Block/AdvertisingSlot.php b/src/Plugin/Block/AdvertisingSlot.php index f433621..e1a9e46 100644 --- a/src/Plugin/Block/AdvertisingSlot.php +++ b/src/Plugin/Block/AdvertisingSlot.php @@ -97,7 +97,7 @@ public function blockForm($form, FormStateInterface $form_state) { $device = $mapping['device']; $form[$device] = [ '#type' => 'textfield', - '#title' => t('Adtag for :device', array(':device' => $device)), + '#title' => $this->t('Adtag for :device', array(':device' => $device)), '#default_value' => $config[$device] ? $config[$device] : NULL, ]; } @@ -105,7 +105,7 @@ public function blockForm($form, FormStateInterface $form_state) { else { $form['adtag'] = [ '#type' => 'textfield', - '#title' => t('Adtag'), + '#title' => $this->t('Adtag'), '#default_value' => $config['adtag'] ? $config['adtag'] : NULL, ]; } diff --git a/tests/src/Kernel/AdIntegrationTest.php b/tests/src/Kernel/AdIntegrationTest.php index 43e29f1..f52758e 100644 --- a/tests/src/Kernel/AdIntegrationTest.php +++ b/tests/src/Kernel/AdIntegrationTest.php @@ -1,23 +1,18 @@ save(); } -} \ No newline at end of file +}