WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#8331 closed defect (bug) (fixed)

Automatic Plugin Installer always fails when using SSH

Reported by: ydekproductions Owned by:
Milestone: 2.7 Priority: high
Severity: normal Version: 2.7
Component: Administration Keywords:
Focuses: Cc:

Description

Every time I attempt to use the automatic plugin installer, it fails.

I don't know any way that I can really get you a specific error log, but if you can point me in the direction, I'd be glad to look.

I'm running a VPS with PHP 5.2, and I'm attempting to install over SFTP. Yes, my entire wp-content directory is chmodded to 777, so it has the correct permissions.

Downloading plugin package from http://downloads.wordpress.org/plugin/blahblahblah.zip

Unpacking the plugin package

Folder allready exists.: blah

Installation Failed

I'm setting priority to High since this is a nice newly added feature, so it should probably work for everyone. :)

Attachments (3)

strip-only-whitespace.diff (425 bytes) - added by ydekproductions 6 years ago.
This fixes it. It is stripping all whitecharacters in the returned $data stream is that is the only thing contained in the stream. Doesn't appear to break anything.
trim.diff (562 bytes) - added by ydekproductions 6 years ago.
trim-return-bool.diff (852 bytes) - added by ydekproductions 6 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 @ydekproductions6 years ago

Might I add that doing a upgrade on an existing plugin works flawlessly. This error is only when installing a brand new plugin.

comment:2 @DD326 years ago

The installer doesnt install the plugin if a folder exists of that name allready (the folder name being the plugin slug from wordpress.org)

Is that the case for you?

for 2.8(Since its too close to release) It'd probably be best to check that the folder actually contains some files before bailing on that requirement, Or perhaps incrementing the folername with a -2, -3, etc.

comment:3 @ydekproductions6 years ago

I should have stated that I don't have a folder by that name in the directory, haha!

It doesn't just fail on one plugin, it's on any that I've tested.

comment:4 @ydekproductions6 years ago

Ok, I'm digging into this and finding where the bug is. It falls in the exists() function of class-wp-filesystem-ssh2.php

	function exists($file) {
		$this->debug("exists();");
		$list = $this->run_command($this->link, sprintf('ls -lad %s', $file));
		return (bool) $list;
	}

When no directory is found, $list is a newline character only. Not an empty string. So the newline character when cast to a boolean is considered true.

Let me poke some pore time into this and see if I can come up with a patch that doesn't break everything else.

comment:5 @DD326 years ago

  • Summary changed from Automatic Plugin Installer always fails to Automatic Plugin Installer always fails when using SSH

Eugh, Wish people would call 'Sftp' 'ssh2', simply for the fact that 99.99% of it isnt sftp at all.

perhaps the result of run_command() should be trim'd before being returned (for all cases).

I think this may also depend on the servers configuration(&shell), because SSH2 + plugin install works for me, using Debian4 + Bash, which is why standardising a triming of the return values of shell commands could work?

comment:6 @DD326 years ago

Eugh, Wish people would call 'Sftp' 'ssh2', simply for the fact that 99.99% of it isnt sftp at all.

(And it just confuses me when i'm not fully with it!)

@ydekproductions6 years ago

This fixes it. It is stripping all whitecharacters in the returned $data stream is that is the only thing contained in the stream. Doesn't appear to break anything.

comment:7 @ydekproductions6 years ago

I think this may also depend on the servers configuration(&shell), because SSH2 + plugin install works for me, using Debian4 + Bash, which is why standardising a triming of the return values of shell commands could work?

Odd, I'm using Debian 4 + Bash as well. But you're right, just a basic trim() to the data seems to fix it right up.

comment:8 @DD326 years ago

Hm, Actually, Looks like

} elseif (($returnbool) && (! (int) $data )) {

should probably be set so that the $data is trimmed earlier, And that the exists check calls run_command() with $returnbool = true.. following that rather obscure description at all?

comment:9 @ydekproductions6 years ago

No, the exists check is not called using $returnbool = true.

$list = $this->run_command($this->link, sprintf('ls -lad %s', $file));

But regardless, it can probably be trimmed a bit earlier just to be safe. That cast to an (int) makes me think that somewhere it'll cause the same issue. :)

I updated my patch.

@ydekproductions6 years ago

comment:10 @ydekproductions6 years ago

should probably be set so that the $data is trimmed earlier, And that the exists check calls run_command() with $returnbool = true.. following that rather obscure description at all?

Wow, I completely misread that. I'm waaaay too tired. But yes, I just modified it to use the $returnbool = true, and it works fine as well. That seems like the more logical solution in this case since we ARE checking for a boolean value.

comment:11 @DD326 years ago

Wow, I completely misread that.

If i wasnt tired i would've written up a diff to show it instead of that convulated line.. :) But that patch is exactly what i was thinking of.

comment:12 @ryan6 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [9862]) Fix SSH2 fs exists() method. Props ydekproductions. fixes #8331

comment:13 follow-up: @chk6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I just upgraded to Wordpress 2.7, and this change isn't working for me on a relatively stock Ubuntu 8.10 system, for an existing plugin.

The problem is in the exists() and run_command() method. With the change above, when the directory *does* exist, the data string returned is:

drwxr-xr-x 5 chk chk 4096 2008-12-11 10:38 /var/websites/blog/chk/html/wp-content/plugins

With this data, the code in run_command():

if (($returnbool) && ( (int) $data )) {

return true;

} elseif (($returnbool) && (! (int) $data )) {

return false;

} else {

return $data;

}


Always returns false, and WP thinks my plugin directory doesn't exist.

I reverted the change (9892) in this bug report, and now my install can upgrade existing plugins.

comment:14 in reply to: ↑ 13 @westi6 years ago

  • Milestone changed from 2.7 to 2.7.1

Hmm.

I don't see how that code can ever work correctly.

The return from ls -lad /dir/filename is never going to be an (int)!

You are going to get either:
drwxr-xr-x 6 peter peter 4096 2008-12-11 08:50 /dir/filename
Or:
ls: trunky: No such file or directory

Surely

I had kind of assumed that this filesystem used actual sftp not ssh2 and shell commands :-(

comment:15 @DD326 years ago

The return from ls -lad /dir/filename is never going to be an (int)!

The stupid code uses that to tell if its an empty string or not.

The entire class needs a lot of things rewritten to work with some hosts.

comment:16 @Denis-de-Bernardy6 years ago

shall we merge this one into #7875?

comment:17 @DD326 years ago

  • Milestone changed from 2.7.2 to 2.7
  • Resolution set to fixed
  • Status changed from reopened to closed

shall we merge this one into #7875?

Yep, Its going to be better dealt with there, Infact, I think some of the remaining issues here have been corrected with my patch on that ticket.

Reclosing as fixed in 2.7, Remaining issues to be moved to #7875

comment:18 @DD326 years ago

Correction: Any remaining issues can be dealt with on #8210

Note: See TracTickets for help on using tickets.