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

New events + some other misc changes #13388

Merged
merged 6 commits into from Sep 10, 2018

Conversation

Projects
None yet
2 participants
@diosmosis
Member

diosmosis commented Sep 5, 2018

Changes:

  • Added event Access.modifyUserAccess so plugins can directly edit user access w/o having to go to the DB.
  • Add a couple template events.
  • Use Request::process() for some LanguagesManager API calls so events will execute for it.
  • Use the result of Site.setSites event in the SitesManager API so the output will change. (this will change API output)

@diosmosis diosmosis added this to the 3.6.1 milestone Sep 5, 2018

@@ -168,6 +169,8 @@ public static function setSitesFromArray($sites)
self::setSiteFromArray($idSite, $site);
}
return $sites;

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

Can a return value be avoided as it is a "set" method? what gets modified here?

This comment has been minimized.

@diosmosis

diosmosis Sep 8, 2018

Member

The event can modify the list of sites and this function is called in SitesManager API. So if we don't return here, the API output isn't affected by any changes in the event.

This comment has been minimized.

@diosmosis

diosmosis Sep 8, 2018

Member

So if a plugin changes the name of a site, it will be reflected in the Site class cache, but not in SitesManager API output (and thus not in, eg, the site selector).

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

Would it be possible to call Sites::getSites instead of returning a value there? It is really not expected that a setter method changes the value and returns a different value. Maybe getSites would under circumstances return too many sites?

This comment has been minimized.

@diosmosis

diosmosis Sep 8, 2018

Member

I suppose it would work for getting all sites, but I don't think it would work when working w/ a subset of all sites (eg, getting one site or the pattern match sites method).

The alternative is to pass sites in by reference, but that could be a BC break. Personally I don't see an issue w/ it returning since it's not really setting a property but adding sites to a cache. Perhaps it would be better to rename this method to something like Site::addToSiteCache() and keep Site::setSite() as deprecated proxy to new method (for BC).

This comment has been minimized.

@tsteur

tsteur Sep 9, 2018

Member

I think it would be still quite unexpected. Could we maybe expose the method triggerSetSitesEvent somehow but possibly with a better name? Site::enrichSites or something? The set method does currently two things, enriching plus setting. We only want the "enriching" part so could just expose that method? Wouldn't be too much better as you would maybe already expect to get a site "enriched". It also means the "enriching part" would happen twice and possibly lead to problems? So it might not be an option either.

So OK to use the regular set method as already done. Let's just add a comment to the method to make clear what is happening and I wonder if we should mark the method as deprecated plus internal or so but not too important maybe as long as there is a good comment.

@diosmosis diosmosis force-pushed the more-events branch from 248dea9 to 2b400cd Sep 9, 2018

@diosmosis

This comment has been minimized.

Member

diosmosis commented Sep 10, 2018

Updated.

@tsteur

This comment has been minimized.

Member

tsteur commented Sep 10, 2018

Didn't test the PR but if tests pass looks good to merge.

@diosmosis diosmosis merged commit a977e87 into 3.x-dev Sep 10, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@diosmosis diosmosis deleted the more-events branch Sep 10, 2018

diosmosis added a commit that referenced this pull request Sep 20, 2018

New events + some other misc changes (#13388)
* Add Access.modifyUserAccess event.

* Add some template events & use request::process for LanguagesManager API.

* Use the result of Sites.setSites in SitesManager API.

* More comments for Site::setSitesFromArray().

* fixing plugin test.

InfinityVoid added a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018

New events + some other misc changes (matomo-org#13388)
* Add Access.modifyUserAccess event.

* Add some template events & use request::process for LanguagesManager API.

* Use the result of Sites.setSites in SitesManager API.

* More comments for Site::setSitesFromArray().

* fixing plugin test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment