Nexmo / nexmo-laravel

Add Vonage functionality such as SMS and voice calling to your Laravel app with this Laravel Service Provider.
MIT License
317 stars 79 forks source link

Add support for app name in user agent, and some defaults #30

Closed lornajane closed 5 years ago

lornajane commented 5 years ago

I've defaulted the app version to 1.1.2 which I guess will be our next release - but we could do with a better way to keep this in sync in future. Related to https://github.com/Nexmo/nexmo-php/pull/112 but I think this change is still an improvement.

mheap commented 5 years ago

This is working great for me.

We wondered how it would work if people had prior versions installed. It turns out that Laravel is fairly clever.

When php artisan package:discover is run it finds ./vendor/nexmo/laravel/config/nexmo.php and registers it in bootstrap/cache/packages.php. This means that the provider is always loaded from the vendor folder.

If a user runs php artisan vendor:publish and generates a copy in their project it takes priority, but Laravel still loads the config in the vendor folder. This means any missing keys in the local config/nexmo.php (e.g. app) are populated from the vendor folder.

This means that it's a safe upgrade for all users, and we'll start collecting user agent data from all consumers.

lornajane commented 5 years ago

Hmm, I can document it but I intentionally didn't since in most cases we don't want users to change these values, we'd rather get the data of which library version is used.

mheap commented 5 years ago

That feels like a workaround rather than a solution to me. This is a way to allow people to specify custom user agents in app when using nexmo-laravel. I think that info should come from our PackageVersions work.

mheap commented 5 years ago

@lornajane How would you like to proceed with this? Document the env variables or remove them and have hardcoded values in the provider in the app section (meaning that we always track the nexmo-laravel version)

Allowing the user to change them but not documenting it feels wrong to me

afolson commented 5 years ago

Normally I'd agree, but this is for our own telemetry so I'm not sure we want to encourage people to change it by documenting it. base64_decode() and make it more effort? :D (Totally kidding here, please don't do this.) How much do we need to document? Is it enough to just put some comments in the config file? We can maybe explain why it's there in that section? But...that's a huge comment too.

mheap commented 5 years ago

If it's for our own telemetry we shouldn't make it customisable at all imo. If the user can use it we should document it.

It's the same as our APIs for me - we shouldn't have special features that aren't documented

lornajane commented 5 years ago

This is a good discussion about the data collection via User Agents although I didn't realise we had so much to say on the topic (myself included)! I want to note that I'm not wedded to this - I'm working on demos so part of that is improving our data collection and this was one part of that since we don't currently have usage stats for this project (but I've also added it to the package management tracking so it will be interesting to compare the two).

The ability to set an app name and version exists on the PHP library but isn't documented there - it allows us to set the app name and version on the sample apps we build. For nexmo-laravel it is a sample app but it might not be the final thing in the chain - it's a library really rather than an app. So while it makes sense to set the app name and version as if it were a sample app, it might not be helpful to hardcode in case we built something like DeskMo using this library and wanted to use the app name and version of that instead in that app - this makes those variables available to either ourselves or a user if they want to use it (I am not sure why they would but it's all data!).

The additional environment variables have descriptive comments in config/nexmo.php which brings them into line with other environment variables that are available but not specifically called out in the README, such as NEXMO_SIGNATURE_SECRET which some users may want but probably aren't in the majority use case.

afolson commented 5 years ago

So I was typing this up as you posted that, Lorna:

You're totally going to be like "Amanda, you bikeshedding charlatan no stop" but:

Maybe we can add their APP_NAME (or some string like "nexmo-laravel" that we pass as a variable) to the user agent in nexmo-php somehow? That also creates some fun options for demos and other PHP frameworks. But I'm also aware that it's WAY out of scope for this as it's something we may have to agree to adopt across ALL libraries. If I'm honest I almost prefer this so we can encourage users to tell us who they are* without breaking stuff for us.

lornajane commented 5 years ago

Amanda, you are spot on and the PHP library already has this feature! It does a series of app/version pairs: we get one for PHP itself, one for the library, and then the ability to set an app name and version - which is what this PR uses but it sets default values on the basis that most users won't override them.

You will love what we're planning to do with detecting frameworks automatically :)

afolson commented 5 years ago

Well, I guess that tells you how long it's been since I've looked at it 🤣 Wonder if I should write more code...

mheap commented 5 years ago

Given the context of NEXMO_SIGNATURE_SECRET, Deskmo as an example and in the interests of collecting data, I'm convinced! Let's get this shipped