Problem/Motivation

* Add gitlab ci-pipeline
* Fix issues that are reported
* Fix issues reported by HeikkiY
* Create a starting point for collaboration

Issue fork piwik_pro-3448034

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

JoshaHubbers created an issue. See original summary.

joshahubbers’s picture

Assigned: Unassigned » joshahubbers

joshahubbers’s picture

Oops, pushed the gitlab-ci.yml file directly to the 1.0.x branch. But fixes will be done in this MR.

joshahubbers’s picture

Done:
* gitlab ci added
* composer.json added
* .gitignore added
* cspell words list added
* test fixted

Todo:
* phpstan fixes

joshahubbers’s picture

Done:
* gitlab ci added
* composer.json added
* .gitignore added
* cspell words list added
* test fixted
* most phpstan fixes. Some phpstan errors are coming from hook-declarations that are inherited from core.

joshahubbers’s picture

Assigned: joshahubbers » heikkiy
Status: Active » Needs review
heikkiy’s picture

Thanks for the fixes!

The original test report was sent to @joshahubbers-0 through e-mail. The module was audited in collaboration with Cookie information who owns Piwik Pro currently. I will add the PHPStan and PHPUnit test results here so that we have the completely history also in Drupal.org.

The original audit was done for level 9 but that is probably too strict level because even core doesn't validate for that. Looking at https://github.com/mglaman/phpstan-drupal/blob/main/phpstan.neon it seems like level 8 would be good. I will test your changes and report any findings.

------ -----------------------------------------------------------------------------------------------------------------------
Line piwik_pro.module
------ -----------------------------------------------------------------------------------------------------------------------
17 Function piwik_pro_help() has no return type specified.
17 Function piwik_pro_help() has parameter $route_name with no type specified.
37 Function piwik_pro_page_attachments() has no return type specified.
37 Function piwik_pro_page_attachments() has parameter $attachments with no value type specified in iterable type array.
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iter...
56 Function piwik_pro_page_top() has no return type specified.
56 Function piwik_pro_page_top() has parameter $page with no type specified.
------ -----------------------------------------------------------------------------------------------------------------------

------ --------------------------------------------------------------------------------------------------------------------------------------------------
Line src/Form/PiwikProAdminSettingsForm.php
------ --------------------------------------------------------------------------------------------------------------------------------------------------
23 Method Drupal\piwik_pro\Form\PiwikProAdminSettingsForm::getEditableConfigNames() return type has no value type specified in iterable type array.
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iter...
30 Method Drupal\piwik_pro\Form\PiwikProAdminSettingsForm::buildForm() has parameter $form with no value type specified in iterable type array.
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iter...
30 Method Drupal\piwik_pro\Form\PiwikProAdminSettingsForm::buildForm() return type has no value type specified in iterable type array.
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iter...
102 Method Drupal\piwik_pro\Form\PiwikProAdminSettingsForm::validateForm() has no return type specified.
102 Method Drupal\piwik_pro\Form\PiwikProAdminSettingsForm::validateForm() has parameter $form with no value type specified in iterable type array.
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iter...
106 Parameter #1 $string of function rtrim expects string, mixed given.
112 Parameter #1 $string of function strtolower expects string, mixed given.
118 Parameter #2 $subject of function preg_match expects string, mixed given.
123 Parameter #1 $string of function trim expects string, mixed given.
127 Parameter #2 $subject of function preg_split expects string, mixed given.
128 Argument of an invalid type array|false supplied for foreach, only iterables are supported.
140 Method Drupal\piwik_pro\Form\PiwikProAdminSettingsForm::submitForm() has no return type specified.
140 Method Drupal\piwik_pro\Form\PiwikProAdminSettingsForm::submitForm() has parameter $form with no value type specified in iterable type array.
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iter...
145 Parameter #1 $string of function substr expects string, mixed given.
------ --------------------------------------------------------------------------------------------------------------------------------------------------

------ ----------------------------------------------------------------------------------------------------------------------
Line src/PiwikProSnippet.php
------ ----------------------------------------------------------------------------------------------------------------------
83 Method Drupal\piwik_pro\PiwikProSnippet::getScript() return type has no value type specified in iterable type array.
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iter...
97 Parameter #2 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
97 Parameter #3 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
97 Parameter #4 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
100 Parameter #2 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
100 Parameter #3 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
109 Method Drupal\piwik_pro\PiwikProSnippet::getScript() should return array but returns null.
131 Parameter #2 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
131 Parameter #3 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
131 Parameter #4 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
137 Method Drupal\piwik_pro\PiwikProSnippet::getSyncScript() should return string but returns null.
145 Method Drupal\piwik_pro\PiwikProSnippet::getVisibilityPages() has no return type specified.
155 Parameter #1 $string of function mb_strtolower expects string, mixed given.
------ ----------------------------------------------------------------------------------------------------------------------

