Unhandled exception when using `require_once` in `config.php`

[tillprochaska]

Description

When using require_once to split up complex site configurations into multiple config files (as recommended in the docs), some Panel actions (e.g. changing the page status) will raise an exception (e.g. changing the page status). Please see below for details instructions to reproduce the issue.

I don’t think I’d consider this an issue with Kirby itself. However, it occurs when configuring Kirby using patterns recommended in the official documentation, so I figured creating a bug report would still be appropriate.

Expected behavior Changing the page status shouldn’t raise an unhandled exception.

Screenshots

To reproduce

1. Clone tillprochaska/starterkit.
2. Execute composer start.
3. Open the Panel at http://localhost:8000/panel.
4. Set up a user account and log in.
5. Navigate to "Notes" → "In the jungle of Sumatra".
6. Click on the "Draft" button below the title page to open the page status dialog.
7. In the page status dialog, select published.
8. Click "Change".

Your setup

Kirby Version
3.6.5

Console output
-/-

Your system (please complete the following information) -/-

Additional context
I think I've figured out why this issue occurs:

• require_once returns true if the file has already been included before.
• In most cases, config.php will only be loaded once during a request/reponse cycle. Thus, any files required in config.php will also be included only once.
• However, if the page blueprint uses a custom num value, Kirby will attempt to clone the current Kirby instance when changing the page status to listed (see PageActions::createNum).
• This will in turn load config.php again. This time, however, calling require_once will return true (instead of the actual configuration from the required file).

(This issue might also occur in other situations that cause the Kirby instance to be cloned. I did a quick search for references to App::clone and didn’t find similar code paths, but I might have missed something.)

I guess the simplest (best?) solution to this problem is to always use require instead of require_once when including files in config.php. If you agree, I think the docs should be updated, possibly even using a warning/callout to make sure new users are aware of the difference between using require/require_once. I’d be happy to create a PR! :)

(I’m aware that in the case from the repro repo I could have used num: date instead of num: "{{ page.date.toDate('Ymd') }}" in the blueprint. However, that’s obviously not a real solution to the underlying problem and won’t work in other cases.)

[lukasbestle]

To be honest I feel like it's a bug that App::clone() results in the config being loaded another time. For basic config files without side effects (= that just return an array) the effect is negligible. But if the config file has any side effects (like when it defines a class or alters static properties of Kirby classes), the cloning behavior of the App class would break the site. Now we don't recommend using side effects in config files, but it can happen by accident and cause hard to debug issues.

Unfortunately it looks like changing the cloning behavior is not really viable at this point. The loading order of the App class is quite complex and intertwined because it handles a lot of the internal workings of the core.

So I think the best solution is to document this behavior:

• [ ] In the configuration guide, change the code example to require and add a warning that require_once should never be used.
• [ ] Also explain that the config file may be loaded more than once. So users should be careful with side effects (= anything else than returning an array).

@getkirby/kirby-staff What do you think? Do you see a viable way to make the config file only loaded once, even when the App object is cloned? Do you see another possible fix? Or should we indeed go the documentation route?

[distantnative]

Tbh I'm too afraid to touch the clone method as long as we use the Properties trait. That has so many side effects I think sooner than later we need to get rid of the trait in general, but it is quite a painful process. Especially when trying to avoid breaking changes.

Once we have done this, App::__construct() could allow passing certain arguments, e.g. options, and only call the load methods when none are passed directly. So then the clone method could just pass the already loaded options.

For now, I would definitely add that comment to the guide as @lukasbestle suggests.

[distantnative]

Thinking a bit more about this, maybe I have an idea - let me know what you think, @lukasbestle.

With the App class as is, we could add a bool $loadOptions = true argument to the constructor. And in the try-block that's handling the options we could do

if ($loadOptions === true) {
    $this->optionsFromConfig();
    $this->optionsFromProps($props['options'] ?? []);
    $this->optionsFromEnvironment($props);
} else {
    $this->optionsFromProps($props['options'] ?? []);
}

In the close method, we pass false for loadOptions and make sure to add the full options array of the old instance to $props['options].

[lukasbestle]

Sounds good. :+1:

[distantnative]

Either I am doing something wrong or this isn't a solution after all - getting many errors with https://github.com/getkirby/kirby/commit/a9917e6773312a2f1d472345c872f826c44c228d

[lukasbestle]

The optionsFrom* methods have a few side effects. optionsFromConfig resets Config::$data and optionsFromEnvironment initializes the $app->environment object. Both of these would need to be moved out of these methods.

[distantnative]

Have to admit that I had been trying to solve this quite a bit with https://github.com/getkirby/kirby/compare/develop...fix/4286-config-require-once but just running into too many side effects 🙈