Skip to content

Give install warning on all OS other than Linux#28761

Closed
phil-davis wants to merge 2 commits into
masterfrom
issue-28760-bsd-warning
Closed

Give install warning on all OS other than Linux#28761
phil-davis wants to merge 2 commits into
masterfrom
issue-28760-bsd-warning

Conversation

@phil-davis

@phil-davis phil-davis commented Aug 22, 2017

Copy link
Copy Markdown
Contributor

Description

Make the install warning for Mac OS X more generic, so the warning is given for any OS that is not Linux.

Related Issue

#28760

Motivation and Context

People can install on officially-unsupported *BSD at the moment, without being given a warning.

How Has This Been Tested?

Remove the "!" in front of \OC_Util::runningOnLinux() and set installed to false in a dev system.
(to trick it into thinking it is an unsupported system)
Go to the web UI. Confirm that the setup page gives a warning with reasonable text.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

This was noticed when looking at code that used runningOnMac(), PHP_OS and/or php_uname to do stuff based on the operating system.

@phil-davis phil-davis self-assigned this Aug 22, 2017
@phil-davis phil-davis added this to the development milestone Aug 22, 2017
@phil-davis

Copy link
Copy Markdown
Contributor Author

Note: this is on top of #28759 so that it gets function runningOnLinux()
If that PR does not happen, then this one will need refactoring.

@DeepDiver1975

Copy link
Copy Markdown
Member

Agreed 👍

@DeepDiver1975

Copy link
Copy Markdown
Member

Backport Please

@mmattel

mmattel commented Aug 22, 2017

Copy link
Copy Markdown
Contributor

Q: why not a generic runningOn function and return a defined value which than can be queried against?

Comment thread lib/private/Setup.php
'%s is not supported and %s will not work properly on this platform. ' .
'Use it at your own risk! ',
$this->defaults->getName()
array($os, $this->defaults->getName())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[ ] please not array()

@phil-davis phil-davis Aug 22, 2017

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.

I took the example from https://doc.owncloud.org/server/10.0/developer_manual/core/translation.html

$l->t('%s is available. Get <a href="%s">more information</a>', array($data['versionstring'], $data['web']));

I guess that example is not the current way it is liked to be done.

* Checks whether the server is running on Linux
*
* @return bool true if running on Linux, false otherwise
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

generally speaking we should not put more code into legacy classes .... but I have no better location where to but this ...

@phil-davis phil-davis force-pushed the issue-28760-bsd-warning branch from c03b7dd to 3ec59bb Compare August 22, 2017 13:36
@phil-davis

Copy link
Copy Markdown
Contributor Author

@mmattel yes, I could make

runningOn($os_type)

and have it take "linux", "mac" or "bsd" and return the boolean answer if on that type of OS.

Does it make much difference to having the 3 functions?

@mmattel

mmattel commented Aug 22, 2017

Copy link
Copy Markdown
Contributor

@phil-davis
I always think that things can be used for other purposes maybe later on, and I am esthetic, it just looks better - from my pov 😄
(or return the text result and query against that like runningOn() === "linux" )

@phil-davis

Copy link
Copy Markdown
Contributor Author

Note: the "runningOn" functions are actually added in #28759 - which this sits on top of. (When I started looking at a BSD detection thing for filemtime then I also noticed the setup page thing, so had 2 reasons for similar underlying code) So I will refactor there...

@phil-davis

Copy link
Copy Markdown
Contributor Author

After refactoring, the code all "just fitted" in #28759
So closing this here.

@phil-davis phil-davis closed this Aug 22, 2017
@phil-davis phil-davis deleted the issue-28760-bsd-warning branch August 22, 2017 15:03
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants