Moved all configuration to a YAML system

[pikzen]

OK, that's a kind of big PR, so let me try to synthesize what I
changed.

1. Imported Symfony's YAML Component.
   Yup, this adds quite a bit of code. But it's either that or using parse_yaml_file, which requires to install PECL, which is a hell of a lot less convenient for end users.
   Note: this is the main source of those 6900 lines. The actual patch is roughly 200 lines

2. Globals variables considered harmful
   At least for config stuff. Although $config (the variable in which conf.yaml is loaded) is kind of a global variable, I limited its access in two functions.

   • option() retrieves a config setting. This has the added benefit of abstracting away the fact that the underlying datastructure is an array.
     ex: option('display.links_per_page')
   • option_set() sets a config setting.
     ex: option_set('display.links_per_page', 200);

   You might wonder what happens if you try to retrieve a non-existing config setting. I'm a big fan of fail fast, fail hard.So it throws an Exception. And if you, yourself modified conf.yaml well you fucked it up and you deserve to have it crashing on your face. Why the hell are you doing this anyways. Ideally, everything non critical should be configurable in ?do=configure. Technically, if you just modify settings without deleting them, everything should be fine.
   Note: you can access $config and modify it directly. I just hope you'll feel bad after doing it.

3. Added a 'dev' flag
   This is pretty simple. dev: true means that every single error is shown. Warnings, errors, exceptions, everything. fix yo shit. dev: false is basically production mode, and all errors are silenced (except critical ones of course). Ideally, this should allow a minimal amount of errors by catching as many as possible in development.

4. Updated templates
   Abstraction again. Accessing $GLOBALS and functions into the templates makes me cry. All the templates now get passed a $options variable, as well as a $logged_in variable
   ex: {if="$options.display.show_atom || $logged_in"}

5. Loading from old style options.php
   It should normally import it, save it into the conf.yaml and delete it. I haven't tested this yet though. Also, if the options.php has been moved from the default 'data/' directory, well you won a ticket for vim conf/conf.yaml.

tl;dr: it's a kind of magic.
Tested on all the pages, it werks. The warnings that may be shown are not caused by this patch, so I've been an asshole and let it there

[nodiscc]

I have mixed feelings about this:

