Typo in whichOpenOffice test

[oparoz]

No description provided.

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[scrutinizer-notifier]

The inspection completed: No new issues

[oparoz]

This contribution is MIT licensed, but seriously, it should just be updated upstream

[bantu]

@oparoz Hey there. Just to clearify, what do you mean with upstream in this case?

[oparoz]

I meant, it's a typo and should just be updated when the file is next touched, but I suppose it's easier to not forget by just merging my change.

[bantu]

and should just be updated when the file is next touched

Sorry, that's just not how development works.

I suppose it's easier to not forget by just merging my change

Your pull request is much appreciated, thanks.

[bantu]

@georgehrke Please have a look. The logic doesn't seem to make too much sense to me, btw. If you find libreoffice, why would you keep looking for openoffice?

[georgehrke]

👍 fix makes sense

[georgehrke]

If you find libreoffice, why would you keep looking for openoffice?

It just checks if either libreoffice or openoffice is installed. I don't think it's a big deal that it checks openoffice as well

[DeepDiver1975]

Is that typo present in master and stable6 as well? @oparoz can you double check? THX

[bantu]

It just checks if either libreoffice or openoffice is installed. I don't think it's a big deal that it checks openoffice as well

I don't know whether this code is hot or not. Just wanted to point out that unnecessary work is being done. Not sure how expensive shell_exec calls are, they might be depending on what happens in the background, e.g. actually setting up a new shell.

[oparoz]

@DeepDiver1975, indeed, both stable 6 and master have the same typo.

[th3fallen]

👍 Agree with fix but i think @bantu Comment needs attention because it is a completely unnecessary additional check

[PVince81]

@owncloud-bot ok to test

[ghost]

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/7076/

[PVince81]

👍

[PVince81]

NO! What? Why is that going to stable7 ?

[PVince81]

@karlitschek should we revert or is this backport material ?

[PVince81]

@oparoz please only send pull requests to master, not stable7. They might selectively be backported.

[PVince81]

Pushed to master as bbc2d7c

Waiting for confirmation from @karlitschek whether we need to revert on stable7.

[oparoz]

@PVince81, will do! Sorry about that.

[bantu]

@karlitschek Please see #10823 (comment)

[karlitschek]

@PVince81 yes please

[PVince81]

@karlitschek yes revert or yes keep the backport ?

[karlitschek]

Hehe sorry. :-) Let´s keep the backport.
And forward port if not done yet

[PVince81]

Port to master is already here: bbc2d7c