Add user properties convenience methods #9
Conversation
| * | ||
| * @param array $userProperties | ||
| */ | ||
| public function addUserProperties(array $userProperties) | ||
| { | ||
| $this->userProperties = array_merge($this->userProperties, $userProperties); | ||
| $this->userProperties = $this->userProperties + $userProperties; |
There was a problem hiding this comment.
@jonyo Shouldn't be the other way round? This is ignoring keys that are previously defined.
There was a problem hiding this comment.
@jrbasso I figured if you are calling a method named addUserProperties you would expect it to add any new properties and may be surprised if existing properties get overwritten. Otherwise mergeUserProperties() might be a better name...
There was a problem hiding this comment.
@jonyo Hmmmm, silently ignoring data could be a problem too, even on purpose. Maybe the merge name fits better.
There was a problem hiding this comment.
The reason I originally used addUserProperties() name to begin with: in Amplitude, you are always adding to or changing existing properties, you are never clearing old properties.
The above changes were because I figured, well, what I said above, the name implies it would not overwrite, just add new properties.
There was a problem hiding this comment.
@jrbasso When in doubt, I guess look at how the official version works...
Looking at the JS SDK, they use setUserProperties(). I could change to use that?
There was a problem hiding this comment.
@jonyo Sounds good to me. Using the same name of other SDKs is also a good thing. 👍
|
Closing, going to use |
This adds method
addUserProperties()to event object, to easily add properties. It also changes the behavior of$amplitude->addUserProperties()to only add new properties, not change ones already set, so there are no surprises with the name addUserProperties...