Fix PHP8 Trim Error #18903
Fix PHP8 Trim Error #18903
Conversation
An error happens if sites if names or skus are arrays in PHP 8 > Uncaught exception in core/Tracker/GoalManager.php line 567: > trim(): Argument matomo-org#1 ($string) must be of type string, array given
There was a problem hiding this comment.
Hey @deybhayden. Thanks for creating this PR. The code looks fine.
I guess actually name and sku are meant to be passed as a string only, as multiple values for a single product won't make that much sense. But guess it shouldn't hurt to convert arrays to strings to avoid possible errors.
@tsteur what do you think about that? Is it fine to change that, or should an exception be thrown if the values are not provided as strings?
|
Awesome! Yeah, I wasn't sure if the fix made sense to the wider community. However, when we upgraded an Matomo install for a client, we started hitting this error as tracking requests tried to process on PHP 8. We didn't have the ability to tell sites to change their tracking code, so this is the fix we went with to get past the exception. |
|
I'm having mixed feelings :) On one side it's not a big harm, on the other side once it's in there we need to keep supporting it and it might cause a regression in the future if we need to change behaviour around that. Considering it's only done for SKU and name it may be fine by me. It be different if it was done for a category as products can be actually assigned to multiple categories. Note that if we merge this, it could regress any time and we wouldn't make it a supported feature. Meaning this could actually cause issues again in the future if we support this now, then stop supporting it for some reason and then we're breaking it. I mean when people don't realise they send an array there. It should all be edge case though and be fine to merge I suppose. @sgiehl do you have any thoughts? @deybhayden did this tracking actually ever work using an array? I wonder if it wouldn't have recorded like |
|
So, the change I did here isn't exactly what we did for our client, most of the examples were like this: $name = ['shirt'] So, we just |
|
@deybhayden actually the tracking on the website of the client seems to be incorrect, as they are sending an array for name, which shouldn't be the case. Our documentation also says it should be given as string. Lines 6745 to 6755 in 05082ab I guess the name could be set to an array at that point and would then be passed to the tracking. We actually also could change the javascript tracker so it throws an error if someone tries to set an sku or name as an array. So someone implementing the tracking directly sees it won't work. We could merge this to be a bit less error-prone when incorrect data is sent with the request. But as @tsteur this won't be officially supported and could break or change the behavior again any time in the future. |
|
Nice sleuthing! I didn't know enough to be able to dig into the JS library to follow the steps all the way through. I'm happy to change the code to throw errors or update the JS if that's the right path? I'm definitely a bit out of depth when it comes to the right solve. |
|
@sgiehl sounds good to merge it 👍 |
Description:
An error happens if sites if names or skus are arrays in PHP 8 (was just a warning in PHP 7)
Review