• It moves all config settings in one file conf.yaml whis is cleaner
• It allows getting rid of the use of global variables, which is good (http://c2.com/cgi/wiki?GlobalVariablesConsideredHarmful, http://c2.com/cgi/wiki?GlobalVariablesAreBad)
• It adds 6000 lines of a third-party library (Sympfony), that we will have to depend on, update regularly and hope it doesn't break. I find 6000 LOC for a simple configuration parser is a bit much. Though, Symfony is widely used and considered quite high quality.
• The current way of using $GLOBALS to handle config and some other items has proven reliable over the time. It's not-so-clean, but works.
• The alternative is to use PHP's native .ini parser which has strange behaviour (parses everything as strings so we would have to add extra code to do validation)
• Nitpicking about style: there is no description for new functions; there is no description for config options

Deciding whether we merge this becomes a priority, because it affects future developments regarding the plugin system. It also sets a milestone where we start delegating more work to third-party libs. So it's a balance between the current short/hackish style vs. more code which can handle more cases seriously. @ArthurHoaro @Alkarex @sebsauvage @BoboTiG @e2jk @Sbgodin @pikzen your input/thoughts are welcome.

[nodiscc]

I'll do some tests on this PR now. Without input before 1 month (29 Feb. 2015) and if it works as expected, I'll merge this code.

[pikzen]

I totally agree as far as the size goes. It's a MASSIVE piece of code. Although we can shave off 4.000 something lines by throwing away tests and all that stuff. This is a simple Symfony\Yaml pull through Composer, so I haven't sorted stuff nor cleaned it up.

An alternative would be to use php-yaml, which

• Pulling an executable is kind of like adding lines, except not directly included in the repo. I call that cheating :3
• Pulls through PECL, which needs to be installed, is a pain in the ass to run on windows, and more.
• Has a PHP requirement of 5.5+ IIRC (although supporting PHP 5.1 is borderline criminal.)

I could have used .ini files, if PHP's INI parser wasn't such a piece of crap. Everything it parses, it parses as a string. 1 is a string, 0.18941981 is a string, "hey" is a string. Wich would be fine if PHP didn't have such abysmal casting rules for strings.
As an example, ini_parse interprets 'no' as "", but 'yes" as "1" (http://3v4l.org/8ILmg).

I'm a big fan of not reinventing the wheel for anything in production. Libraries are tested and stable, and do what they're asked to.

(As an aside, Shaarli is including jquery-ui which is 16.582 lines unminified and jquery is 10.347 lines)

EDIT: deleted Tests. Symfony\Yaml is now 1957 lines.

[ArthurHoaro]

I'd say this kind of decision was to be expected since Shaarli is a single 2.5k lines file of vanilla PHP, and we don't have any proper way to handle dependencies. As if we want to let it like it is or slowly make it go to the right direction, I stand on the second option. So I'm in favor of merging this.

However, I did not really look at the code nor test it. I also don't know if this is the best way of loading YAML, but @pikzen seems to have put a lot of thoughts on it.

Only thing, I think we should set the dev option at false by default.

[pikzen]

Addendum: If anyone is working on a feature that uses, $GLOBALS, DO NOT let this patch prevent you from continuing it. I will rebase on top of your work if it's merged before, or will provide help if needed. I don't want Shaarli development frozen because of this PR.

[e2jk]

I do like this attitude. Thanks.

I haven't looked at the code, but have one thing to say: the moment we
merge this, I consider we are effectively not working on a temporary fork
anymore, but have plainly taken over the future direction of Shaarli. I'm
not against that per-se, as we would be able to move forward with other
wide-ranging changes (plugins, internationalization). But I do want us to
be aware of the fact that this is a major step away from the original way
in which Shaarli started (one single file) (which by the way in effect was
already abandoned long ago, see
#71 (comment))

+Emilien

2015-01-30 12:46 GMT+01:00 Florian Eula:

Addendum: If anyone is working on a feature that uses, $GLOBALS, DO NOT
let this patch prevent you from continuing it. I will rebase on top of your
work if it's merged before, or will provide help if needed. I don't want
Shaarli development frozen because of this PR.

[nodiscc]

we are effectively not working on a temporary fork anymore

Judging by the current situation this is already the case. @sebsauvage didn't reply to my mail requesting bug tracker access and permission to add a paragraph to his README. There are few reasons to expect a merge-back in the future and we should move forward. The old tracker is full of abandoned reports and pull requests and I don't feel like taking care of it anymore. I just point users to this fork (sebsauvage#219).

this is a major step away from the original way in which Shaarli started (one single file) (which by the way in effect was already abandoned long ago

As long as we keep to these guidelines I'm ok with it: easy install/upgrade/configuration, easy code maintenance, bug-free, secure, clean code, bloat-free core. This PR would help with all of these points.

[nodiscc]

@pikzen we need a clean path to upgrade the symfony libraries. If you can provide a script or howto, I'll add it to the wiki.

[nodiscc]

Please also add the relevant copyright information for Symfony in the COPYING file at the root of the repository.

[nodiscc]

@pikzen would you be ok to move conf.yaml to the data/ directory? This way

1. It allows easy backup/migration of the user settings and data by simply copying the data directory
2. It removes the need for Shaarli to be able to write in it's root directory, which simplifies fixing Require write permissions only for relevant directories #40

Thanks again. Other comments still welcome!

[pikzen]

Sure. I'll be adding the copyright info and moving that right away.

[virtualtam]

@pikzen awesome work ;-)

This raises some design questions, though:

• should we consider migrating Shaarli's code to a robust PHP framework (yup, I'm thinking Symfony here)?
• should we use a PHP package manager to handle libs and dependencies? Composer seems to be the most popular solution out there, and is very convenient to use

[pikzen]

Symfony is a bit of a huge monster, and pulling 40 megabytes of code would steer us a bit far away from Shaarli's goal of staying minimalist and lightweight :D
I'd rather pick out some small parts (like Symfony's YAML parser, RainTPL, or projects like PHPRouter) and build something efficient with it, rather than using a whole framework. Having already worked on projects with Symfony, it's a behemoth to setup, and I don't think we'd gain much from it. (Note that I think the same way about CodeIgniter et. al., I'd much rather do it the way Chyrp does, which is a lightweight architecture built for the needs of the application.)