------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------
Line tests/src/Functional/PiwikProSnippetTest.php
------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------
53 Property Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::$config (Drupal\Core\Config\ImmutableConfig) does not accept Drupal\Core\Config\Config.
71 Parameter #1 $account of method Drupal\Tests\BrowserTestBase::drupalLogin() expects Drupal\Core\Session\AccountInterface, Drupal\user\Entity\User|false given.
80 Method Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testPiwikProConfiguration() has no return type specified.
93 Method Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testPiwikProHelp() has no return type specified.
96 Call to an undefined method Drupal\Tests\WebAssert::pageTextContains().
100 Call to an undefined method Drupal\Tests\WebAssert::pageTextContains().
106 Method Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testSnippetVisibility() has no return type specified.
110 Parameter #2 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
111 Parameter #3 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
113 Parameter #1 $text of method Drupal\Tests\WebAssert::responseContains() expects object|string, mixed given.
117 Parameter #2 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
122 Parameter #1 $text of method Drupal\Tests\WebAssert::responseNotContains() expects object|string, mixed given.
128 Method Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testSnippetVisibilityPagesInverted() has no return type specified.
133 Parameter #1 $text of method Drupal\Tests\WebAssert::responseNotContains() expects object|string, mixed given.
136 Parameter #1 $text of method Drupal\Tests\WebAssert::responseContains() expects object|string, mixed given.
142 Method Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testSnippetVisibilitySyncSnippet() has no return type specified.
148 Parameter #2 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
155 Method Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testSnippetVisibilityChangedDataLayer() has no return type specified.
162 Parameter #3 ...$values of function sprintf expects bool|float|int|string|null, mixed given.
169 Method Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testSnippetVisibilityInvalidSettings() has no return type specified.
205 Method Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testSettingsForm() has no return type specified.
207 Call to an undefined method Drupal\Tests\WebAssert::statusCodeEquals().
208 Call to an undefined method Drupal\Tests\WebAssert::pageTextContains().
209 Call to an undefined method Drupal\Tests\WebAssert::elementExists().
------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------

[ERROR] Found 57 errors

Also the PHPUnit test results were the following

Piwik Pro Snippet (Drupal\Tests\piwik_pro\Functional\PiwikProSnippet)
✔ Piwik pro configuration
✘ Piwik pro help

│ Behat\Mink\Exception\ResponseTextException: The text "Piwik Pro is a GDPR-proof tracking tool that allows you to track user visits." was not found anywhere in the text of the current page.

│ /var/www/html/vendor/behat/mink/src/WebAssert.php:907
│ /var/www/html/vendor/behat/mink/src/WebAssert.php:293
│ /var/www/html/tests/src/Functional/PiwikProSnippetTest.php:100
│ /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

✔ Snippet visibility
✔ Snippet visibility pages inverted
✔ Snippet visibility sync snippet
✔ Snippet visibility changed data layer
✔ Snippet visibility invalid settings
✘ Settings form

│ TypeError: Behat\Mink\Element\TraversableElement::findButton(): Argument #1 ($locator) must be of type string, Drupal\Core\StringTranslation\TranslatableMarkup given, called in /var/www/html/web/core/tests/Drupal/Tests/WebAssert.php on line 143

│ /var/www/html/vendor/behat/mink/src/Element/TraversableElement.php:97
│ /var/www/html/web/core/tests/Drupal/Tests/WebAssert.php:143
│ /var/www/html/web/core/tests/Drupal/Tests/UiHelperTrait.php:78
│ /var/www/html/tests/src/Functional/PiwikProSnippetTest.php:215
│ /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

Time: 00:13.857, Memory: 4.00 MB

Summary of non-successful tests:

Piwik Pro Snippet (Drupal\Tests\piwik_pro\Functional\PiwikProSnippet)
✘ Piwik pro help

│ Behat\Mink\Exception\ResponseTextException: The text "Piwik Pro is a GDPR-proof tracking tool that allows you to track user visits." was not found anywhere in the text of the current page.

│ /var/www/html/vendor/behat/mink/src/WebAssert.php:907
│ /var/www/html/vendor/behat/mink/src/WebAssert.php:293
│ /var/www/html/tests/src/Functional/PiwikProSnippetTest.php:100
│ /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

✘ Settings form

│ TypeError: Behat\Mink\Element\TraversableElement::findButton(): Argument #1 ($locator) must be of type string, Drupal\Core\StringTranslation\TranslatableMarkup given, called in /var/www/html/web/core/tests/Drupal/Tests/WebAssert.php on line 143

│ /var/www/html/vendor/behat/mink/src/Element/TraversableElement.php:97
│ /var/www/html/web/core/tests/Drupal/Tests/WebAssert.php:143
│ /var/www/html/web/core/tests/Drupal/Tests/UiHelperTrait.php:78
│ /var/www/html/tests/src/Functional/PiwikProSnippetTest.php:215
│ /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

ERRORS!
Tests: 8, Assertions: 75, Errors: 2.
Failed to run phpunit : exit status 2

heikkiy’s picture

Confirmed that all PHPUnit tests pass now.

joshahubbers’s picture

Hi @HeikkiY.

Sorry for my absense. Had a very busy time lately. Switched work and also personally. I will look at your comments as soon as possible, but some quick responses:

* I run the tests with the ddev tooling
* git instructions are updated automatically on d.org normally as soon as one starts committing in the repo. Strange that it doesn't here.
* I agree that we can start a new major branch, and drop support for older Drupal versions. I would suggest only supporting the currently supported versions in the new branch

Maybe a good start to commit the fixes in this MR, because most of the issues are solved. And than we move to a new 2.x branch to start working on a new release?

heikkiy’s picture

Agreed. We can review these with @kekkis and approve the MR as soon as possible.

heikkiy’s picture

@joshahubbers-0 Can you change the MR from Draft to an actual MR?

Also there are a couple of comments to the MR which would be low hanging fruits like adding the core requirement and PHP requirement to composer.json.

joshahubbers’s picture

Fine by me! Lets release.

joshahubbers’s picture

I had the same reasoning as you. If someone is on 8.x, then I don't think they will be doing minor modules updates either...

joshahubbers’s picture

Status: Needs review » Reviewed & tested by the community

  • HeikkiY committed dab13cca on 1.0.x authored by JoshaHubbers
    Issue #3448034 by JoshaHubbers, HeikkiY: Add gitlab ci and fix issues,...
joshahubbers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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