Skip to content
This repository has been archived by the owner on Mar 15, 2020. It is now read-only.

Allow checking for updates with an underprivileged user. Resolve Issue #20 #21

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jxn
Copy link

@jxn jxn commented Mar 8, 2017

This PR should delay checking for temp and phar file writability until they actually need to be checked, allowing use of the update check command with a user who cannot write to the phar file. This should close Issue #20

@@ -327,6 +327,13 @@ protected function newVersionAvailable()

protected function backupPhar()
{
$tempDirectory = $this->getTempDirectory();
Copy link
Contributor

@aik099 aik099 Mar 8, 2017

Choose a reason for hiding this comment

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

This block is copy-pasted several times. Maybe it can be moved out to separate method?

I see that this is moved out from setTempDirectory method. Instead you can:

  1. move that check into getTempDirectory method
  2. use getTempDirectory method internally instead of accessing tempDirectory property directly

Copy link
Author

Choose a reason for hiding this comment

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

Concerning #1, I don't think that will work, because getTempDirectory() method is called in the update check process. That would cause the update check to fail in the same way that required the change (Issue #20 ).

Concerning #2, here in line 330 I am using the getTempDirectory() method as requested and assigning it to a local variable in the method's scope. I don't see a place where I am using the tempDirectory property directly, but please clarify if you see a line where that is done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Concerning #1, I don't think that will work, because getTempDirectory() method is called in the update check process. That would cause the update check to fail in the same way that required the change (Issue #20 ).

I've tracked back every call of \Humbug\SelfUpdate\Updater::getTempDirectory and each of them is prefixed by exception throwing when path isn't writable in one way or another. So moving exception throwing into that method would reduce duplication and preserve logic introduced in this PR.

Concerning #2, here in line 330 I am using the getTempDirectory() method as requested and assigning it to a local variable in the method's scope. I don't see a place where I am using the tempDirectory property directly, but please clarify if you see a line where that is done.

I've double checked again and the place is gone. Probably something in the code was changed or I've written my comment theoretically (without looking at the code).

@@ -371,19 +385,41 @@ protected function downloadPhar()

protected function replacePhar()
{
rename($this->getTempPharFile(), $this->getLocalPharFile());
$localPharFile = $this->getLocalPharFile();
if (!is_writable($localPharFile)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code block was moved out from setLocalPharFile and you can move it to getLocalPharFile method to avoid duplication.

Copy link
Author

@jxn jxn May 2, 2017

Choose a reason for hiding this comment

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

getLocalPharFile() is also called by setTempDirectory(), which would cause this to fail unnecessarily during a check also. I understand wanting to reduce code duplication, but I think it also makes sense to have these exceptions associated with the code that requires writability, because it prevents these methods from being run without the possibility of running the check first. If you'd really like to avoid duplicating the exceptions, I would propose an interstitial method, like protected isDirectoryWritable($this->getTempDirectory); which just does the writability check and throws the exception. I decided not to do that originally because it actually creates more lines of code, but perhaps you'd prefer that to the current PR?

Copy link
Collaborator

@padraic padraic left a comment

Choose a reason for hiding this comment

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

Per review by @aik099 prior to merge (or add an issue to review after).

@padraic padraic added this to the 1.1.0 milestone May 14, 2017
@hopeseekr
Copy link

Well, I guess phpoole is basically useless because of this. No way am I going to run it with sudo privs just because this PR isn't merged.

@theofidry
Copy link
Member

I don't really understand why phpoole would be useless because of this PR. Just download a newer version available directly or use PHIVE.

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