Site settings cleanup - Githubissues

kojoty commented 8 years ago

@andrixnet:

I want to tackle a serious improvement in the website settings section and I would like to ask for advice first.

Currently there is settingsDefault.inc.php which lists a lot of defaults. There is settings.inc.php which is individual to each node and overrides the defaults above.

What I intend is a cleanup and reorganization in a much better fashion all the existing settings, which most likely involves changing many existing variables (that are currently defined very chaotically) to a unique convention based on the $config array and introducing some new ones.

In order not to adversly affect the currently running production nodes, I believe the best option is the following:

keep the current settings.inc.php as it is, until all work is finished (may take a while)

add a glue file that includes settings.inc.php and is included in the code instead of settings.inc.php. The purpose of this file would be to override the new definitions in settingsDefault.inc.php using the old variable names from settings.inc.php.

change the settings-example.inc.php to the new definitions

make all the changes sequentially based on thematic sections. (easier to track, test and manage)

While the work is in progress, partial implementation updates can be deployed this way and the old variables in settings.inc.php are performing their purpose such that production nodes work properly.

When all changes have been migrated, all node server admins will be instructed to take the new settings-example.inc.php and recreate their settings.inc.php. When all node server admins have confirmed that these changes have been implemented, the glue file can be eliminated.

kojoty commented 8 years ago

I think that cleaning up the mess around the configuration is a good idea and we should do that!

My comments:

It seems to be a bigger work in the core of the website, but we are planned the second huge refactoring around mysql_* ( #276 ) and I think we shouldn't do that in the same time
I think we should use lib/objects/OcConfig class for access interface to the config settings for the rest of the code, and then:
- all settings should be closed inside this class (maybe in separate files, but it should be closed inside),
- access to the config settings should be through the methods of this class

From my point of view it can be done in a steps:

implement the OcConfig class - to be sure that all current settings can be accessed through it
change all the "old-way" style accesses to the config across the whole code (it also can be done in many steps)
clean up the config files, add sections etc. (and ocClass which is the only reader of them)

There are a lot of work, but this seems to be quite easy to do...

andrixnet commented 8 years ago

One point I whish to stress greately: Current implementation with $config in settingsDefault.inc.php seems straightforward and reminds me of MediaWiki style configuration. And it seems very ok to me, simple, easy to undestand (IMHO).

However there is a huge drawback in the way it is currently written. In settingsDefault.inc.php $config is defined as an entire array. In settings.inc.php one would redefine single elements of $config and they way it has to be written is totally different. And this creates confusion. Maybe a lot of confusion.

Since PHP syntax allows it (and this is a good thing), in both files the way it is written should be identical (and portable = copyable) from one file to the other.

From maitenance and administration perspective, having the defaults defined in a file in another directory I believe is more of a headache then help. I am not sure if the class based definitions would be as easy + simple to handle as $config array.

One point is about defining data structures for the OC application and another point is defining configurable settings with default values.

Please take a look at $config['limits'] at the bottom of settingsDefault.inc.php. This is the way I would proceed. Same code can be directly taken with copy/paste to settings.inc.php and values changed to whatever that node's sysadmin desires. Simple as that.

And of course, with all the necessary changes throughout the code to use the new array elements.

wloczynutka commented 8 years ago

Using a lot of global variables definied in settings.inc.php is not very good solution since we use some OOP in opencaching. (variables are not visible inside of classes).

I'm also against adding more and more things configurable per node. This make code more misty. This is bad way to change scripts logic depend on site.

andrixnet commented 8 years ago

Forgive me, but what would be the good way of configuring things on a per node basis? for example: node 1 wants attribute theme X, node 2 wants attribute theme Y. Node 1 wants to limit images to 640x480. Node 2 wants 800x600 (maybe they have more storage) Node 1 wants special banner for april fools. Node 2 does not. etc. etc. etc. There are lots of others. Some of them are currently not possible.

I don't believe a well chosen names for a configuration item increases confusion (mistyness) in the code.

andrixnet commented 8 years ago

I am not sure what problems does $config array creates in the OOP sections of the code, but I do support moving all configuration variables currently indeed declared as individual global variables in settings.inc.php to $config or a similar structure, while taking into consideration the aspects outlined above.

wloczynutka commented 8 years ago

We have common system, so why should we split logic for more combinations? Better is find solution common for all nodes. Configurable things should be used only for simple personalizations like site name, datetime format etc. Spliting engine logic for each node sepratelly is time consuming and leading to incompability. We can add thousands if's fore every thing, but is it really what we want?

andrixnet commented 8 years ago

I am simply asking how can personalizations such as those described above be acomplished?

andrixnet commented 8 years ago

Another example: how can one set the panorama banner on the top of the page on a per node basis? OCRO may whish to show images from Romania. OCNL may whish to show images from the Netherlands...

andrixnet commented 8 years ago

One node may whish to use Google Map as default map layer. Another node may whish to use OSM as default map layer.

examples can go on. It is very useful to have things configurable. it may not necessarily mean lot's of if's, but rather the use of a configuration variable instead of a predefined value.

harrieklomp commented 8 years ago

About the maps. This is remembered by cookie. So user will choose what map he gets on the screen. Except the small map on mainpage.

andrixnet commented 8 years ago

But there is a default map layer before a user may change it.

Also, the small map on mainpage is Google Maps only (not configurable). The small map on cache page is Google Terrain only (not configurable).

harrieklomp commented 8 years ago

I think this is the look of all oc sites. oc.de and oc.us uses the same maptype on mentioned pages. That makes the OC sites as one.

andrixnet commented 8 years ago

@harrieklomp would you like to have the top panorama show images familiar to dutch people?

andrixnet commented 8 years ago

@harrieklomp Also, consider not only the exact visual look, but the principle: on that page, in that location, the map is shown. "the map" is a specific entity. The map is still the map. Having a different default layer per site is no different then the fact that once the user changes the layer, this is remembered in a cookie, and next the map is shown (map as entity) but with the remembered layer ?

andrixnet commented 8 years ago

Another example: OCPL has MAPv3 with Google Map, Google Satelitte, OSMapa, UMP as available layers. OCRO and OCNL have MAPv3 with Google Map, Google Satelitte, OSM as available layers.

For example the UMP layer is polish only, totally useless in countries other then Poland. Having it active on OCPL is a good thing. Having it active on OCNL/OCRO would be useless and counter-productive towards your own users.

There are/can be several alternate layers available as defined in configuration, see settings-example.inc.php, look for $mapsConfig. Including instructions on how to define more.

IMO this is a good thing.

harrieklomp commented 8 years ago

@andrixnet . When you look at those pictures in /images/head you can see different countries. For example i can see pictures who can be made in Belgium, the Netherlands or even in Luxembourg. Now this is a international site, i am happy with it. If you like you can add one or two pictures from Romania.

andrixnet commented 8 years ago

The thing is there are quite a number of personalizations that absolutely require that a node operator is also a developer and that changes be made in the project repository. IMO this is conceptually wrong.

kojoty commented 8 years ago

Colleagues, Generally, I think we all are right - I mean that the dispute is virtual:

I talked about the way of access to the config (by ocConfig class)
@andrixnet talked about the form of the configuration from the server administrator perspective

I think that the $config[] style is a great form of preparing configuration files, but the ocConfig class should be the only one which reads files which contains this $config[] array.

Now I'm sure that we also need modules in configuration to prepare for for more config variables. Let me show you my vision of it - I'll prepare doc with my vision of solving it ok?

andrixnet commented 8 years ago

Sure. i will eagerly wait to see it. I opened this issue for just that purpose, to discuss a better way of improving config, taken from many points of view, and acomplish this as a team. I definitely did not want to start making changes that would perhaps undermine other people's work.

andrixnet commented 8 years ago

Changes to the configuration subsystem will proceed using the following (rough) roadmap:

create glue file to be included after settings.inc.php (purpose: map existing settings to their new counterparts)
redefine a set/group of configuration variables in settingsDefault.inc.php (including documentation)
create their mandatory / optional copy in settings-example.inc.php
create their glue mapping
deploy new configuration variables into the code
test and pull-request
merge this set of configuration variables
repeat steps 2-8 for all configuration variable sets/groups
request all node administrators that they take settings-example.inc.php and rewrite their settings.inc.php
upon completion of 9), remove the glue file
create guidelines to adding new configuration variables / group of variables 12) done

kojoty commented 8 years ago

@andrixnet, First, I'd like to ask you to suspend works on it till the end of removing mysql_* (#276). I'm going to start work at this this week and I suspect it takes 2 maybe 3 weeks (it depend how much time I will have for oc).

Second, My part of vision for this change:

create/reimplement OcConfig class which will have a method to access every config variable from current settings.* files in manner: OcConfig::GetOcNodeId(), which read and return the value $oc_nodeid from current settings file
correct all places in the code which use the "old-style" config: for example all places which uses global var $oc_nodeid should be updated to OcConfig::GetOcNodeId() - (there is a work across many files but I think It's possible) - it needs to be done for all vars from current config...
till then we have old config files (settings.) untouched - at this point the only code which reads the settings. files is closed inside OcConfig class
then we can design the new way of config files - my ideas:
- I think we should create /config dir
- there we should put separate config files for site modules like for example:
  - map.default.php - to store map-related default values
  - map.config.php - to store map-related overrides added by node server admin
- every config file can have a simple and intuitive content in style (for example): $config['key'] = 'value'; or whatever we choose + comment, examples etc.
then we can implement access to the new config files inside OcConfig class. OcConfig uses at this point old-style configs (by default) but there is a "switch" which makes OcConfig works with "new-style" configs only. This "switch" is located in a new-way config files.
then every node admin can select the moment to:
- prepare the "new-way" config files which override "new-way" defaults
- test the new configuration in dev env.
- "switch" his node server to the "new-way" configs
when all our nodes report switch to new-way of config we'll remove all the settings.* files and code related to "old-config" inside OcConfig class

I think I can prepare all the necessary changes in the code for this and drive the whole process, but I would like to finish current works first.

andrixnet commented 8 years ago

Very well, I can postpone until then. However I can edit settingsDefault.php with proposed new configuration variables, all commented.

As discussion on your points:

OcConfig class should be rewritten after the new configuration variables have been defined 1.1. I can redefine the configuration variables but I'm not sure I can properly rewrite OcConfig class. Will investigate. 1.2. I believe a better implementation for OcConfig class would be to dynamically create it's exported options based on the contents of $config[] rather then have everything hardcoded twice (once defined in $config and again defined explicitely on OcConfig). Such an implementation would also allow true modularization, because the module configuration only needs to define it's variables in $config[] and have them in the code via OcConfig class, without the need to change OcConfig as well, when developing and installing a module.
indeed there are many places in the entire code that need to be updated, and this is definitely extensive work. That is one of the reasons why I suggested a gradual migration over time. 2.1. there is stuff currently in settings.inc.php that rather belongs elsewhere, so doing this cleanup also depends on other issues. One example already reported would be https://github.com/opencaching/opencaching-pl/issues/299
the idea of the "glue file" was also to leave the old settings.inc.php untouched until the very end, but have the configuration variables implemented in the code and working already. (this includes having configuration handled OcConfig class) Otherwise it would need to be a two step change: 3.1. implement OcConfig class with existing variables and deploy it 3.2. change the variables and change OcConfig again and then deploy it again
I am not sure a /config directory with many files is beneficial. Administratively would be more work to manage it then a single file. Also, the code is not modular to justify this. Yes, there are features that can be enabled/disabled, but not modules. I believe modules, if they existed, should be external projects that can be plugged into OPENCACHING-PL and add new functionalities to the basic website. 4.0. currently I vote for a single configuration file for the website (with a separate file for defaults). @harrieklomp should also comment on this as another OC node admin 4.1. I support hierarchical definitions like this :

$config['server']['db']['host'] = 'localhost';
$config['server']['db']['port'] = '3306';
$config['server']['db']['database'] = 'ocpl';
$config['server']['db']['username'] = 'db_username';
$config['server']['db']['password'] = 'my_password';

$config['server']['path']['root'] = '/path/to/ocpl';
$config['server']['path']['dynamic'] = '/path/to/oc-dynamic';

// of course there can be config variables on 1 or 2 levels as well.

They are clearly structured from both human and machine point of view. Admins can easily locate and manage config sections. Grouping also allows handling that group individually in the code and also modularization (see 1.2. above)

the point of the glue file was to remove the need for such switches. (both in config and dual implementation). All implementation is to be done on the new variables. When settings.inc.php is changed to new style, the glue file no longer has any influence, while still being in place (thus can be removed)

I will try to prepare as much as possible of config variables (as comments only) in settingsDefault as a place to start.

andrixnet commented 8 years ago

Follow my progress here: https://github.com/andrixnet/opencaching-pl/tree/settings

kojoty commented 8 years ago

OK, DB refactoring is generally over - it's time to come back to configuration...

I created doc to preset my idea of oc settings refactoring - fell free to comment: [https://docs.google.com/document/d/1LdJi2O-RVCT6lE0VEGrDqO1zPJnBUfTzggqjZSpLyXA/edit#]

andrixnet commented 8 years ago

I disagree with point 4) in the implementation plan. It needlessly complicates administration task, in a pure developer oriented way.

Please take a look at: https://github.com/andrixnet/opencaching-pl/blob/settings/lib/settingsDefault.inc.php

Structuring project level defaults here on several levels ensures that the configurations can be dynamically read and interpreted, even without dedicated configuration handling code for each variable.

settings.inc.php (or similar) single configuration file for the website is a good idea from administration point of view. It contains the subset of variables from settingsDefault.inc.php, those which are mandatory to be set locally and any optional variable that needs to be redefined.

Although some sysadmins may have PHP skills, editing configuration as ... php classes is a big no-no.

There are also items in current configuration which do not belong there. See https://github.com/opencaching/opencaching-pl/issues/299 These should gain priority before changing config.

andrixnet commented 8 years ago

Regarding config handling (with advice from a friend with much more PHP experience then mine): $config as a global resource is much more efficient. It is a better idea then individual variable names for each setting, because only one variable (namely $config) needs to be referenced anywhere in the code and have all the settings at your fingertips.

Making it and it's components into a class / classes adds unjustified memory and CPU penalties. But is it really necessary to rewrite all the settings two more times? (once as class properties and once more as class getter methods)

andrixnet commented 8 years ago

https://github.com/andrixnet/opencaching-pl/commit/b7737c46accb5997eda2bd2fbcd2fcb095109955

andrixnet commented 8 years ago

https://github.com/andrixnet/opencaching-pl/commit/e54d910dafdbf3328d352e27c8d33599322eeaea

opencaching / opencaching-pl

Site settings cleanup #290

Currently there is settingsDefault.inc.php which lists a lot of defaults. There is settings.inc.php which is individual to each node and overrides the defaults above.