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

Why config_cascade merge is not recursive #3759

Open
sinavir opened this issue Aug 31, 2022 · 7 comments
Open

Why config_cascade merge is not recursive #3759

sinavir opened this issue Aug 31, 2022 · 7 comments

Comments

@sinavir
Copy link

sinavir commented Aug 31, 2022

$config_cascade is merged with default values here : https://github.com/splitbrain/dokuwiki/blob/6fd2d4b07c014a9eb907759ea660163bf766558e/inc/config_cascade.php#L8

It seems to me that a recursive merge with array_merge_recursive function would be more appropriate in order to avoid redefining large blocks of $config_cascade in preload.php

Is there any reasons against using a deep merge ?

@Klap-in
Copy link
Collaborator

Klap-in commented Sep 1, 2022

So you mean that if I add to preload.php:
$config_cascade['main']['protected']=array('extrafile')
It will overwrite array(DOKU_CONF . 'local.protected.php')?
Does this also happen for if you set the it to another key?
$config_cascade['main']['protected'][1]='extrafile'

I understand it as that array_merge() overrides entries with the same key? That can also be considered as intentional in this design. That current design allows you to override entries. But indeed, extending the array is then more effort.

I do not know what the intention was exactly. The text above the config cascade suggests override is on purpose. Also changing to array_merge_recursive() is probably not directly backwards compatible.
This often is used to change current path to other file structure, e.g. with a farm/animal structure.

@Chris--S
Copy link
Collaborator

Chris--S commented Sep 1, 2022

per Klap-in, that is the intention, to allow people to override particular parts of the config. array_merge does that, array_merge_recursive doesn't.

I do that to keep the server updatable local files (users.auth.php, acl.auth.php) in a separate part of the file system*. I assign just those keys to $config_cascade in preload.php.

If you wish to make larger changes, include config_cascade.php from your preload.php and assign the extra keys you wish.

  • makes it much easier to make archive policies by whole parts of the directory tree (i.e. exclude source controlled areas) and easier to replicate environments.

@sinavir
Copy link
Author

sinavir commented Sep 2, 2022

Ok I understand why recursive is not the right solution : With that you're not able to override paths, just to add some.

The goal I wanted to achieve was to override $config_cascade['main']['local'] without having to specify $config_cascade['main']['protected'] in preload.php (because when preload.php gets executed, we don't have DOKU_CONF so we can't copy-paste the default and must give a hardcoded path)

@Klap-in
Copy link
Collaborator

Klap-in commented Sep 2, 2022

Aha, so the issue for you is that if you set main-local, also main-protected is deleted. Is that because the key ‘main’ is the same, and therefore the entire array set to ‘main’ is overriden?

@Chris--S
Copy link
Collaborator

Chris--S commented Sep 2, 2022

@Klap-in yes, the top level of the array is merged, so ['main'] will be replaced. An argument could be made for a config_merge() which would iterate over the top level keys and perform array_merge on each. Possibly that wouldn't be a breaking change - guessing that people replacing config keys at the top level are using the same key names at the second level.

@Klap-in
Copy link
Collaborator

Klap-in commented Sep 2, 2022

Ok, in that case a pull request is welcome including unit tests.

Eventually, inspiration from existing test: https://github.com/splitbrain/dokuwiki/pull/3349/files

@sinavir
Copy link
Author

sinavir commented Sep 2, 2022

Ok, I can write the PR but I'm not confident in writing the unit test. Any help is welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants