Cookie Control is a JavaScript module that can help make a website compliant with EU cookie legislation; and specifically since version 8 with the General Data Protection Regulation's (GDPR) guidelines on the use of cookies.

Please Note:

You will need to obtain an API KEY from Civic UK in order to use the module.

Cookie Control is simply a mechanism to enable you to comply with UK and EU law on cookies. You need to determine which elements of your website are using cookies (this can be done via a Cookie Audit, and ensure they are connected to Cookie Control.

Installation

  1. Obtain an API Key from Civic UK for the site that you wish to deploy Cookie Control.
  2. Add the module in the corresponding Drupal folder.
  3. Enable the module.
  4. Run "drush updb" or update the database from update.php.
  5. Configure the module from the 'Configuration->Civic Cookie Control menu'.
  6. All done. Good job!

Similar Module

An outdated, unsupported, drupal 7 module using a really old version of the cookie control js script (which is not GDPR compliant) already exist in https://www.drupal.org/project/cookiecontrol. Taking under consideration that the new Drupal 8/9 which is GDPR compliant is officially supported/maintained by CIVIC - the cookie control widget creator - we have decided to create a new project to stand out from the old module.

Project Link

https://www.drupal.org/project/civicccookiecontrol

Git Information

git clone --branch 4.4.x-dev https://git.drupalcode.org/project/civicccookiecontrol.git

PAReview checklist

https://pareview.net/r/160

Comments

tper created an issue. See original summary.

swetha_bapanah’s picture

swetha_bapanah commented

Hi @tper,

Please fix the below code review comments.

FILE: ...-site1/modules/contrib/civicccookiecontrol/config/install/iab.settings.yml
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
24 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: ...ontrib/civicccookiecontrol/tests/src/Functional/CookieControl8TestCase.php
--------------------------------------------------------------------------------
FOUND 7 ERRORS AFFECTING 7 LINES
--------------------------------------------------------------------------------
28 | ERROR | Doc comment is empty
40 | ERROR | Doc comment is empty
43 | ERROR | Public method name "CookieControl8TestCase::testCCInit" is not
| | in lowerCamel format
131 | ERROR | Doc comment is empty
134 | ERROR | Public method name "CookieControl8TestCase::testCCV8" is not in
| | lowerCamel format
176 | ERROR | Doc comment is empty
179 | ERROR | Public method name "CookieControl8TestCase::testCCV9" is not in
| | lowerCamel format
--------------------------------------------------------------------------------

FILE: ...upal-site1/modules/contrib/civicccookiecontrol/civiccookiecontrol.info.yml
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 1 LINE
--------------------------------------------------------------------------------
1 | WARNING | Remove "project" from the info file, it will be added by
| | drupal.org packaging automatically
1 | WARNING | Remove "datestamp" from the info file, it will be added by
| | drupal.org packaging automatically
1 | WARNING | Remove "version" from the info file, it will be added by
| | drupal.org packaging automatically
--------------------------------------------------------------------------------

FILE: ...rupal-site1/modules/contrib/civicccookiecontrol/civiccookiecontrol.install
--------------------------------------------------------------------------------
FOUND 16 ERRORS AFFECTING 13 LINES
--------------------------------------------------------------------------------
147 | ERROR | [x] You must use "/**" style comments for a function comment
152 | ERROR | [x] Doc comment short description must end with a full stop
157 | ERROR | [x] Functions must not contain multiple empty lines in a row;
| | found 2 empty lines
185 | ERROR | [x] Doc comment short description must end with a full stop
219 | ERROR | [x] Doc comment short description must end with a full stop
231 | ERROR | [x] Doc comment short description must end with a full stop
266 | ERROR | [x] Doc comment short description must end with a full stop
310 | ERROR | [x] Doc comment short description must end with a full stop
337 | ERROR | [x] Expected 1 blank line after function; 2 found
343 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found 1
343 | ERROR | [ ] Invalid function name, expected clear_tempstore but found
| | clearTempstore
343 | ERROR | [x] Expected 1 space before opening brace; found 0
344 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 3
345 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 3
348 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found 1
348 | ERROR | [x] Expected 1 newline at end of file; 2 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 15 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: ...tes/devdesktop/drupal-site1/modules/contrib/civicccookiecontrol/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 26 WARNINGS AFFECTING 26 LINES
--------------------------------------------------------------------------------
19 | WARNING | Line exceeds 80 characters; contains 181 characters
23 | WARNING | Line exceeds 80 characters; contains 202 characters
25 | WARNING | Line exceeds 80 characters; contains 134 characters
27 | WARNING | Line exceeds 80 characters; contains 144 characters
56 | WARNING | Line exceeds 80 characters; contains 81 characters
60 | WARNING | Line exceeds 80 characters; contains 81 characters
62 | WARNING | Line exceeds 80 characters; contains 138 characters
69 | WARNING | Line exceeds 80 characters; contains 95 characters
71 | WARNING | Line exceeds 80 characters; contains 181 characters
79 | WARNING | Line exceeds 80 characters; contains 110 characters
80 | WARNING | Line exceeds 80 characters; contains 133 characters
81 | WARNING | Line exceeds 80 characters; contains 263 characters
87 | WARNING | Line exceeds 80 characters; contains 101 characters
88 | WARNING | Line exceeds 80 characters; contains 86 characters
96 | WARNING | Line exceeds 80 characters; contains 92 characters
97 | WARNING | Line exceeds 80 characters; contains 91 characters
99 | WARNING | Line exceeds 80 characters; contains 389 characters
100 | WARNING | Line exceeds 80 characters; contains 186 characters
102 | WARNING | Line exceeds 80 characters; contains 177 characters
103 | WARNING | Line exceeds 80 characters; contains 227 characters
104 | WARNING | Line exceeds 80 characters; contains 174 characters
105 | WARNING | Line exceeds 80 characters; contains 156 characters
106 | WARNING | Line exceeds 80 characters; contains 107 characters
110 | WARNING | Line exceeds 80 characters; contains 124 characters
114 | WARNING | Line exceeds 80 characters; contains 102 characters
123 | WARNING | Line exceeds 80 characters; contains 136 characters
--------------------------------------------------------------------------------

FILE: ...drupal-site1/modules/contrib/civicccookiecontrol/civiccookiecontrol.module
--------------------------------------------------------------------------------
FOUND 91 ERRORS AND 2 WARNINGS AFFECTING 79 LINES
--------------------------------------------------------------------------------
14 | WARNING | [x] Unused use statement
20 | ERROR | [x] Opening brace should be on the same line as the
| | declaration
21 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
22 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
23 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 12
24 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 12
25 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 12
28 | ERROR | [x] Case breaking statement indented incorrectly; expected 10
| | spaces, found 12
29 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
36 | ERROR | [x] Opening brace should be on the same line as the
| | declaration
37 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
38 | ERROR | [x] Array indentation error, expected 6 spaces but found 8
39 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
40 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
42 | ERROR | [x] Array indentation error, expected 6 spaces but found 8
43 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
44 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
46 | ERROR | [x] Array indentation error, expected 6 spaces but found 8
47 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
48 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
50 | ERROR | [x] Array indentation error, expected 6 spaces but found 8
51 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
52 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
54 | ERROR | [x] Array indentation error, expected 6 spaces but found 8
55 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
56 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
65 | ERROR | [x] Opening brace should be on the same line as the
| | declaration
67 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
67 | ERROR | [x] There should be no white space after an opening "("
73 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
74 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
75 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
76 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
77 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
84 | ERROR | [x] Opening brace should be on the same line as the
| | declaration
85 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
87 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
87 | ERROR | [x] Expected 1 space before "=="; 0 found
87 | ERROR | [x] Expected 1 space after "=="; 0 found
87 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "TRUE"
| | but found "true"
87 | ERROR | [x] Expected 1 space before "=="; 0 found
87 | ERROR | [x] Expected 1 space after "=="; 0 found
87 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "TRUE"
| | but found "true"
88 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
88 | ERROR | [x] Concat operator must be surrounded by a single space
89 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
89 | ERROR | [x] Expected newline after closing brace
89 | ERROR | [x] Expected 1 space after ELSE keyword; 0 found
90 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
91 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
92 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
93 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
94 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
95 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
96 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
98 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
99 | ERROR | [x] Array indentation error, expected 6 spaces but found 8
101 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
102 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
103 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
110 | ERROR | [x] Opening brace should be on the same line as the
| | declaration
111 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
112 | ERROR | [x] Object operator not indented correctly; expected 6 spaces
| | but found 4
115 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
115 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced
| | with use statements
116 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
117 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
118 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
119 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
127 | ERROR | [x] Separate the @param and @return sections by a blank line.
128 | ERROR | [x] Return comment indentation must be 3 spaces, found 4
| | spaces
130 | ERROR | [ ] Type hint "array" missing for $configElement
131 | ERROR | [x] Opening brace should be on the same line as the
| | declaration
132 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
133 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
134 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
136 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
137 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
138 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
145 | ERROR | [x] Opening brace should be on the same line as the
| | declaration
146 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
146 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced
| | with use statements
147 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
148 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
148 | ERROR | [x] Expected newline after closing brace
148 | ERROR | [x] Expected 1 space after ELSE keyword; 0 found
149 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
150 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
154 | WARNING | [ ] Format should be "* Implements hook_foo().", "* Implements
| | hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements
| | hook_foo_BAR_ID_bar() for xyz-bar.html.twig.", "*
| | Implements hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.", or
| | "* Implements hook_foo_BAR_ID_bar() for block templates."
157 | ERROR | [x] Parameter comment indentation must be 3 spaces, found 2
| | spaces
159 | ERROR | [ ] Type hint "array" missing for $js
162 | ERROR | [x] Expected 1 space after closing parenthesis; found 5
168 | ERROR | [x] Expected 1 newline at end of file; 2 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 90 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: ...l-site1/modules/contrib/civicccookiecontrol/src/Form/Steps/CCCSettings.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AND 4 WARNINGS AFFECTING 5 LINES
--------------------------------------------------------------------------------
109 | ERROR | [x] Expected 1 space after "=="; 0 found
109 | ERROR | [x] There should be no white space before a closing ")"
256 | WARNING | [ ] Only string literals should be passed to t() where
| | possible
259 | WARNING | [ ] Only string literals should be passed to t() where
| | possible
263 | WARNING | [ ] Only string literals should be passed to t() where
| | possible
271 | WARNING | [ ] Only string literals should be passed to t() where
| | possible
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: ...l-site1/modules/contrib/civicccookiecontrol/src/Form/Steps/CCCBaseStep.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
149 | ERROR | [x] Expected newline after closing brace
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Time: 901ms; Memory: 14MB

FILE: ../Sites/devdesktop/drupal-site1/modules/contrib/civicccookiecontrol/civiccookiecontrol.info.yml
--------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------
1 | WARNING | "core_version_requirement" property is missing in the info.yml file
--------------------------------------------------------------------------------------------------------------------------

FILE: ../Sites/devdesktop/drupal-site1/modules/contrib/civicccookiecontrol/civiccookiecontrol.module
------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------
160 | WARNING | Unused variable $path.
------------------------------------------------------------------------------------------------------------------------

FILE: ../Sites/devdesktop/drupal-site1/modules/contrib/civicccookiecontrol/src/Form/CivicCookieControlSettings.php
--------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------------------
376 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------------------------------------------------------------

FILE: ../Sites/devdesktop/drupal-site1/modules/contrib/civicccookiecontrol/src/Form/Steps/CCCStepsManager.php
---------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------
48 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
---------------------------------------------------------------------------------------------------------------------------------

Time: 470ms; Memory: 12MB

Thanks,
Swetha

swetha_bapanah’s picture

swetha_bapanah commented
Status: Needs review » Needs work

Please fix the above mentioned review comments.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: -PAreview: security
rohitrajputsahab’s picture

rohitrajputsahab commented

I have checked your module code. Most of the ".PHP" file you have used "Drupal::config". See the example files below. Please avoided this and use dependency injection instead .

Please check your all files.

Line src/Access/CookieControlAccess.php
------ ------------------------------------------------------------------------------
37 \Drupal calls should be avoided in classes, use dependency injection instead
50 \Drupal calls should be avoided in classes, use dependency injection instead
63 \Drupal calls should be avoided in classes, use dependency injection instead
67 \Drupal calls should be avoided in classes, use dependency injection instead
72 \Drupal calls should be avoided in classes, use dependency injection instead
------ ------------------------------------------------------------------------------

------ ------------------------------------------------------------------------------
Line src/CCCConfig/AbstractCCCConfig.php
------ ------------------------------------------------------------------------------
40 \Drupal calls should be avoided in classes, use dependency injection instead
102 \Drupal calls should be avoided in classes, use dependency injection instead
225 \Drupal calls should be avoided in classes, use dependency injection instead
241 \Drupal calls should be avoided in classes, use dependency injection instead
269 \Drupal calls should be avoided in classes, use dependency injection instead
288 \Drupal calls should be avoided in classes, use dependency injection instead
293 \Drupal calls should be avoided in classes, use dependency injection instead
------ ------------------------------------------------------------------------------

rajandro’s picture

rajandro commented
Issue summary: View changes

Update the Issue summary a bit by changing the Git Information.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Priority: Normal » Minor
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Closed (won't fix)

I am closing this application due to lack of replies.

tper’s picture

tper commented
Assigned: Unassigned » tper
Status: Closed (won't fix) » Needs work
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: tper » Unassigned
tper’s picture

tper commented

A new branch is now added in the module

git clone --branch '4.4.x-dev' git@git.drupal.org:project/civicccookiecontrol.git

and we intent to make a stable/secure release.

Here https://pareview.net/r/160/revisions/338/view are the results from pareview.

There are a some issues reported but these are intentionally not fixed. For example the ".. Only string literals should be passed to t() .." warnings are reported for variables that are getting their values from static yml config files.

Could you please advise how we should proceed with this one and what actions we need to take in order to get the security clearance?

tper’s picture

tper commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
Status: Needs review » Needs work
  • What follows is a quick review of the project; it doesn't mean to be complete
  • For every point, I didn't make a complete list of where the code should be fixed, but an example of what is wrong in the code
  • Not all the points are application stoppers; some of them describe changes that would be preferable to make
  /**
   * CCCSettings constructor.
   *
   * @param \Drupal\Core\Locale\CountryManager $countryManager
   *   Injected country manager service.
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config
   *   Injected config factory service.
   * @param \Drupal\Core\File\FileSystemInterface $fileSystem
   *   Injected files system service.
   * @param \Drupal\Core\TempStore\PrivateTempStoreFactory $tempStoreFactory
   *   Injected tempstore private service.
   */
  public function __construct(
        CountryManager $countryManager,
        ConfigFactoryInterface $config,
        FileSystemInterface $fileSystem,
        PrivateTempStoreFactory $tempStoreFactory
    ) {
    $this->countryManager = $countryManager;
    $this->fileSystem = $fileSystem;
    $this->tempStore = $tempStoreFactory->get('civiccookiecontrol');
    $this->config = $config->getEditable(CCCConfigNames::COOKIECONTROL);
    $this->loadFormElements();
  }

  /**
   * Set current step.
   *
   * @return int
   *   Return CCCStepsEnum value.
   */
  protected function setStep() {
    return CCCStepsEnum::CCC_SETTINGS;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    // Instantiates this form class.
    return new static(
    // Load the service required to construct this class.
          $container->get('country_manager'),
          $container->get('config.factory'),
          $container->get('file_system'),
          $container->get('tempstore.private')
      );
  }

A service defined in civiccookiecontrol.services.yml doesn't use create().
Contrary to what indicated from the comment, that isn't a form class, since forms aren't implemented as service, and they implement FormInterface.

  /**
   * Fetches step from steps property, If it doesn't exist, create step object.
   *
   * @param int $step_id
   *   Step ID.
   *
   * @return \Drupal\civiccookiecontrol\Form\Steps\CCCStepInterface
   *   Return step object.
   */
  public function getStep($step_id) {
    if (isset($this->steps[$step_id])) {
      $step = $this->steps[$step_id];
    }
    else {
      $class = CCCStepsEnum::map($step_id);
      $step = \Drupal::service($class);
      $step->setStepManager($this);
    }

A service doesn't use the Drupal class, but dependency injection.

      if (!empty($element['#title'])) {
        $element['#title'] = $this->t($element['#title']);
      }

The first argument of $this->t() needs to be a literal string. The code searching for the strings to translate handles only literal strings, not any other dynamic value, including the concatenation of strings.

class CookieCategoryForm extends EntityForm {

  /**
   * Cache configuration object from dependency injection.
   *
   * @var \Drupal\Core\Cache\CacheBackendInterface
   */
  protected $cache;

  /**
   * Router builder for dependency injection.
   *
   * @var \Drupal\Core\Routing\RouteBuilderInterface
   */
  protected $routerBuilder;

  /**
   * CookieCategoryForm constructor.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   *   Dependency injected entityTypeManager.
   * @param \Drupal\Core\Cache\CacheBackendInterface $cache
   *   Dependency injected cache object.
   * @param \Drupal\Core\Routing\RouteBuilderInterface $routeBuilder
   *   Dependency injected router builder object.
   */
  public function __construct(EntityTypeManagerInterface $entityTypeManager, CacheBackendInterface $cache, RouteBuilderInterface $routeBuilder) {
    $this->entityTypeManager = $entityTypeManager;
    $this->cache = $cache;
    $this->routerBuilder = $routeBuilder;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
          $container->get('entity_type.manager'),
          $container->get('cache.data'),
          $container->get('router.builder')
      );
  }

}

Classes that extends EntityForm aren't used for services. If they are services, they don't implement create().

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Tag names like 4.4.x-dev are wrong. They need to be deleted, after creating a correct tag and making it the default one. A correct tag name doesn't end in -dev.

tper’s picture

tper commented

Hi @kiamlaluno,

I just made another commit trying to resolve the dependency injection/services issues you reported.

The translation function ($this->t()) is used with variables only in places where we load the corresponding strings from yml configuration files. For example:

    $iabYamlPath = drupal_get_path('module', 'civiccookiecontrol') . "/src/Form/IAB2FormElements/iab.settings.yml";
    $formItems = Yaml::parse(file_get_contents($iabYamlPath));
    foreach ($formItems as $key => $element) {
      if (!empty($element['#title'])) {
        $element['#title'] = $this->t($element['#title']);
      }

What's the suggested solution to make these strings translatable?

Also we don't have tag name 4.4.x-dev:

git tag 
4.2.0-rc1
4.3.0-rc1
4.3.1-rc1
4.3.2-rc1
4.3.3-rc1
4.4.0
4.4.1
8.x-3.5-rc1
8.x-3.6-rc1
8.x-3.7-rc1

Are you referring to the branch name? Shall we rename the branch?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

4.4.x-dev is reported on https://www.drupal.org/project/civicccookiecontrol/git-instructions, under Branch to work from. The ones reported from git tag are Git tags, not Git branches.

As far as I recall, branches cannot be renamed on Drupal.org. They need to be deleted and replaced by a different branch.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Configuration files are translatable, but those are placed in a specific directory, as described in Defining and using your own configuration in Drupal.

Using a generic YAML file to contain strings to translate isn't supported by Drupal.

tper’s picture

tper commented

Branches are now renamed as requested.

tper’s picture

tper commented

Hi @kiamlaluno,

The approach followed for the forms is actually inspired by webforms. We have the form configuration in our custom YAML files and we load/construct the form elements by parsing and loading the corresponding fields from YAML files. I'm aware that passing variables from t() function is discouraged in Drupal but changing this while keeping the translatability of forms means a large overhaul of the module to construct forms field by field. Is this something we could postpone and schedule to be fixed in a feature release or this is considered mandatory to get the security clearance?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Just avoid calling $this->t(), as it's not passing those sentences to the translation interface.

tper’s picture

tper commented

Hi @kiamlaluno,

We removed $this->t() where needed.

tper’s picture

tper commented

Hi @kiamlaluno,

Did you have the time to complete the review?

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work 
services:
  civiccookiecontrol.IAB1Settings:
    class: \Drupal\civiccookiecontrol\Form\IAB1Settings
    arguments: ['@config.factory','@config.factory', '@cache.data']
  civiccookiecontrol.IAB2Settings:
    class: \Drupal\civiccookiecontrol\Form\IAB2Settings
    arguments: ['@config.factory','@config.factory', '@cache.data', '@router.builder']

A form class cannot be used to implement a service.

  public function __construct(
        CountryManager $countryManager,
        ConfigFactoryInterface $config,
        CacheBackendInterface $cache
    ) {
    $this->countryManager = $countryManager;
    $this->config = $config->getEditable(CCCConfigNames::IAB);
    $this->cache = $cache;

    civiccookiecontrol_check_cookie_categories();
  }

The constructor of a class that extends ConfigFormBase or Formbase needs to call the parent constructor. There isn't any need to store the configuration handler in class properties, as there is already ConfigFormBase::config() to access the configuration.

  public function saveIabOption(array $form, FormStateInterface $form_state) {
    $this->config->set('iabCMP', $form_state->getValue('iabCMP'))->save();
  }

An AJAX callback isn't used for saving a setting, but changing how a form element is rendered. That AJAX callback isn't returning what Drupal expects, and it's probably not working as expected.

  /**
   * {@inheritdoc}
   */
  public function validateForm(array &$form, FormStateInterface $form_state) {
  }

If the class isn't validating any form element, it's not necessary to define an empty validateForm(); that method is already implemented by one of the base classes.

  /**
   * Entity type manager object from dependency injection.
   *
   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   */
  protected $entityTypeManager;

  /**
   * CookieCategoryForm constructor.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   *   The entityTypeManager.
   * @param \Drupal\Core\Cache\CacheBackendInterface $cache
   *   Cache interface.
   *
   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   */
  public function __construct(EntityTypeManagerInterface $entityTypeManager, CacheBackendInterface $cache) {
    $this->entityTypeManager = $entityTypeManager;
    $this->cache = $cache;
    $this->cookieCategories = $entityTypeManager->getStorage('cookiecategory')->loadMultiple();
  }

$entityTypeManager is already a property of the base class. The class constructor doesn't need to initialize that property. See the code used in the FieldConfigEditForm.php file.

tper’s picture

tper commented

Hi @kiamlaluno,

I just pushed changes in 4.4.x tackling issues raised in #24.
Please take a look and let me know.

tper’s picture

tper commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Priority: Minor » Normal
Status: Needs review » Fixed

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.