Skip to content
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

Introduce a configuration manager #570

Merged
merged 13 commits into from
Jul 9, 2016

Conversation

ArthurHoaro
Copy link
Member

@ArthurHoaro ArthurHoaro commented May 18, 2016

Warning: this PR is huge, even if I try to split commits, since it touches the whole codebase.

This introduces a configuration manager which let us read and write settings anywhere properly:

$conf = ConfigManager::getInstance();
$foo = $conf->get('settings.foo', 'default');
$foo = 'bar';
$conf->set('settings.foo', $foo);
$conf->write(isLoggedIn());

It will allow us to get rid of the $GLOBALS configuration system, which is a very bad practice. We'll switch to JSON according to #489.

This PR doesn't contain JSON conf yet, but proxy the PHP conf file behind ConfigManagement, for transition.

Done:

  • JSON settings
  • Use the updater to create an initial JSON settings file from config.php
  • Set the timezone
  • Update plugins with configuration
  • More robust unit tests
  • Rename and group configuration keys.
  • Documentation
  • Remove remaining config initialization from index.php.

closes #471 closes #464

@ArthurHoaro ArthurHoaro added this to the 0.7.1 milestone May 18, 2016
@ArthurHoaro ArthurHoaro self-assigned this May 18, 2016
@virtualtam
Copy link
Member

Cool! Does this PR obsolete #464 and #471 ?

@ArthurHoaro
Copy link
Member Author

Yes it does. We'll keep everything in a single file. I've added plugin update in the todo list.

@virtualtam
Copy link
Member

An other point to check: prevent the JSON configuration file from being accessed from a browser. This will require users to update their server / virtualhost configuration, or to store the JSON data in a serialized PHP script.

@ArthurHoaro
Copy link
Member Author

The access should be denied for the whole data/ directory (that's what the .htaccess file is doing). But we still can wrap the JSON data in a PHP file to prevent badly configured servers to expose their credentials.

@virtualtam
Copy link
Member

htaccess is Apache-specific; we need to handle other setups as well (nginx, lighthttpd) and provide a sane default configuration in the docs (the lighthttpd section is empty btw, I'll see if I can fix this)

@nicolasdanelon
Copy link

And what about store de json between php tags?

@ArthurHoaro
Copy link
Member Author

@virtualtam There is no htaccess equivalent for nginx, but we have a pretty good documentation on it (thanks to you I believe). I never used another webserver.

@nicolasdanelon Yep that's what we meant.

@nicolasdanelon
Copy link

Oh great! Good luck then ^^

@ArthurHoaro ArthurHoaro force-pushed the config-manager branch 5 times, most recently from cca9ba1 to c64005e Compare May 30, 2016 18:24
@ArthurHoaro
Copy link
Member Author

ArthurHoaro commented May 30, 2016

It's starting to look good. Please review and test it, since I might have missed a few things in all of this.

I've renamed configuration keys to be a bit more coherent. The documentation is here. It will be added in the wiki when this PR is merged.

Also note that the automatic transition PHP -> JSON code is a bit messy (especially in unit tests). It might be a good idea to drop it when we'll reach 1.0.0.

@nodiscc
Copy link
Member

nodiscc commented May 31, 2016

Does it migrate current user settings to the new format? Or does the user have to reconfigure/reinstall? Great work!

@ArthurHoaro
Copy link
Member Author

Everything is automagic, and the PHP conf file is backed up.

@virtualtam
Copy link
Member

Neat! I'm starting the review ;-)

$this->setEmpty('path.page_cache', 'pagecache');

$this->setEmpty('security.ban_after', 4);
$this->setEmpty('security.ban_after', 1800);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ban_after is reset twice

@virtualtam
Copy link
Member

rename 0.7.1 to 0.8.0 (and bump the next milestones)

+1, this way we can flag 0.8.x as introducing breaking changes for 3rd-party plugins and theme customizations, and stick to only handling core/vanilla Shaarli settings migration from serialized PHP to JSON :)

"plugins": {
"WALLABAG_VERSION": 1
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing comment closing tag */

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it useless?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, yet generating valid files would do no harm ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this also mean adding it to ConfigJson reading and writing methods, and editing unit tests. I can do it, but for something useless, is it worth it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users are expected to read and edit these configuration files, so it could be worth it. I'm OK with keeping the matter for a future issue/PR, though :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I'll do it in this PR.

@ArthurHoaro
Copy link
Member Author

@kalvn I've added the configuration manager to templates variables.

If you have in config.php

$GLOBALS['config']['foo'] = 'bar';

You can display it in the templates with:

{$conf->get('config.foo')}

When the updater will run, you'll end up with

"config": {
     "foo": "bar"
}

Note that this won't work with settings outside of config or plugins array in config.php.

@virtualtam virtualtam mentioned this pull request Jun 16, 2016
20 tasks
@virtualtam
Copy link
Member

OK for me, ping @nodiscc ;-)

@ArthurHoaro ArthurHoaro added the template HTML rendering label Jul 9, 2016
@ArthurHoaro
Copy link
Member Author

Let's go!

@ArthurHoaro ArthurHoaro merged commit 649af5b into shaarli:master Jul 9, 2016
@kalvn
Copy link

kalvn commented Jul 9, 2016

Nice work!

I noticed a typo in configure.html template where $private_links_default is used instead of what I believe will be $session_protection_disabled on line 39.

And therefore, the variable $session_protection_disabled must be passed to the template in index.php around line 1164.

I keep migrating my theme and will let you know if I find something else :)

@ArthurHoaro
Copy link
Member Author

Thanks for the feedback, I'll fix that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup code cleanup and refactoring enhancement plugin bells and whistles security server template HTML rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin settings aren't write in plugin folder
6 participants