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

feature: Added support for setting php memory_limit in config.php #7704

Merged
merged 1 commit into from Nov 14, 2017

Conversation

Projects
None yet
3 participants
@laf
Member

laf commented Nov 10, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Users can now change this dynamically rather than having to reload the web service / php-fpm.

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Nov 10, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Nov 10, 2017

The inspection completed: No new issues

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 10, 2017

Member

Seems reasonable, I would be much happier if we could just display the error to the user.

Member

murrant commented Nov 10, 2017

Seems reasonable, I would be much happier if we could just display the error to the user.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 10, 2017

Member

Seems reasonable, I would be much happier if we could just display the error to the user.

Registering our own custom error handler has limitations afaik so we don't get to trap all errors that occur.

Member

laf commented Nov 10, 2017

Seems reasonable, I would be much happier if we could just display the error to the user.

Registering our own custom error handler has limitations afaik so we don't get to trap all errors that occur.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 10, 2017

Member

I mean setting display errors to a different level. To many errors would be printed at this time :/

Member

murrant commented Nov 10, 2017

I mean setting display errors to a different level. To many errors would be printed at this time :/

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 10, 2017

Member

Seems like a different discussion / PR. Regardless of showing this to the user, allowing people to change it in config.php makes it easier for users to fix.

Member

laf commented Nov 10, 2017

Seems like a different discussion / PR. Regardless of showing this to the user, allowing people to change it in config.php makes it easier for users to fix.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 13, 2017

Member

I was concerned we might not be able to set that in some situations, but seems ok. http://php.net/manual/en/ini.list.php

Where you've placed this in the init.php it doesn't allow the setting to be set via mysql. Ok with this?

Member

murrant commented Nov 13, 2017

I was concerned we might not be able to set that in some situations, but seems ok. http://php.net/manual/en/ini.list.php

Where you've placed this in the init.php it doesn't allow the setting to be set via mysql. Ok with this?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 13, 2017

Member

Where you've placed this in the init.php it doesn't allow the setting to be set via mysql. Ok with this?

I decided not to bother with the webui ability as most likely you'll want to edit this when you can't access it.

Member

laf commented Nov 13, 2017

Where you've placed this in the init.php it doesn't allow the setting to be set via mysql. Ok with this?

I decided not to bother with the webui ability as most likely you'll want to edit this when you can't access it.

@murrant murrant merged commit 8e61c63 into librenms:master Nov 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@laf laf deleted the laf:feature/php-memory-limit branch Nov 14, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.