hotfix – DataObject->setContentLength : Ensure int#607
Conversation
|
It's worth noting that this commit only seems necessary due to documentation/code comments designating the |
|
@jbottigliero This is a great catch, thanks for reporting. Although I completely agree with you about this (I think we should cast to an integer), I'm worried about the implications of changing behaviour that will affect somebody's implementation. Since Object Store is one of the most used services, it's within the realm of possibility that somebody is relying on a specific data type for the content length. We could do this in a major version bump, but not in a minor or patch release. For that reason, I think we should change the docblock types in L67 and L260 to |
|
@jamiehannaford – with that in mind, should I open up a new PR with just the documentation changes as suggested and keep this isolated for a future release? |
|
@jbottigliero I'm actively working on a v2, which is residing in a new Github org. It's effectively a full rewrite, so I'm not sure whether the above code change will ever be merged. If you could |
Updates content length getter and setter docblocks to better reflect what can be returned due to various population methods.
8eadbcb to
dd794ab
Compare
|
PR has been updated to only alter the docblock |
…ngth hotfix – DataObject->setContentLength : Ensure int
|
@jbottigliero Thanks! 🚀 |
The type returned from
DataObject.contentLengthvaries based on how the objectis populated. This change ensures that when 'setContentLength' is called
the property is set to the expected type (int).
It seems like this could be solved a number of ways, but this seemed like the "silver bullet". At first glance it seems the problematic code is in the two
populate*methods on the object.populateseems to be given a string for the$info->bytesandpopulateFromResponseis explicitly cast to a string. These could be updated to cast to an integer but I thought I'd let someone more familiar with the library make the call.Just let me know, and I would be happy to amend this pull request.