Skip to main content
Topic: No packages after addons installed (Read 6365 times) previous topic - next topic
0 Members and 1 Guest are viewing this topic.

Re: No packages after addons installed

Reply #15

In addition to the above, the final return also seems wrong to me
Code: [Select]
		return
substr(strtolower($filename), -7) == '.tar.gz'
|| substr(strtolower($filename), -4) != '.tgz'
|| substr(strtolower($filename), -4) != '.zip';
I think those != should be ===



Re: No packages after addons installed

Reply #16

On the other hand, I think it should be:
Code: [Select]
		return
substr(strtolower($filename), -7) != '.tar.gz'
|| substr(strtolower($filename), -4) != '.tgz'
|| substr(strtolower($filename), -4) != '.zip';
But I do not want to propose changing it because I haven't tested that part. So far my packages are .zip files and current fixes already work fine. But I do think for tar.gz it should be != and not ==. Again, I don't touch it unless it is clearly broken on my side.

Re: No packages after addons installed

Reply #17

I must say looking at this line in the old code
Code: [Select]
!(is_dir($packagesdir . '/' . $package) && file_exists($packagesdir . '/' . $package . '/package-info.xml'))
Makes my head hurt a bit.  As stated already, that will only "fail" if both conditions are true .. so if it is a directory and package-info.xml exists, then it fails .. ??
Code: [Select]
! (true && true)  => false
! (false && true) => true
! (false && false) => true
I would think (again as already stated) that if it is a directory then the package-info file must exist in it. which means the line should be.  This avoids us placing a non package zip or whatever in the package directory and listing it as a package.
Code: [Select]
if ($current->isDir() && !file_exists($current->getPathname() . '/package-info.xml'))

Doing that along with fixing the != to === should fix the problems, at least it does on my install.

Re: No packages after addons installed

Reply #18

Okay @Spuds, you are concerned with the logic and here is what I think.

For example, to read a package of Masa.sahabat.ahrasis.com.zip, it must not be a directory and there must not exist package-info.xml file[1] at the end of the path. The logic is if it has any one of this, it is definitely not a package. So we return false or do not continue to check them further as a package. Remember we are checking the file in the directory but ignoring the directory and package-info.xml.

But if both are not a directory and not a package-info.xml file at the end of the path, then it could be a package, so EA will continue or return true to check them based on filename extension.

So since we are checking "if (!($current->isDir() && file_exists($current->getPathname() . '/package-info.xml')))" the above logic says this should return true. Using this, I have all of the packages properly listed. Based on the previous code and based on the logic, I'd say it is correct.

I may be wrong though, so this could be turned down at any time. I don't really mind since everybody has its own working solution to solve this problem. I leave the official one to the best hand.
This check for package-info.xml file existence is may be a safeguard for something but I do not know.

Re: No packages after addons installed

Reply #19

Quote from: ahrasis – if (!($current->isDir() && file_exists($current->getPathname() . '/package-info.xml')))
- If this is not a directory and if there exist no file under pathname/package-info.xml <--- This right to me and previous code confirms it is right too.
Quite but not exact.

The check goes like this in php:
  • is $current a directory?
       
    • => yes: does pathname/package-info.xml exists?
             
      • => yes: then (! true) ) => false and do not enter the if, but continue with the code because it may be a valid package
             
      • => no: then "if (! false) => true is a directory, but it doesn't contain a file, he file is skipped (return false)
    • => no: then (! false) => true is a file, skip it (return false)

So, translated in English is more like: if the "entry" is a directory and has a package-info.xml then it looks like a package. Otherwise not.
What is wrong here is the case when an "entry" is not a directory.
So the code should simply be changed to:
Code: [Select]
		// Anything that, once extracted, doesn't contain a package-info.xml.
if ($current->isDir() && !file_exists($current->getPathname() . '/package-info.xml')
{
return false;
}

That said, I feel you will face again the open base dir error, because as far as I can tell, for your file, the "isDir" is evaluated to true (otherwise it would never check it package-info.xml exists generating the open base dir error).
Bugs creator.
Features destroyer.
Template killer.

Re: No packages after addons installed

Reply #20

Quote from: Spuds – I must say looking at this line in the old code
Code: [Select]
!(is_dir($packagesdir . '/' . $package) && file_exists($packagesdir . '/' . $package . '/package-info.xml'))
Makes my head hurt a bit.  As stated already, that will only "fail" if both conditions are true .. so if it is a directory and package-info.xml exists, then it fails .. ??
Code: [Select]
! (true && true)  => false
! (false && true) => true
! (false && false) => true
Yes, the old code was failing here correctly, because in case of success the condition was "continue;" (i.e. skip the file).

Quote from: Spuds – I would think (again as already stated) that if it is a directory then the package-info file must exist in it. which means the line should be.  This avoids us placing a non package zip or whatever in the package directory and listing it as a package.
Code: [Select]
if ($current->isDir() && !file_exists($current->getPathname() . '/package-info.xml'))
Yep, right.
At least with my current headache.

Quote from: Spuds – Doing that along with fixing the != to === should fix the problems, at least it does on my install.
True also that.
The logic was inverted, I guess I forgot to invert part of it. O:-)
Bugs creator.
Features destroyer.
Template killer.

Re: No packages after addons installed

Reply #21

@ahrasis
While your logic is fine I think you would agree that your premise sounds rather odd. If it has a file called package-info.xml it's not a package? :)

@emanuele
No, it will fix the error because the code was checking !isDir(). So what you'd get was a file
Code: [Select]
/blabla/existingfile.txt

concatenated with package-info.xml

Code: [Select]
/blabla/existingfile.txt/package-info.xml

That in turn triggers the PHP bug I linked that says there's a permission issue even though you'd have permission to access the file if the path resolved to anything. For a function called file_exists I'd call it a rather egregious bug, but apparently over at PHP they think it's an edge case. :P

Re: No packages after addons installed

Reply #22

In the old code it states as follows:
Code: [Select]
if ($package == '.' || $package == '..' || $package == 'temp' || (!(is_dir(BOARDDIR . '/packages/' . $package) && file_exists(BOARDDIR . '/packages/' . $package . '/package-info.xml')) && substr(strtolower($package), -7) != '.tar.gz' && substr(strtolower($package), -4) != '.tgz' && substr(strtolower($package), -4) != '.zip'))
continue;
The real check is below it:
Code: [Select]
				$skip = false;
foreach ($context['package_types'] as $type)
if (isset($context['available_' . $type][md5($package)]))
$skip = true;

if ($skip)
continue;

// Skip directories or files that are named the same.
if (is_dir(BOARDDIR . '/packages/' . $package))
{
if (in_array($package, $dirs))
continue;
$dirs[] = $package;
}
elseif (substr(strtolower($package), -7) == '.tar.gz')
{
if (in_array(substr($package, 0, -7), $dirs))
continue;
$dirs[] = substr($package, 0, -7);
}
elseif (substr(strtolower($package), -4) == '.zip' || substr(strtolower($package), -4) == '.tgz')
{
if (in_array(substr($package, 0, -4), $dirs))
continue;
$dirs[] = substr($package, 0, -4);
}

It continue to skip any file / directory with the said criteria and not to take any of them as package. The one that is not skip is continue to be checked and filtered and only the true one is caught as a package and got listed.

$current which is previously $package, is never meant to be a dot, double dots, a directory, an extracted package-info.xml, not .tar.gz, not .tgz and not .zip, so they are all skipped from being filtered further.

Whether I understand it correctly or not, I don't mind anymore. You are all clever boys. :P

Re: No packages after addons installed

Reply #23

That helps, at least me.  So the way the old code was written (after skipping dot and temp files) was/is
Code: [Select]
	!(is_dir(BOARDDIR . '/packages/' . $package) && file_exists(BOARDDIR . '/packages/' . $package . '/package-info.xml'))
&& substr(strtolower($package), -7) != '.tar.gz'
&& substr(strtolower($package), -4) != '.tgz'
&& substr(strtolower($package), -4) != '.zip'))
If a given file could pass this conditional (i.e. does not return true), it was considered a package.

So lets try by example ...

Given a filename of test.zip it would eval as
!(false && false) && true && true && false => true && true && true && false => false ... a potential package file

Given a directory of test that has a package-info.xml the eval would be
!(true && true) && true && true && true => false && true && true && true => false ... a potential package file

Given a directory of test without a package-info.xml  the eval would be
!(true && false) && true && true && true => true && true && true && true => true ... not a potential package file

Just to stress it a bit, the following

Given a directory named test.zip with a package-info.xml the eval would be
!(true && true) && true && true && false => false && true && true && false => false ... a potential package file

Given a directory of test.zip without a package-info.xml  the eval would be
!(true && false) && true && true && false => true && true && true && false => false ... a potential package file



Breaking the old conditional in to two parts breaks the boolean concatenation, mainly we want to allow directorys with a package-info.xml to be considered as potential packages.  But the new function will only return true if the file is a .zip, .tgz or
tar.gz ... To match the "old" code I think it should be

Code: [Select]
		// Anything that, once extracted, doesn't contain a package-info.xml.
if ($current->isDir())
{
return file_exists($current->getPathname() . '/package-info.xml');
}

// And finally, accept anything that has a "package-like" extension.
return
substr(strtolower($filename), -7) === '.tar.gz'
|| substr(strtolower($filename), -4) === '.tgz'
|| substr(strtolower($filename), -4) === '.zip';
Last Edit: February 10, 2017, 04:10:05 pm by Spuds

Re: No packages after addons installed

Reply #24

Quote from: Frenzie – @emanuele
No, it will fix the error because the code was checking !isDir(). So what you'd get was a file
Code: [Select]
/blabla/existingfile.txt

concatenated with package-info.xml

Code: [Select]
/blabla/existingfile.txt/package-info.xml

That in turn triggers the PHP bug I linked that says there's a permission issue even though you'd have permission to access the file if the path resolved to anything. For a function called file_exists I'd call it a rather egregious bug, but apparently over at PHP they think it's an edge case. :P
I guess I lost track of what code we were actually discussing. lol

@Spuds sounds right with all the cases explained like that. O:-)
Bugs creator.
Features destroyer.
Template killer.

Re: No packages after addons installed

Reply #25

I already used the suggestion[1]and so far, no issue with open_basedir and no other error. So this could be the acceptable final solution.

I would also like to agree to @Spuds too with regards to the compressed files extension. This, as said, is not a problem to me, as the current code for it is working so far.

I am however surprised as it's working (in positive == for tar.gz and in negative != for tgz and zip) so far. Which means, if what you said is true, it should not have listed the files in my list of packages since mine are zip files.

So, is the filtering of the file as package with compressed file considered working if it can work both ways where it should not? I mean if they are in negative =! zip, they should return false but in == zip, they should return true, right? Or it can work both ways but we preferred in positive?

Well, again, I don't know and don't understand why it works the other way around too.
if ($current->isDir() && !(file_exists($current->getPathname() . '/package-info.xml')))<--- with added brackets for conditions after !

Re: No packages after addons installed

Reply #26

That would indicate, at least to me, that the function is returning "true" prior to it ever reaching that last conditional.

The entire purpose of the function is to minimize the work for the next step. 

The next step is to unzip, untar etc all of the archive (zip, tar.gz, tgz) files that pass this filter,  and then validate that they are indeed package files and for the version of ElkArte being run. 

If the entire function was simply "return true;" the system would still list the package files as expected, however it would be doing extra work, quite a bit extra depending on how many things are in that directory.

Re: No packages after addons installed

Reply #27

Quote from: Spuds – To match the "old" code I think it should be
So the exact opposite of what it was doing? lol :P

Re: No packages after addons installed

Reply #28

Pretty much  :)  That conditional was a mess to begin with and breaking it up made a mess of the old "logic".

Re: No packages after addons installed

Reply #29

Why don't just filter them out rather than allowing them to go through? That is what the new filter is all about, right?