Skip to content

Conversation

@butonic
Copy link
Member

@butonic butonic commented Aug 10, 2016

otherwise we get a "sequence does not exist error" on oracle. just check all other occurences of lastInsertId.
without this files_external is broken on oracle

@mention-bot
Copy link

@butonic, thanks for your PR! By analyzing the annotation information on this pull request, we identified @icewind1991, @nickvergessen and @Xenopathic to be potential reviewers

@butonic butonic added this to the 9.2 milestone Aug 10, 2016
@butonic
Copy link
Member Author

butonic commented Aug 10, 2016

cc @PVince81 because he wanted me to give him oracle

@DeepDiver1975
Copy link
Member

Just another case where unit testing would have helped.

@butonic butonic force-pushed the missingprefixforlastinsertid branch from a231020 to 6b3c653 Compare August 11, 2016 14:46
@PVince81
Copy link
Contributor

@butonic any chance to add a quick unit test for this ? Even if it just calls the method with a single test case, just to give it a pass.

@PVince81
Copy link
Contributor

👍 otherwise

@DeepDiver1975
Copy link
Member

@butonic backport?

@butonic
Copy link
Member Author

butonic commented Aug 12, 2016

@PVince81 there are tests that cover this method: https://github.com/owncloud/core/blob/master/apps/files_external/tests/Service/DBConfigServiceTest.php#L66

But I assume that our files external tests only run on sqlite or mysql ... @DeepDiver1975 ?

I'll create backports.

butonic added a commit that referenced this pull request Aug 12, 2016
butonic added a commit that referenced this pull request Aug 12, 2016
@DeepDiver1975
Copy link
Member

But I assume that our files external tests only run on sqlite or mysql ... @DeepDiver1975 ?

yes indeed - bloody hell .... let's ignore for now - @PVince81 will move this code to core anyhow within 9.2

@DeepDiver1975 DeepDiver1975 merged commit 460a772 into master Aug 12, 2016
@DeepDiver1975 DeepDiver1975 deleted the missingprefixforlastinsertid branch August 12, 2016 13:08
DeepDiver1975 pushed a commit that referenced this pull request Aug 12, 2016
DeepDiver1975 pushed a commit that referenced this pull request Aug 12, 2016
LukasReschke pushed a commit to nextcloud/server that referenced this pull request Aug 29, 2016
LukasReschke pushed a commit to nextcloud/server that referenced this pull request Aug 29, 2016
@lock
Copy link

lock bot commented Aug 5, 2019

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 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants