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.

Sign up for GitHub

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

Jump to bottom

Disable UI when web updater is disabled in config.php #331

Merged
merged 3 commits into from
Feb 26, 2021
Merged

Disable UI when web updater is disabled in config.php #331

merged 3 commits into from
Feb 26, 2021

Conversation

PVince81
Copy link
Member

For #281

@PVince81 PVince81 self-assigned this Feb 17, 2021
@PVince81 PVince81 mentioned this pull request Feb 17, 2021
Closed
kesselb
kesselb approved these changes Feb 17, 2021
View reviewed changes
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

index.php Show resolved Hide resolved
@nickvergessen
Copy link
Member

Isn't that code also used in the phar?
You can check php_sapi_name() == 'cli' to see if it's ran via web or CLI

@PVince81
Copy link
Member Author

Isn't that code also used in the phar?
You can check php_sapi_name() == 'cli' to see if it's ran via web or CLI

I've checked box.json and it uses updater.php as entry point which itself is based on NC\Updater\Application.php.

So from my understanding this change is not needed, but I could still do it just to stay on the safe side.

@PVince81
Copy link
Member Author

I've now added a CLI check

@PVince81 PVince81 requested a review from kesselb February 17, 2021 10:11
@kesselb
Copy link
Contributor

kesselb commented Feb 17, 2021

For some reason (#313) index.php and master/lib/Updater.php needs to contain the same code. At least to have a green ci.

kesselb
kesselb reviewed Feb 17, 2021
View reviewed changes
index.php Outdated
@@ -175,6 +177,12 @@ public function __construct($baseDir) {
require_once $configFileName;
$this->configValues = $CONFIG;

if (php_sapi_name() !== 'cli' && $this->configValues['upgrade.disable-web'] ?? false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (php_sapi_name() !== 'cli' && $this->configValues['upgrade.disable-web'] ?? false) {
if (PHP_SAPI !== 'cli' && ($this->configValues['upgrade.disable-web'] ?? false)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure... my PHP expertise level is inferior compared to @kesselb and @nickvergessen, so not sure which to pick.
battle of the experts ? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

It's your choice ;) I would prefer to wrap the second part because it's easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah of course. I meant the sapi thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Some years ago I read an article comparing function call vs reading a constant in PHP. Constant was a bit faster and that's what I do since then: Use the constant if possible. That might be outdated nowadays.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/nextcloud/server/blob/d4d33e3095fb5d0797ec7c3bb8c93c2d1c905ade/lib/base.php#L563

cough

@PVince81
Copy link
Member Author

I've aligned the classes now, let's see...

@PVince81
Copy link
Member Author

I've raised #332 for the CI issues as there seems to be multiple topics

@J0WI J0WI mentioned this pull request Feb 19, 2021
Closed
@PVince81
Disable UI when web updater is disabled in config.php
8929ea1 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Add CLI check in index.php
4a5035b 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Align updater class with index.php
b53b4e3 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 PVince81 force-pushed the bugfix/281/block-updater-when-disabled branch from 550edca to b53b4e3 Compare February 26, 2021 09:50
@PVince81
Copy link
Member Author

rebased for CI

@szaimen
Copy link
Contributor

szaimen commented Feb 26, 2021

CI is green now

@PVince81
Copy link
Member Author

@kesselb can you merge ? thanks

@ChristophWurst ChristophWurst merged commit e490062 into master Feb 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the bugfix/281/block-updater-when-disabled branch February 26, 2021 12:13
@MorrisJobke
Copy link
Member

Thank you @PVince81, @nickvergessen and @kesselb for digging through the updater code 👍

@MorrisJobke
Copy link
Member

/backport to stable19

@MorrisJobke
Copy link
Member

/backport to stable20

@MorrisJobke
Copy link
Member

/backport to stable21

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Mar 23, 2021
Merged
This was referenced Mar 23, 2021
Merged
Merged
@joshtrichards joshtrichards mentioned this pull request Apr 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kesselb kesselb kesselb approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

@PVince81 PVince81

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PVince81 @nickvergessen @kesselb @szaimen @MorrisJobke @ChristophWurst