Fix website measurable type was defining settings and properties for all types #10991
Conversation
…all types The settings specified in website measurable type should not apply to any other type by default.
tsteur
added
Bug
| if ($this->idMeasurableType !== Type::ID && $this->idMeasurableType !== MobileAppType::ID) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Logically that would be right. But I'm wondering if there would be a better way to switch on/off the settings a measurable type defines. If for example someone wants to create another custom measurable type that should be based on the website settings. Could we maybe check the inheritance of the Type class or something like that and let the MobileType class inherit from the WebsiteType class?
There was a problem hiding this comment.
My plan for this was to create classes for all properties and settings similar to https://github.com/piwik/piwik/blob/3.x-dev/plugins/WebsiteMeasurable/Settings/Urls.php and https://github.com/piwik/piwik/blob/3.0.0-rc1/plugins/WebsiteMeasurable/MeasurableSettings.php#L87-L88
So one MeasurableSettings should actually not define anything for another type and each plugin that defines a new type would do eg:
$this->urls = new Urls($this->idSite);
$this->addSetting($this->urls);
$this->excludedUrls = new ExcludedUrls($this->idSite);
$this->addSetting($this->excludedUrls);
...This is a bit of duplicated code and not best solution but most flexible since often it will be cases that types will want to use only some fields of it.
Probably on top we will need to have something like your suggestion as well for cases where they actually want to have exact same fields, always. I will try to quickly define it like this
There was a problem hiding this comment.
I will use inheritance for now, but usually inheritance is a bit of a problem and rather not such a good solution but for now it will do
| protected function shouldShowSettingsForType($type) | ||
| { | ||
| return $type !== Type::ID; | ||
| } |
There was a problem hiding this comment.
shouldn't that be === in both files?
There was a problem hiding this comment.
could point, I have a look. Think you're right. It still worked because there were only 2 types so didn't notice :)
|
Guess it's good to merge now 👍 |
|
It broke the SitesManager.addSite method and won't save anything for any type anymore apart from websites. I will try to find a workaround but am a little stuck. Will see what is possible here without changing too much. |
|
Works now. SitesManager.addSite and updateSite can now be used with any type when calling the API and it still works. Be good to "squash" this PR :) |
The settings specified in website measurable type should not apply to any other type by default.