Using Scalar::Util::reftype instead of just ref(), but mindful this time about definedness to avoid warnings

[jackdeguest]

I have been careful this time to check for variable defined-ness before using Scalar::Util::reftype and also since Scalar::Util::reftype returns undef 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 just ref is when one is using array objects. Using just ref would return the object class whereas using Scalar::util::reftype returns the type as expected, and works for regular arrays as well as array objects.

[haarg]

I don't really like this change. I don't think it should be poking into the internals of objects.

[jackdeguest]

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.

[oalders]

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?

[oalders]

@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]

@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?

[oalders]

@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]

@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?

[oalders]

That sounds good. Can we do this without adding new dependencies?

[jackdeguest]

That sounds good. Can we do this without adding new dependencies?

Of course

[jackdeguest]

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?

[haarg]

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.

[oalders]

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).

[jackdeguest]

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 ?

[oalders]

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.

[jackdeguest]

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.

[oalders]

As far as I can tell, the tests never get to a scenario where overload::Method( $_[0], '""' ) returns true. It would be helpful to allow for that case as well.

[oalders]

Thanks, @jackdeguest!

[oalders]

Thanks @jackdeguest! Merged at the command line via 85c5a5c