Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use OCP classes as much as possible in files_external #15363

Merged
merged 1 commit into from
Jul 1, 2015
Merged

Conversation

RobinMcCorkell
Copy link
Member

To progress towards #4774 , I've modified files_external so that it uses OCP as much as is possible with the current public interfaces.

Also makes the code slightly cleaner and easier to work with in unit tests.

cc @DeepDiver1975 @rullzer @PVince81 @icewind1991

@karlitschek
Copy link
Contributor

👍

@rullzer
Copy link
Contributor

rullzer commented Apr 1, 2015

Looks good! 👍
Lets see what Jenkins has to say :)

@RobinMcCorkell
Copy link
Member Author

Should we merge for oC 8.1? @DeepDiver1975

@scrutinizer-notifier
Copy link

A new inspection was created.

@MorrisJobke
Copy link
Contributor

I rebased this to invoke new CI run.

@ghost
Copy link

ghost commented Jul 1, 2015

🚀 Test PASSed.🚀
chuck

MorrisJobke added a commit that referenced this pull request Jul 1, 2015
Use OCP classes as much as possible in files_external
@MorrisJobke MorrisJobke merged commit b476c12 into master Jul 1, 2015
@MorrisJobke MorrisJobke deleted the ext-ocp branch July 1, 2015 09:16
@DeepDiver1975
Copy link
Member

I'm going to revert this - please resubmit with externals storage tests not failing - THX

@RobinMcCorkell
Copy link
Member Author

Eek, I wonder what happened. Investigating.

@DeepDiver1975
Copy link
Member

Eek, I wonder what happened. Investigating.

no worries - shit happens

@RobinMcCorkell
Copy link
Member Author

Right, it looks like (for some reason) the UserManager cannot find the user passed in to the files_external code, so it returns null, triggering the error when getHome() is called on it. The old OC_User::getHome() code checks for null, and returns what it hopes is a valid home path if the user cannot be found. IMHO this is extremely dangerous, as the user might not even exist on the system.

@PVince81 @icewind1991 @DeepDiver1975 What should we do? Emulate the old behaviour and guess? Or bomb out with some sort of UserNotFound exception?

@lock lock bot locked as resolved and limited conversation to collaborators Aug 11, 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.

6 participants