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.

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

Fix install.php #954

Merged
merged 4 commits into from May 11, 2015

Conversation

Projects
None yet
3 participants
@mohshami

mohshami commented May 10, 2015

The current install.php creates an empty config.php file, this fixes the logic and creates a proper configuration file

@f0o f0o self-assigned this May 10, 2015

@f0o

This comment has been minimized.

Show comment
Hide comment
@f0o

f0o May 10, 2015

Member

I'm going to recheck the issue+PR shortly and report back. I agree that there might be a typo in the variablename containing the file-handle. This would indeed need fixing.

Member

f0o commented May 10, 2015

I'm going to recheck the issue+PR shortly and report back. I agree that there might be a typo in the variablename containing the file-handle. This would indeed need fixing.

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Thank you very much for the quick response.

Yes I have verified that the file doesn't exist, the one that is written is empty and this is the only way I could get it write a non-empty config.php file

mohshami commented May 10, 2015

Thank you very much for the quick response.

Yes I have verified that the file doesn't exist, the one that is written is empty and this is the only way I could get it write a non-empty config.php file

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Oh for the ?> part, I took that from the config.php file on the VMware image

mohshami commented May 10, 2015

Oh for the ?> part, I took that from the config.php file on the VMware image

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 10, 2015

Member

Hi @mohshami

Thanks for the PR. Whilst @f0o checks it over could you sign the contributors agreement. As an example: #953

It's not too important on this one as I wrote install.php so pretty confident no copyright issues with the changes, however for other PRs it might be needed so easier to just do it now :)

On this PR, if you can push a new commit that removes ?> from the end of config.php that would be good, we've dropped this from all other places now as it can lead to issues if left in.

Member

laf commented May 10, 2015

Hi @mohshami

Thanks for the PR. Whilst @f0o checks it over could you sign the contributors agreement. As an example: #953

It's not too important on this one as I wrote install.php so pretty confident no copyright issues with the changes, however for other PRs it might be needed so easier to just do it now :)

On this PR, if you can push a new commit that removes ?> from the end of config.php that would be good, we've dropped this from all other places now as it can lead to issues if left in.

@f0o

This comment has been minimized.

Show comment
Hide comment
@f0o

f0o May 10, 2015

Member

Ok its that filehandle variable-typo.
I'm at my family's currently so I'm bound to to phone-browser :S

@laf you can take over if you're bored :)

Member

f0o commented May 10, 2015

Ok its that filehandle variable-typo.
I'm at my family's currently so I'm bound to to phone-browser :S

@laf you can take over if you're bored :)

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Thanks guys,

@laf so I just write this "I agree to the conditions of the Contributor Agreement contained in doc/General/Contributing.md." ?

mohshami commented May 10, 2015

Thanks guys,

@laf so I just write this "I agree to the conditions of the Contributor Agreement contained in doc/General/Contributing.md." ?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 10, 2015

Member

@mohshami You put that in the commit message and just add your details to the Contributing.md file as you see others have done :)

@f0o I'll try and get to this also soon.

Member

laf commented May 10, 2015

@mohshami You put that in the commit message and just add your details to the Contributing.md file as you see others have done :)

@f0o I'll try and get to this also soon.

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Reverted the commit and created a new one with the required changes

mohshami commented May 10, 2015

Reverted the commit and created a new one with the required changes

Mohammad H. Al-Shami
Update install.php to prevent creating an empty config.php file
I agree to the conditions of the Contributor Agreement contained in doc/General/Contributing.md.
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 10, 2015

Member

Hi @mohshami, testing this patch for me causes a 500 error due to this line being commented out:

#$install_dir = array_pop($cur_dir);

What's the reason for this to be commented out?

Member

laf commented May 10, 2015

Hi @mohshami, testing this patch for me causes a 500 error due to this line being commented out:

#$install_dir = array_pop($cur_dir);

What's the reason for this to be commented out?

@laf laf assigned laf and unassigned f0o May 10, 2015

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

I'm running the code on FreeBSD 10.1, librenms is installed in /usr/local/www/librenms

$_SERVER['DOCUMENT_ROOT'] = /usr/local/www/librenms/html

After running $install_dir = implode('/',$cur_dir); $install_dir should be /usr/local/www/librenms and not /usr/local/www, which is I why I commented out the second $install_dir = array_pop($cur_dir); line

Should $_SERVER['DOCUMENT_ROOT'] be equal to a sub folder in html? or should install_dir point to /usr/local/www/ in the end?

mohshami commented May 10, 2015

I'm running the code on FreeBSD 10.1, librenms is installed in /usr/local/www/librenms

$_SERVER['DOCUMENT_ROOT'] = /usr/local/www/librenms/html

After running $install_dir = implode('/',$cur_dir); $install_dir should be /usr/local/www/librenms and not /usr/local/www, which is I why I commented out the second $install_dir = array_pop($cur_dir); line

Should $_SERVER['DOCUMENT_ROOT'] be equal to a sub folder in html? or should install_dir point to /usr/local/www/ in the end?

@f0o f0o added Bug 🐞 and removed Needs-Verification labels May 10, 2015

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 10, 2015

Member

The implode creates the array and then pops two elements off the end, the first being blank (/usr/local/www/librenms/ ends with BLANK usr local www librenms html BLANK) the second being html so you end up with /usr/local/www/librenms when it's imploded back.

It's a hack to get the true install location, if you leave it in it should be all ok.

Member

laf commented May 10, 2015

The implode creates the array and then pops two elements off the end, the first being blank (/usr/local/www/librenms/ ends with BLANK usr local www librenms html BLANK) the second being html so you end up with /usr/local/www/librenms when it's imploded back.

