search

perftools / php-profiler

A PHP profiling library based on XHGUI Data Collector
MIT License
147 stars 26 forks source link

Rename upload saver "uri" option to "url" #53

Closed glensc closed 4 years ago

glensc commented 4 years ago

Go with the flow, keep it as simple "url", no need to be a pedant about "uri"/"url".

Fallback to uri remains, so this is not a breaking change.

Manawyrm commented 4 years ago

It's a bit annoying that the documentation doesn't reflect this, I installed the current stable version via composer and had to debug it, because I set "url" (like mentioned in the documentation) instead of "uri". A new stable release or a (temporary) notice about this would be super useful :smiley:

glensc commented 4 years ago

The PR says:

Fallback to uri remains, so this is not a breaking change.

so, what you report means that it's broken and this was breaking change?

please share your configuration, as I don't understand how it could be broken.

Manawyrm commented 4 years ago

No, the PR is totally fine.

Here's what happend: I'm a new user that happened to just install the software freshly, I installed php-profiler via composer (and got a stable version, which still only supports the "uri" parameter. I looked at the example configuration which was given in the GitHub Readme (which was already updated to the url-Parameter).

And then I wondered why php-profiler wasn't working for me. A small notice saying something like "The current stable version in composer does not support the "url" parameter yet, use "uri"." would be nice. This notice could later be removed again.

glensc commented 4 years ago

@Manawyrm seems you still don't understand, thee PR should not have broken anything, using either of the option must work. altho I haven't tested/analyzed if you specify both, so what's your config you used that it broke to you?

Manawyrm commented 4 years ago

I'm sorry, I'm not a native english speaker, I might not be writing clearly enough.

What I'm trying to say is: the PR is totally fine, it did not break anything. I am using the current stable version of php-profiler which does not have the PR yet! But the documentation on GitHub here already has the PR. Which means it already talks about "url" as a parameter. Using "url" doesn't work without the PR. I already did that (because it said so in the documentation). It didn't work.

glensc commented 4 years ago

Oh, you are trying to say that this is not released? indeed, seems the release push was forgotten. tagging 0.16.0 now.

also, 0.x versions are not stable per semver:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Manawyrm commented 4 years ago

I wasn't sure if this change should've already been released or if you were still waiting for something else. This is why I wasn't saying that it should be released.

Yes, a new release would be perfect and fix this. Thank you :smile:

glensc commented 4 years ago

It was confusing that you claimed that documentation is not updated, while you just wanted to say that the PR has not included in any release (git tag).