Closed jackdeguest closed 6 months ago
I don't really like this change. I don't think it should be poking into the internals of objects.
I don't really like this change. I don't think it should be poking into the internals of objects.
How do you see it poking into the internals of objects
? It merely checks if the type is an array, including array objects, nothing more.
I don't think it should be poking into the internals of objects
It seems this is just a variation on what the code is already doing?
@jackdeguest the initial version of this triggered warnings when it got into the wild. Would you be able to take a quick look at the test coverage and see if we can improve it before releasing this? There may be some easy wins there and we'd be able to release with more confidence.
https://app.codecov.io/gh/libwww-perl/URI/blob/master/lib%2FURI%2F_query.pm
@jackdeguest the initial version of this triggered warnings when it got into the wild. Would you be able to take a quick look at the test coverage and see if we can improve it before releasing this? There may be some easy wins there and we'd be able to release with more confidence.
https://app.codecov.io/gh/libwww-perl/URI/blob/master/lib%2FURI%2F_query.pm
Thank you Olaf. Sorry I am a bit puzzled, because the code you reference is not my version, but the version before my change, or did I miss something?
@jackdeguest I believe the coverage will not have changed in your branch. Or it will not have gone up, since there are no tests added, so that seemed safe to reference. I'm not sure why coverage reports are not being created here. Might be an issue with GitHub secrets on PRs from outside of the org.
@jackdeguest I believe the coverage will not have changed in your branch. Or it will not have gone up, since there are no tests added, so that seemed safe to reference. I'm not sure why coverage reports are not being created here. Might be an issue with GitHub secrets on PRs from outside of the org.
Ok, then I propose to add some tests also using array objects, since this is the purpose of this change. Would that be satisfactory?
That sounds good. Can we do this without adding new dependencies?
That sounds good. Can we do this without adding new dependencies?
Of course
I have added tests in files t/old-base.t
, t/query-param.t
and t/query.t
.
I could run all tests without problem. Can you please check?
How do you see it
poking into the internals of objects
? It merely checks if the type is an array, including array objects, nothing more.
Previously, if given an object, it would not try to dereference. The inside of an object is supposed to be an internal implementation detail which you aren't meant to look inside. With this check, objects are handled differently depending on what their internal implementation is.
It seems this is just a variation on what the code is already doing?
The current code will reject handling an object at all, treating it the same as a string.
Consider an object that implements string overloading and is intended to be used that way.
Consider an object that implements string overloading and is intended to be used that way.
So, I think what you're saying is that someone could provide an object which is a blessed arrayref, but that it would be incorrect to make a blanket assumption that dereferencing the array would yield what we expect, because we'd be making an assumption about how the internals of the object are stored.
Looking at the docs for ref
:
If the operand is a reference to a blessed object, then the name of the class into which the referent is blessed will be returned
So, previously with a blessed array, we never would have tried to turn treat it as an array, but would have tried to stringify it, which would work in cases where stringification was overloaded.
(Edited my initial comment as I realized I hadn't fully grasped the issue in my first pass).
So, previously with a blessed array, we never would have tried to turn treat it as an array, but would have tried to stringify it, which would work in cases where stringification was overloaded.
Would it then be satisfactory if we checked if said array object has stringification and then pass it through ?
I think a stringification check would make sense if it's an object. That way we don't care what the underlying representation of the object is. If we could refactor that check into a function then that might reduce the size of the diff for this change.
I think a stringification check would make sense if it's an object. That way we don't care what the underlying representation of the object is. If we could refactor that check into a function then that might reduce the size of the diff for this change.
Done :) Kindly please check the proposed change I have pushed. I have run the unit tests without any issue on my end.
Thanks @jackdeguest! Merged at the command line via 85c5a5c6a366da3
I have been careful this time to check for variable defined-ness before using
Scalar::Util::reftype
and also sinceScalar::Util::reftype
returnsundef
when false, I made sure to use an empty string by default in equality checks, such as(Scalar::Util::reftype($r) || '') eq 'ARRAY'
I have performed all the test units and all went well.
The advantage of using
Scalar::Util::reftype
over justref
is when one is using array objects. Using justref
would return the object class whereas usingScalar::util::reftype
returns the type as expected, and works for regular arrays as well as array objects.