It's a hack to get the true install location, if you leave it in it should be all ok.

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

If I leave it the final value will be /usr/local/www, not /usr/local/www/librenms

mohshami commented May 10, 2015

If I leave it the final value will be /usr/local/www, not /usr/local/www/librenms

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Could this be an nginx issue?

mohshami commented May 10, 2015

Could this be an nginx issue?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 10, 2015

Member

Yup quite possibly. You'd need to code to be compatible with nginx and apache, at present apache works so I'd prefer not to merge this change and affect the majority of users :)

Member

laf commented May 10, 2015

Yup quite possibly. You'd need to code to be compatible with nginx and apache, at present apache works so I'd prefer not to merge this change and affect the majority of users :)

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Of course, I'd like to give it a shot to have both versions working.

For your setup, what is the value of $_SERVER['DOCUMENT_ROOT']?

mohshami commented May 10, 2015

Of course, I'd like to give it a shot to have both versions working.

For your setup, what is the value of $_SERVER['DOCUMENT_ROOT']?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 10, 2015

Member

For me it's:

/opt/librenms/html/

Member

laf commented May 10, 2015

For me it's:

/opt/librenms/html/

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Awesome, thanks mate.

The issue was simpler than I expected, for you there is a "/" at the end of $_SERVER['DOCUMENT_ROOT'], but for me there wasn't. I'm not a PHP guy and this didn't register for me. When I changed the following setting in nginx

fastcgi_param DOCUMENT_ROOT $document_root;
to
fastcgi_param DOCUMENT_ROOT $document_root/;

$_SERVER['DOCUMENT_ROOT'] changed and now it ends with a "/" and removing the comment works the same way it does for you, no change needed. I will remove the commit and add the code again

mohshami commented May 10, 2015

Awesome, thanks mate.

The issue was simpler than I expected, for you there is a "/" at the end of $_SERVER['DOCUMENT_ROOT'], but for me there wasn't. I'm not a PHP guy and this didn't register for me. When I changed the following setting in nginx

fastcgi_param DOCUMENT_ROOT $document_root;
to
fastcgi_param DOCUMENT_ROOT $document_root/;

$_SERVER['DOCUMENT_ROOT'] changed and now it ends with a "/" and removing the comment works the same way it does for you, no change needed. I will remove the commit and add the code again

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 10, 2015

Member

No worries :)

No need to remove the commit btw, just add a new one with the changes. Before re-submitting you will need to update your local install as other PRs have been done since this one and the AUTHORS.md file is now out of sync.

Member

laf commented May 10, 2015

No worries :)

No need to remove the commit btw, just add a new one with the changes. Before re-submitting you will need to update your local install as other PRs have been done since this one and the AUTHORS.md file is now out of sync.

Mohammad Al-Shami
Uncommented second
$install_dir = array_pop($cur_dir);

line. Issue was happening because nginx doesn't add a trailing slash to DOCUMENT_ROOT. Simple change to nginx configuration adds that so no need to edit the code here.

Fixed conflict with AUTHORS.md
@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Thanks for the info mate.

Removed comment from install.php and updated AUTHORS.md

mohshami commented May 10, 2015

Thanks for the info mate.

Removed comment from install.php and updated AUTHORS.md

Mohammad Al-Shami
Merged changes with upstream and fixed AUTHORS.md
Merge remote-tracking branch 'upstream/master'
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 10, 2015

Member

Nearly there :)

So now when the file creation fails the user isn't shown any errors or anything. This patch will fix that for now, needs tidying up but we can do that later:

https://gist.github.com/laf/29180c36583e509a04c8

If you apply that and commit the update it should be good to go. One comment on how you've done the PR is that you're working in your master branch which isn't really the best way. We can merge this time around but it would be worth you having a look at http://docs.librenms.org/Developing/Using-Git/ to get a better idea of how to work using git locally before submitting a PR :)

Member

laf commented May 10, 2015

Nearly there :)

So now when the file creation fails the user isn't shown any errors or anything. This patch will fix that for now, needs tidying up but we can do that later:

https://gist.github.com/laf/29180c36583e509a04c8

If you apply that and commit the update it should be good to go. One comment on how you've done the PR is that you're working in your master branch which isn't really the best way. We can merge this time around but it would be worth you having a look at http://docs.librenms.org/Developing/Using-Git/ to get a better idea of how to work using git locally before submitting a PR :)

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Forgive me for forgetting that.

So far I've only used git for personal projects where I haven't done any collaboration with other people, and this is my first PR.

I will read your document. Thanks again my friend :)

mohshami commented May 10, 2015

Forgive me for forgetting that.

So far I've only used git for personal projects where I haven't done any collaboration with other people, and this is my first PR.

I will read your document. Thanks again my friend :)

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Oh BTW,

Code Added

mohshami commented May 10, 2015

Oh BTW,

Code Added

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 10, 2015

Member

No problem at all @mohshami.

I'll retest this tomorrow and then merge all being well :)

Member

laf commented May 10, 2015

No problem at all @mohshami.

I'll retest this tomorrow and then merge all being well :)

@mohshami

This comment has been minimized.

Show comment
Hide comment
@mohshami

mohshami May 10, 2015

Awesome :)

Have a pleasant night

mohshami commented May 10, 2015

Awesome :)

Have a pleasant night

laf added a commit that referenced this pull request May 11, 2015

@laf laf merged commit 78bd514 into librenms:master May 11, 2015

1 check passed

Scrutinizer No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment