Make the download url method more re-usable/overloadable

[jrfnl]

No description provided.

[YahnisElsts]

Making the URL generation method public might be a good idea, but introduce a whole another helper function just for slug-based URLs? I'd say generateDownloadUrl($package) is just as overloadable and it provides more information to subclasses like the current version number and last modification time. Instead of being restricted to bare, slug-based URLs, subclasses can override generateDownloadUrl() to create download URLs like, say "plugin-name-v1.2.zip".

[jrfnl]

Actually there is a reason for that: as you used type hinting, a method overloading the original can only have a Wpup_Package object as the first parameter.
I could remove the type hint, but didn't want to break compatibility with implementations counting on the type hint.

The actual implementation I'm using it in is one which needs the download url to be a package attribute, so when you generate the download url, the package object isn't available yet.

So that's how I ended up with the currently suggested PR ;-)

[YahnisElsts]

The actual implementation I'm using it in is one which needs the download url to be a package attribute, so when you generate the download url, the package object isn't available yet.

That's actually a pretty big change in dependencies. Instead of generating a download URL for a package, it inverts that by building a package instance using a download URL. I doubt it's realistically possible for an API to support both of those approaches at the same time.

The deeper questions seems to be, does the download URL belong to a package, or is it something that pertains to the server itself? I'd say it's a responsibility of the server since it can change without the package changing (server base URL changes, etc).

On the other hand, treating it as a property of the package is also reasonable. This, however, makes the package more tightly coupled to the server implementation/configuration since you can't get the URL without involving the server somehow. That's not necessarily wrong, but it changes the meaning of what a package is.

In any case, having two overridable URL generation functions is probably not a good idea; with this PR if someone overrides generateDownloadUrl, it and getDownloadUrl will start producing different results. That would be unnecessarily confusing and it's extra work for the hypothetical developer.

[jrfnl]

The deeper questions seems to be, does the download URL belong to a package, or is it something that pertains to the server itself?

Very true. IMHO it's a package property, the server is only the facilitator and if you'd move server and the download url would change, that wouldn't be much of an issue anyway as the cache would be (should be) regenerated on a different server anyway.

The reason for having it split into two is that - again IMHO - even though in the current implementation, they are very closely related, they don't have to be.
The one (original) function decides which data to use from the Package to pass to the url. The one function defines how the url is build up. Maybe I should make that one actually more generic by passing it an array of data to add to the url.
In the specific implementation I'm working on that would be a slug and version nr, not just a slug.

Anyways, not to worry. I'll overload this in the local implementation. Just thought I'd put it out there as I can imagine other people might run into this as well.

[ddur]

I'm very happy with current generateDownloadUrl method. It allows me to override it and provide download path to the Puc client on same host.

Hopefully, proposed changes won't break that functionality.