matomo-org / matomo-sdk-ios

Matomo iOS, tvOS and macOS SDK: a Matomo tracker written in Swift
MIT License
388 stars 164 forks source link

Remove assertion preventing initialization #288

Closed TheIronMarx closed 5 years ago

TheIronMarx commented 5 years ago

As of Matomo SDK 6, the first line of the convenience initializer is the following assertion.

@objc convenience public init(siteId: String, baseURL: URL, userAgent: String? = nil) {
assert(baseURL.absoluteString.hasSuffix("piwik.php"), "The baseURL is expected to end in piwik.php")
// ...
}

On top of being out of date and incorrect, it seems like a really needless enforcement. At worst, maybe a warning or some other nonblocking logs to the console. Furthermore, this is immediately stopping me from continuing as our baseURL does not conclude with piwik.php but matomo.php.

Thoughts?

brototyp commented 5 years ago

Hi @TheIronMarx,

yes you are correct. That should be changed to also allow matomo.php. There was a ticket in this regard but it got closed.

I will accept PR's that change this. In the meantime you can simply change your baseURL to end on piwik.php instead of matomo.php. Both are supported.

TheIronMarx commented 5 years ago

Yeah I see that! Whew. I'll get to the PR later this week.

Hopefully.

In the mean time, opinions on changing the assertion to a print out to the console?

brototyp commented 5 years ago

I don't think there is a reason to use a URL other than ending in either matomo.php or piwik.php or am I missing something here?

Changing it to just a log makes it far easier missable. If one misses that there is just no data in the backend at all. Having an assertion here will probably save many developers time debugging and me quite a bit time supporting.

Unless there is a strong reason to remove the assertion I guess I would keep it.

TheIronMarx commented 5 years ago

Alright. I suppose that's much more your call than mine. I'll include both in the PR.

brototyp commented 5 years ago

Oh man, super sorry about that. I already implemented the changes but never merged them. There is an open PR in this regard: #286. Looks like my brain isn't fully booted for the start of the week.

brototyp commented 5 years ago

I just merged that PR. It will be part of the next release.

TheIronMarx commented 5 years ago

Thank you!