Skip to content
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

Make sure to always trigger site event when creating a new site instance #11001

Merged
merged 1 commit into from Dec 14, 2016

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Dec 13, 2016

In #10990 I already tried to achieve this but noticed there are still case when this is not working. For example any request to SitesManager.getSiteFromId will overwrite any site information in Site::$infoSites.

Meaning a controller might do new Site(), then further down an API request, or the archiver does SitesManager.getSiteFromId and overwrites a previously enriched site. It is even more messy that at any point the data within new Site() might be changed by another API request or method that calls Sites::set*.

It is much better to instead keep the site in a non-static property when an instance is created and to always work on that data. This is much more reliable and what consumers expect. In theory, there could be a case that a site is deleted or updated via an API request, and then the data within Site->site is not updated automatically but this is not a problem for us.

Having this PR merged will make sure that when you change eg the name of a website with the event Site.setSites, it will appear correctly in the website selector and in the mobile app.

There are still more inconsistencies to tackle eventually. Eg all SitesManager.get* methods might trigger the Site.getSites event letting consumers change the site objects, but none of the APIs actually return the changed value. This is much harder to solve though and therefore not included in this PR

@tsteur tsteur added c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review labels Dec 13, 2016
@tsteur tsteur added this to the 3.0.0-rc milestone Dec 13, 2016
@mattab mattab merged commit cc9fc7a into 3.x-dev Dec 14, 2016
@mattab mattab deleted the site_trigger_event branch December 14, 2016 22:54
@mattab
Copy link
Member

mattab commented Dec 14, 2016

Good find 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants