Clean install of 1.1 RC1. Package Manager is empty (no packages yet) and Installed Packages shows nothing on its list after successful install of addons. Is it a known bug that I missed or forgot?
I only got this in error log but is unlikely related to package manage but more to the install. It happened once only.
I ran across the same issue, but I thought it might be because I was doing some partially failed install/uninstall actions during add-on development.
Edit: hm, I've got 1 at install state one and 3 at install state 2 but none are showing.
Running the query myself I indeed get 4 results.
SELECT id_install, package_id, filename, name, version FROM elkarte11_log_packages WHERE install_state != 0 ORDER BY time_installed DESC
Yet shouldn't those show up in the package manager?
Edit 2: also, even if I did cause it with a partially broken install or uninstall it should probably be more robust against that.
Edit 3: anyway, none of those have a time_installed. Weird.
Okay, so everything seems fine. I just installed an add-on. In the DB it says time_installed 1486640301
In installed.list it says 1486640301
But it's not showing. Everything's still fine in loadInstalledPackages, which returns
Array
(
[0] => Array
(
[id] => 21
[name] => Stop Spammer
[filename] => elk-stop-spammer.zip
[package_id] => Frenzie:StopSpammer
[version] => 3.0
)
)
I'll see if I can track down where it goes wrong.
Okay, I found it. The problem is this commit (by you :P): https://github.com/elkarte/Elkarte/commit/efdb6e7f32730ee4463e60ed32e620ee08ff7dbb
Edit: The problem there is that you changed the logic, which was good. open_basedir probably means you have a permission issue on your own server. Maybe there's a way to prevent that, but this commit should be reverted. :)
Alternatively it could mean that $current is giving out the wrong directory, which perhaps could happen if the DB points to the wrong location.
Edit 2: PR here: https://github.com/elkarte/Elkarte/pull/2865
I'll be glad to help work out where the open_basedir error is coming from. Like I said, my first guess is permissions.
Reverting won't open package manager at all and will only shows this error:
This has been discussed before and at that point of time (1.1 Beta 3), it already fixed the error and package manager is showing properly. Other setup of 1.0.9 is working fine with no issue of open_basedir. This happened only to EA 1.1. Opening it is not safe at all. There is definitely "something" in the code that causes this though I do not know what it is.
Looking a tad better at the error:
/var/www/clients/client2/web15/web/packages/Masa.sahabat.ahrasis.com.v.104.zip/package-info.xml
so you have a directory named "Masa.sahabat.ahrasis.com.v.104.zip"?
And just to be sure, it's really a directory or a symlink? (I ask because I frequently do this kind of... mistakes O:-)
It this a mockery or something because you should have known that better.
That is clearly an addon package name installed which is in the /web/packages folder as /web is the root folder where elkarte 1.1 RC 1 is installed, and definitely is not a symlink or a directory. That is also clearly within the permitted open_basedir of /var/www/clients/client2/web15/web.
EA 1.1 doesn't read the addons in the packages directory correctly but EA 1.0 in the same server does. So the problem lies somewhere in the enhanced code for packages in 1.1 RC 1.
I have resolved this by reverting back to Packages.controller.php and Packages.subs.php from 1.1 Beta 3 of which the fixed has been forwarded before.
All I can say is something are definitely wrong when certain code are ported to the new PackagesFilterIterator.class.php. That from what I believe is from this part of the 1.1 Beta 3 Packages.controller.php:
if ($package->getFilename() == 'temp'
|| (!($package->isDir() && file_exists($package->getPathname() . '/package-info.xml'))
&& substr(strtolower($package->getFilename()), -7) !== '.tar.gz'
&& strtolower($package->getExtension()) !== 'tgz'
&& strtolower($package->getExtension()) !== 'zip'))
continue;
Something went wrong when that code are converted to this in 1.1 RC 1 PackagesFilterIterator.class.php:
public function accept()
{
$current = $this->current();
$filename = $current->getFilename();
// Skip hidden files and directories.
if ($filename[0] === '.')
{
return false;
}
// The temp directory that may or may not be present.
if ($filename === 'temp')
{
return false;
}
// Anything that, once extracted, doesn't contain a package-info.xml.
if (!($current->isDir()) && file_exists($current->getPathname() . '/package-info.xml'))
{
return false;
}
// 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';
I cannot read a code written in advanced ways quite well so I couldn't find fault with it. Those who able to read it, can most probably see its fault.
Not really.
When I test, I do not use packages, I just have a directory with the name of the zip (frequently including the extension) with the files inside, so that I can just edit the files and test "live" the changes without having to re-package everything every time.
And frequently they are just symlink to the repo.
It's not actually
that clear.
Unzipped files are not stored in "packages" directly, but in "temp".
In the code without your change, this path passes the test for "isDir"
$current->isDir()
so it must be a directory and not a zip file, but you say it is a file.
Do you see where my doubt comes from?
Is the account you passed me a while ago configured the same way?
Have you tried some debugging to see what happens exactly? Stick something like this where it goes wrong and we'll know more.
if (!($current->isDir()))
{
// what's the pathname?
print_r('getPathname ' . $current->getPathname() . "\n");
// is just the path any different?
print_r('getPath ' . $current->getPath() . "\n");
// what's the type?
print_r('getType ' . $current->getType() . "\n");
return false;
}
PS Normally I'd throw error_log around that and watch the error logs with tail -f, but that only works on a server properly under your control like a VPS or your own computer. Plus you'd need to know what I'm talking about. This way you only need to view source. :)
Anyway, if you do know what I'm talking about use
error_log(print_r('getPathname ' . $current->getPathname(), true));Edit:
@emanuele If I understand correctly you're saying the code should actually be this?
if ($current->isDir() && !file_exists($current->getPathname() . '/package-info.xml'))
Actually in light of the comment that would make more sense…
No. I don't debug. I read the code and I compare the code. If the differences are small, it is normally easy to understand. If they are too much differences, I'll try to understand its reason why. If I got lucky, I understand them. If not I don't.
I know the code are different. One of them is tgz and zip was previously get by getExtension() but now getFilename(). Also last time they continue, now it return false. And last time they use && (and) now they use || (or).
I no, I don't understand why and I know I am not that learned in programming.
So, rather than blaming my server, what is your output? Is your solution by reverting back is the best you can do being learned in this?
Like I said, it sounds like the code might need to be this instead. It seems like PHP can be blamed for throwing open_basedir errors on files or directories that don't exist but are actually within allowed folders. I didn't know it displayed such strange behavior.
if ($current->isDir() && !file_exists($current->getPathname() . '/package-info.xml'))
Incidentally,
@emanuele would it make sense to rewrite the whole thing with
isDot() (http://php.net/manual/en/directoryiterator.isdot.php) and getExtension() (http://php.net/manual/en/directoryiterator.getextension.php)?
Ok
@emanuele, let's try this shall we.
if (!($current->isDir()) && file_exists($current->getPathname() . '/package-info.xml'))
- If this is not a directory and if there exist a file under pathname/package-info.xml <--- This is wrong to me and previous code confirms it wrong too.
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.
Temp directory is only check while installing not after. I do say the installation went fine. What is needed to be checked is whether packages directory has addon package(s) or not where if it does, list them, if not don't. This is why I asked for support.
So this goes back to getFilename() for tar.gz and getExtension() for tgz and zip, previously. But in RC 1, getFilename() is used for all. This might be caused of the error. But I do not know because I am tired of testing it while people easily jump into conclusion based on their bad assumption.
I have already said previous 1.0.9 install works fine and this include 1.1 Beta 3. I also confirmed that using previous files from Beta 3 in RC 1 works with no errors. So this is definitely a bug to me. So do check if you want and ignore if you want.
I hardly think it's fair to say that stating a hypothesis ("probably", "first guess") is jumping to conclusions, nor did I say the open_basedir issue is not a bug. I said that by almost always evaluating to true this change is what caused no packages to ever show.
This almost always evaluates to true, hence why no packages will show. Remove the ! and they will, plus it will solve your open_basedir issue which was caused by
this bug in PHP (https://bugs.php.net/bug.php?id=52065).
I prefer to read and follow the old code which already are proven than a hypothesis that leads no where and I already find its solution which is supposed to return true and not return false for both lines.
// The temp directory that may or may not be present.
if ($filename == 'temp')
{
return true;
}
// Anything that, once extracted, doesn't contain a package-info.xml.
if (!($current->isDir() && file_exists($current->getPathname() . '/package-info.xml')))
{
return true;
}
This based on the previous code says to continue if the same conditions apply, and that is the solution. If this is accepted, I'll PR to the github.
In addition to the above, the final return also seems wrong to me
return
substr(strtolower($filename), -7) == '.tar.gz'
|| substr(strtolower($filename), -4) != '.tgz'
|| substr(strtolower($filename), -4) != '.zip';
I think those != should be ===
On the other hand, I think it should be:
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.
I must say looking at this line in the old code
!(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 .. ??
! (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.
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.
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 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.
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:
// 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).
Yes, the old code was failing here correctly, because in case of success the condition was "continue;" (i.e. skip the file).
Yep, right.
At least with my current headache.
True also that.
The logic was inverted, I guess I forgot to invert part of it. O:-)
@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
/blabla/existingfile.txt
concatenated with package-info.xml
/blabla/existingfile.txt/package-info.xml
That in turn triggers
the PHP bug (https://bugs.php.net/bug.php?id=52065) 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
In the old code it states as follows:
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:
$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
That helps, at least me. So the way the old code was written (after skipping dot and temp files) was/is
!(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 fileGiven 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 fileGiven 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 fileJust 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 fileGiven 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
// 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';
I guess I lost track of what code we were actually discussing. lol
@Spuds sounds right with all the cases explained like that. O:-)
I already used the suggestionand 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.
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.
So the exact opposite of what it was doing? lol :P
Pretty much :) That conditional was a mess to begin with and breaking it up made a mess of the old "logic".
Why don't just filter them out rather than allowing them to go through? That is what the new filter is all about, right?
Correct, and I think thats what the updates should. The new code / function needs to return true if the file is to be included, the old code it had to return false to continue processing.
Also on this topic, is there any reason to not skip over the packages/backup directory as well?
@Spuds I forgot the status of this, did you commit the fixes you suggested? Can the other two PRs be closed?
ETA: Stupid question, the commit is https://github.com/elkarte/Elkarte/pull/2870/commits/080097968fed339c4415c13e17ffde0b8f093bc9