https://github.com/matomo-org/matomo/pull/17457 - Mouve

Site d'origine

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround error in Overlay when site has no URLs #17457

Merged
merged 6 commits into from Apr 15, 2021
Merged

Conversation

@diosmosis
Copy link
Member

@diosmosis diosmosis commented Apr 14, 2021

Description:

When creating a site it is possible to not specify URLs. In this case, we send a plugin setting entry in the HTTP request as ['name' => 'urls'] w/ no value key. Currently this is ignored by Matomo, and the validation that happens does not occur. Which allows sites to have no URLs.

If this happens and Overlay is loaded, the JS will error when trying to find the host of the site URLs.

This is fixed in this PR w/ tests. The issue in Overlay is also worked around in case there are users who created sites w/ no URLs.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed
@diosmosis diosmosis added this to the 4.3.0 milestone Apr 14, 2021
@@ -598,7 +598,7 @@ public function addSite($siteName,
}

$coreProperties = array();
$coreProperties = $this->setSettingValue('urls', $urls, $coreProperties, $settingValues);
$coreProperties = $this->setSettingValue('urls', $urls, $coreProperties, $settingValues, $isRequired = true);

This comment has been minimized.

@sgiehl

sgiehl Apr 14, 2021
Member

This makes it impossible to create any new roll ups, as for roll ups you can't define any urls.

This comment has been minimized.

@diosmosis

diosmosis Apr 14, 2021
Author Member

Good point. I guess I can make it required if type == 'website'. (cc @tsteur)

@tsteur
Copy link
Member

@tsteur tsteur commented Apr 14, 2021

@diosmosis this would be breaking API (even though in the code it should have already worked but from a user perspective things are potentially still breaking as it probably wasn't working for a while) and not sure if want to require URLs now or maybe wait to Matomo 5. Is there an issue for this PR?

@diosmosis
Copy link
Member Author

@diosmosis diosmosis commented Apr 14, 2021

@tsteur this is to fix the root cause of L3-65

@diosmosis
Copy link
Member Author

@diosmosis diosmosis commented Apr 15, 2021

@tsteur updated. Warning looks like:

image

@tsteur
Copy link
Member

@tsteur tsteur commented Apr 15, 2021

👍 be great to have an issue to fix the addSite/updateSite API regarding requiring URLs as part of Matomo 5

@diosmosis diosmosis changed the title Do not allow sites to be created w/ empty URLs and workaround this case in Overlay Workaround error in Overlay when site has no URLs Apr 15, 2021
@diosmosis
Copy link
Member Author

@diosmosis diosmosis commented Apr 15, 2021

Created #17459

@diosmosis diosmosis merged commit 7e091d1 into 4.x-dev Apr 15, 2021
1 of 3 checks passed
1 of 3 checks passed
@github-actions
PHPCS
Details
@travis-ci
Travis CI - Branch Build Failed
Details
@travis-ci
Travis CI - Pull Request Build Failed
Details
@diosmosis diosmosis deleted the l3-65-site-url-empty branch Apr 15, 2021
diosmosis added a commit that referenced this pull request Apr 18, 2021
* get segment hash

* convert tab indentations to spaces

* Create 4.3.0-b2.php

* update tests and update file

* bump version

* Update 4.3.0-b3.php

* Update ApiTest.php

* fixing urlencode bugs

* cache segment hashes

* update segment caching

* update segment caching

* add segment cache test

* add testdox to phpunit.xml

* revert phunit.xml

* test investigation

* Update phpunit.xml.dist

* update blobreportlimitingt test

* Revert "update blobreportlimitingt test"

This reverts commit 90fe735.

* Update phpunit.xml.dist

* Update BlobReportLimitingTest.php

* Update BlobReportLimitingTest.php

* Update SystemTestCase.php

* Update BlobReportLimitingTest.php

* Update SystemTestCase.php

* Update phpunit.xml.dist

* modify mem limit for travis

* Update .travis.yml

* revert travis.yml

* Update SystemTestCase.php

* try test without cache

* test witch cache and gc_disabled

* Revert "test witch cache and gc_disabled"

This reverts commit 7e1d370.

* test witch cache and gc_disabled

* use other model method

* refactor test

* Workaround error in Overlay when site has no URLs (#17457)

* Set setting value even if set to NULL so it will still be validated.

* Make sure when creating a site that the urls options is set.

* workaround in Overlay for instances that have an invalid site URL set for some reason

* Add integration tests for changes to SitesManager API.

* revert non-overlay changes

* Add warning if site has no URLs when viewing Overlay.

* add more tests for segment caches

* get segment hash

* convert tab indentations to spaces

* Create 4.3.0-b2.php

* update tests and update file

* bump version

* Update 4.3.0-b3.php

* Update ApiTest.php

* fixing urlencode bugs

* cache segment hashes

* update segment caching

* update segment caching

* add segment cache test

* add testdox to phpunit.xml

* revert phunit.xml

* test investigation

* Update phpunit.xml.dist

* update blobreportlimitingt test

* Revert "update blobreportlimitingt test"

This reverts commit 90fe735.

* Update phpunit.xml.dist

* Update BlobReportLimitingTest.php

* Update BlobReportLimitingTest.php

* Update SystemTestCase.php

* Update BlobReportLimitingTest.php

* Update SystemTestCase.php

* Update phpunit.xml.dist

* modify mem limit for travis

* Update .travis.yml

* revert travis.yml

* Update SystemTestCase.php

* try test without cache

* test witch cache and gc_disabled

* Revert "test witch cache and gc_disabled"

This reverts commit 7e1d370.

* test witch cache and gc_disabled

* use other model method

* refactor test

* add more tests for segment caches

* revert phpunit.xml

* revert phpunit.xml

* add more tests

Co-authored-by: dizzy <diosmosis@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants

level de log mis à 1

Récupération du document https://github.com/matomo-org/matomo/pull/17457.

Récupération de l'URL https://github.com/matomo-org/matomo/pull/17457...

Raccourcis

Commandes

Fermer