Symfony\Yaml was actually pulled from Composer, i just didn't include the lockfile and the procedure to update it for some reason.

[nodiscc]

should we use a PHP package manager to handle libs and dependencies

The install and upgrade process (on the user side) should not require command line access (think shared hosting with only FTP access :/) but we could use this on the development side to make sure our components stay up to date. I think @pikzen's Symfony copy was pulled from composer; we'd just need his composer file to maintain it on our side.

[e2jk]

I am not familiar with any PHP framework (I do my own webdev in Python ;) ), but I agree with @nodiscc : the installation must not require command line access, it should just be a question of FTPing files to a remote server and done.

[mro]

-1 from me because I like the KISS of shaarli a lot. Not having any dependencies is unbeatable and was my personal reason to choose shaarli.

I'd prefer a separate php config include file for pure (git) versioning reasons (like #41). And 'globals' in 1 single file are IMHO bearable. What other than a 'global' would a yaml file be anyway? Can't see any benefit from using yaml.

[nodiscc]

Note that Shaarli already "depends" on RainTPL. However I'm still unsure whether we should merge this. The benefit is that it makes the code "cleaner" by not injecting code directly from options.php and getting rid of the use of globals. As I said the current system works.

I'd like to make the best decision regarding future improvements (eg. will the plugin system be cleaner/easier to deal with if we stay with globals/move to yaml?)

[virtualtam]

Thanks @pikzen, it's been a good while since I haven't done serious PHP coding, I really didn't remember Symfony was that big...

How easy would it be then, to pick features/libs from Symfony and add them to Shaarli?

@mro @nodiscc Regarding the config file format, I've seen the following trend on miscellaneous company and personal projects:

• XML for machine-oriented storage:
  • easy to parse (SAX, DOM),
  • quickly becomes way too verbose for a human to read,
  • easy to constrain (DTD, XSLT).
• YAML for human-friendly storage:
  • incredibly easy to read, very minimalist structure,
  • provides common data structures: strings, lists, dictionaries.

Whether Shaarli uses YAML or not should be transparent for the end user; every config option should be available through the Tools menu... so the matter is rather "how much do we benefit from using YAML as Shaarli/plugin developers?".

[nodiscc]

how much do we benefit from using YAML as Shaarli/plugin developers

My point exactly

[e2jk]

No benefit in my eyes

[pikzen]

No benefit for plugin developers as of now, although a simple patch would allow them to insert their plugins' config into the common options handler, and slightly easier for core devs. And it was already mentioned by @sebsauvage on https://github.com/shaarli/Shaarli/blob/master/index.php#L103. It's a step forward towards a proper conf manager.

Also, if Shaarli ever wants to move towards a router managed page, a lot of them use YAML for easier configuration (routes stored in yaml files instead of hardcoded in index.php). Those include PHPRouter, Symfony's, and I believe Sinatra does. Counter examples includes Klein which is pretty damn awesome.

[nicolasdanelon]

the BEST improvement +1

[nodiscc]

Are we merging this or not? I'll be idling on https://gitter.im/shaarli/Shaarli to discuss this.
Any other chat system (IRC,XMPP) is also fine if you're not comfortable using a proprietary, US-based chat system ;)

[nodiscc]

Closing because of increased LOC/complexity and no clear benefits. We'll keep using php global variables for configuration settings. It will raise warnings in code quality test tools but we can ignore them.

The dev configuration flag was interesting though, feel free to reopen another issue for this.