Move JS snippets to separate script files

Hey @HeikkiY
I solved all the eslint and phpcs errors, they are all green now.
The only pipeline thats failing now is phpunit, and it is giving errors which I think, are not related to the JS files which I have added.
Some of the errors are of type "strings not found", like
"The string "https://yourname.containers.piwik.pro/" was not found anywhere in the HTML response of the current page."

Which, I think is to be provided in the module configurations, please take a look into this.

Hey @heikkiy,

So should I proceed with making changes in the test file "PiwikProSnippetTest.php", as there are many errors stating that "String not found" which could be possibly due to the external Js files are not being supported.

Also is this issue related to the same we are discussing ?

Also, should I remove checks for the sync snippets or should we leave them for now ( till Feb, 2025) ?

Hey @heikkiy,

The current issue fork does not contain the 1.2.x branch. I tried updating the fork and creating a new branch, and received this error : Failed to create branch '3482342-move-snippets-js': invalid reference name '1.2.x'.

Hey,

I have reverted the previous changes and added the mentioned functionality.

Changes made:

• New Setting: Added a boolean field to the module settings page, Load Piwik PRO snippet from a library, as per your suggestion. The default value is set to false, ensuring backward compatibility with existing implementations.
• Load Snippet from Library: Updated the module to check the new setting value. If the setting is TRUE, it will load the JS file from the external library. If FALSE, it will continue using the old method by injecting the Piwik PRO snippet directly into the DOM.
• Installation Hook: Added an installation hook to set the default value of the new setting (FALSE) for sites that already have the module installed.
• ESLint and PHPUnit Tests: The current ESLint and PHPUnit test failures are acknowledged and are still in progress. I am working on updating the tests to reflect the new script loading mechanism:
• When the setting is FALSE, the test will check if the Piwik PRO domain is present in the DOM.
• When the setting is TRUE, the test will validate if the new piwik_pro/piwik_pro_snippet library is properly loaded.

I am still working on finalizing the test validations. Please let me know if there are any other changes or adjustments that need to be made, and I will add them.

I am attaching the screen shot for the config form page.

Thanks for the guidance and let me know if you have any further feedback!

Thank you, @heikkiy.

The PHPCS and ESLint errors have been resolved as well. I am currently working on finalizing the PHPUnit test cases.

Hey,

I think the mentioned changes have been made, the tests are also running fine now.
Please have a look.
Thanks!

Hi @anish.ir and thank you for your contributions with this!

I tested your changes and noticed that the new library mode doesn't take into account the excluded pages and other exclusion settings when it's enabled.

So I mean that the method isVisible() should be used somewhere in the code, for example like this:

In the function:

function piwik_pro_page_top(array &$page) {

You could use isVisible() for example in here

if ($load_from_library && \Drupal::service('piwik_pro.snippet')->isVisible()) {

Normally it's used when getSnippet() is called, so when that gets skipped with the new library mode also isVisible() will be skipped.

Can you look into this a bit more? Thanks!

In addition to this, I was thinking would there be a better way to describe this setting in the admin UI? Some users might think that "external library" is something that is loaded from outside Drupal, like from another server entirely. Maybe the description text could be changed into "Check this to load the Piwik PRO snippet from a JavaScript library shipped with the module. "
What do you think? Any better ideas for the description text?

Changed status to needs work.

Hi,

Thanks for the feedback! I’ve made the following updates based on your suggestions:

• Ensured isVisible() is checked before adding the library in piwik_pro_page_top(), so the new script loading method respects exclusion settings.
• Improved the admin UI description to avoid confusion about "external libraries."

I've also cleared the cache and tested the changes, ensuring they work as expected. Let me know if there's anything else you'd like to adjust!

Thanks again!

Thanks @anish.ir!

Otherwise it looks good but I'm with @heikkiy here that there should be some new tests for this new setting, maybe?

I think the current tests assume the new setting "piwik_pro_load_from_library" is NULL and that's why isVisible() doesn't have any effect.

I would suggest adding maybe a new testing function to PiwikProSnippetTest.php
I'd say maybe one new testing function would be enough to test if the script configs get added to source when piwik_pro_load_from_library is set to TRUE.
Basically even the same kind of assertion should work when using the library mode, because the piwik_domain should exist in the html source code within the drupal-settings-json

    $this->assertSession()->responseContains((string) $this->config->get('piwik_domain'));

"piwik_pro":{"piwik_domain":"https:\/\/yourname.containers.piwik.pro/\/"

Hi,

I've added the test case for the Piwik PRO snippet when loaded via the library. The test ensures that:

• The configuration is set correctly.
• The snippet is loaded from the library.
• The expected piwik_domain appears in the drupal-settings-json script.

Please review the changes and let me know if any modifications are needed. Thanks for your guidance!

Hey,

Since the MR have been merged and the changes have already been verified and reviewed, I think we may transition this to Fixed. Please let me know If I am wrong here.
Thanks !

Moved new features to 1.3.x-dev and kept 1.2.x. as the supported version with only bug fixes.