Skip to content

Ensure that FileInfo return values as required by its phpdoc.#27389

Merged
mrow4a merged 1 commit into
masterfrom
fix_fileinfo
Mar 15, 2017
Merged

Ensure that FileInfo return values as required by its phpdoc.#27389
mrow4a merged 1 commit into
masterfrom
fix_fileinfo

Conversation

@mrow4a

@mrow4a mrow4a commented Mar 15, 2017

Copy link
Copy Markdown
Contributor

This bug fix prevents the following scenario after calling ->getId() on FileInfo :

selection_082

Please merge it as soon as possible, I need it for other PRs

@mrow4a mrow4a added this to the 10.0 milestone Mar 15, 2017
@mention-bot

Copy link
Copy Markdown

@mrow4a, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975 and @PVince81 to be potential reviewers.

@mrow4a mrow4a force-pushed the fix_fileinfo branch 2 times, most recently from d17393e to 959edc4 Compare March 15, 2017 00:21
@jvillafanez

Copy link
Copy Markdown
Member

👍

@mrow4a mrow4a merged commit fc8e611 into master Mar 15, 2017
@PVince81 PVince81 deleted the fix_fileinfo branch March 15, 2017 08:53
*/
public function getId() {
return $this->data['fileid'];
return isset($this->data['fileid']) ? intval($this->data['fileid']) : null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm didn't we say we also allow string file ids ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interface says clearly, INT.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@labkode is that still a requirement ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PVince81 We are currently using int for file ids but we would like to use strings in the future.
The sync client already supports string file ids, if I remember correctly.

@PVince81

PVince81 commented Aug 7, 2017

Copy link
Copy Markdown
Contributor

Regression on 32-bit systems: #28275 (comment)

@mrow4a

mrow4a commented Aug 7, 2017

Copy link
Copy Markdown
Contributor Author

Hmm, if unit tests will pass after changing interface to STRING from INT, I am fine. And that phpdoc would need a proper explanation I guess.

@lock

lock Bot commented Aug 2, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants