Skip to content

Conversation

jbottigliero
Copy link
Contributor

The type returned from DataObject.contentLength varies based on how the object
is 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. populate seems to be given a string for the $info->bytes and populateFromResponse is 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.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 88.11% when pulling f080c2f on jbottigliero:hotfix/DataObject-contentLength into 7b54706 on rackspace:working.

@jbottigliero
Copy link
Contributor Author

It's worth noting that this commit only seems necessary due to documentation/code comments designating the DataObject->getContentLength() return value is expected to be an int. If this is actually just a documentation tweak, I can make that happen too 😄

@jamiehannaford
Copy link
Contributor

@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 null|string|int, or even mixed. That would fix the immediate problem IMO.

@jbottigliero
Copy link
Contributor Author

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

@jamiehannaford
Copy link
Contributor

@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 git reset --hard, amend the docblock annotation, and then push - all the changes can be contained in this PR

Updates content length getter and setter docblocks to better reflect
what can be returned due to various population methods.
@jbottigliero jbottigliero force-pushed the hotfix/DataObject-contentLength branch from 8eadbcb to dd794ab Compare May 27, 2015 01:22
@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.1% when pulling dd794ab on jbottigliero:hotfix/DataObject-contentLength into 7b54706 on rackspace:working.

@jbottigliero
Copy link
Contributor Author

PR has been updated to only alter the docblock @param and @return variable type, and, a typo.

jamiehannaford pushed a commit that referenced this pull request May 27, 2015
…ngth

hotfix – DataObject->setContentLength : Ensure int
@jamiehannaford jamiehannaford merged commit e2ce394 into rackspace:working May 27, 2015
@jamiehannaford
Copy link
Contributor

@jbottigliero Thanks! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants