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

Can't access kirby() from the config file #1724

Closed
lukasbestle opened this issue Apr 26, 2019 · 22 comments

Comments

@lukasbestle
Copy link
Contributor

@lukasbestle lukasbestle commented Apr 26, 2019

Describe the bug

If kirby() (or any object depending on it) is called from within config.php, Kirby runs into an endless loop as it seems to load the config file over and over.

To Reproduce

Use the following config.php:

<?php

kirby();

return [
  // empty
];

Expected behavior

The kirby() object should already be accessible from the config.

Kirby Version

Kirby 3.1.3

Additional context

Forum thread: https://forum.getkirby.com/t/out-of-memory-with-config-variable/13946?u=lukasbestle

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Apr 30, 2019

I have to disagree. The site object relies on the config to be loaded. That's why it cannot be available there.

@lukasbestle

This comment has been minimized.

Copy link
Contributor Author

@lukasbestle lukasbestle commented Apr 30, 2019

That's right, but I think we could solve it like this:

  • When Kirby is initialized for the first time, already set the singleton and initialize the site object with the default options.
  • Then load the config and overwrite the options in the site object with those returned from the config.

That way, the site object would already be available from the config, but with the default options (there's no other way anyway).

@lukasbestle lukasbestle reopened this Apr 30, 2019
@lukasbestle

This comment has been minimized.

Copy link
Contributor Author

@lukasbestle lukasbestle commented Apr 30, 2019

Another argument for this from Slack (https://getkirby.slack.com/archives/C2UTAQAN4/p1556627527330800): Sometimes it can be useful to access $kirby->roots() from the config, e.g. to build a path to an SQLite DB. As the roots don't depend on the config values, that should be possible.

@lukasbestle lukasbestle changed the title Can't access site() from the config file Can't access kirby() from the config file Apr 30, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Apr 30, 2019

Yes, but we need another way to do this. I.e with additional callbacks. We can't make the Kirby instance available in the config.php

@lukasbestle

This comment has been minimized.

Copy link
Contributor Author

@lukasbestle lukasbestle commented Apr 30, 2019

Why not?

@distantnative distantnative added this to the 3.2.2 milestone Jun 2, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.2.2, 3.2.3 Jul 10, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.2.3, 3.2.4 Jul 30, 2019
@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Aug 9, 2019

Do we want this? Can we do this?
Or shall we close it in favor of: #1538?

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Aug 9, 2019

IMHO we should close this in favour of the lazy loaded extension types.

@lukasbestle

This comment has been minimized.

Copy link
Contributor Author

@lukasbestle lukasbestle commented Aug 9, 2019

This is a feature that gets requested a lot in the forum (accidentally because people try it intuitively and it doesn't work).

To be honest, I still don't really understand what's the issue with initializing the Kirby object earlier. Wouldn't my suggestion above work?

Also I don't know exactly what you mean with lazy-loaded extensions. @distantnative Could you please explain that a bit more?

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Aug 10, 2019

Lazy-oases might not be the perfect term, but it’s what Bastian and I have used when talking about it: basically turning more extensions into callbacks (optional), like the routes one. That callback can be called when the app is actually ready for the extension type and can pass the correct app instance then.

To your suggestion: I’m asking myself if it might be more confusing for developers that they get an instance but with different behavior then their usual
setup (since only with the defaults, not their config).

I think for most cases having hooks at all plugins loaded, at config loaded and at app completely loaded might be more suitable. What do you think?

@lukasbestle

This comment has been minimized.

Copy link
Contributor Author

@lukasbestle lukasbestle commented Aug 10, 2019

That makes sense! But what about the use-case of configuring a database path with $kirby->roots()? We'd need to support callbacks for config options as well.

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Aug 10, 2019

I think the most common case for the wish to have $kirby in config and/or plugins is about paths. So yeah, why not allow a callback for options as well that receives the root directory.

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Aug 10, 2019

Or more granular: allow a callback for the database option.

@lukasbestle

This comment has been minimized.

Copy link
Contributor Author

@lukasbestle lukasbestle commented Aug 10, 2019

Yeah, that would be useful. The callback would probably not even need a param as it's possible to use kirby()->roots() after the Kirby object has been initialized.

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Aug 11, 2019

Well, I don't think we can pass an initialized object to a callback that is meant to provide config options... isn't that the crux, that we need the options to initialize Kirby, so we can't use any fully initialized object to set up the options in the first place.

@lukasbestle

This comment has been minimized.

Copy link
Contributor Author

@lukasbestle lukasbestle commented Aug 11, 2019

Wouldn't the callback only need to be called when the database (in this case) is accessed? And that would happen after the Kirby object has been initialized, so it wouldn't be a problem.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 15, 2019

@distantnative @lukasbestle here's an idea. The Kirby instance is ready as soon as the __constructor has reached the end. (before Config::$data = $this->options)

What we could do is to offer an additional ready callback in the config, which can be used to dynamically inject options, once everything else is ready.

// config.php
return [
    'debug' => true,
    'ready' => function ($kirby) {
        return [
            'db' => [
                'database' => $kirby->root('site') . '/db/database.sqlite',
                'type' => 'sqlite'
            ]
        ];
    }
];

Those additional options would then be merged last-minute
The config.php would still be responsible for the initial values that are needed before plugins are loaded for example. But the ready callback can be used with full access to the instance.

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Oct 15, 2019

Sounds good. Just wondering whether ready is the right term if it happens before plugins are loaded.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 15, 2019

No, it happens after plugins are loaded. Pretty much at the second to last line in the constructor.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 15, 2019

Here:

// bake config

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Oct 15, 2019

👍

bastianallgeier added a commit that referenced this issue Oct 15, 2019
bastianallgeier added a commit that referenced this issue Oct 15, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 15, 2019

@lukasbestle

This comment has been minimized.

Copy link
Contributor Author

@lukasbestle lukasbestle commented Oct 15, 2019

Awesome solution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.