Cannot redeclare dump() (and ohter helpers)

[iskrisis]

When using your own helpers (like spatie/ray) cli fails.

[fabianmichael]

Same for e() when using the Blade plugin

[lukasbestle]

Possible solution as discussed on Discord:

The CLI could set a constant that then prevents Kirby from doing anything when $kirby->render() is called. The CLI could then use the custom global app object from index.php. As well as the constants that are set from index.php.

For that to work, we need a change in the core that listens to that constant. The CLI could then read the currently installed Kirby version from kirby/composer.json (or the vendor folder, see #11). If the installed version supports the constant, it would load index.php, otherwise it would behave like at the moment.

[iskrisis]

Stupid question but can't the CLI turn off all helpers through constants before bootstraping kirby? There are not required in cli mode right? I know same issue is there with some plugins that need to know about kirby custom enviroment. I think @bnomei even has some code that rips the config part from index.php to get roots. Seems like it would be good to have some general solution for this (happens also when scripting kirby like filling content or cron jobs etc).

[bastianallgeier]

Could you check if the version on develop fixes this problem for you? Make sure to run the CLI in the same root as your index.php

[samzzi]

Could you check if the version on develop fixes this problem for you? Make sure to run the CLI in the same root as your index.php

The cannot redeclare problem is indeed solved in the dev build. And as you said I need to run in the kirby cli in /public/ (in my case) if I run it in my root folder he says he can't find the kirby installation.

[bastianallgeier]

@samzzi yep, that's intended. The new solution is always looking for the index.php and loads the entire configuration from there. Would that be a problem for you?

[samzzi]

@samzzi yep, that's intended. The new solution is always looking for the index.php and loads the entire configuration from there. Would that be a problem for you?

All my installations use a public folder setup. (I think a lot of nginx users do). So it's rather an "inconvenience" (you always have to cd into public) than a "problem".

I'm curious though, is there a reason you didn't keep the public folder check as in the current main version? It was already there and it doesn't hurt, no? 😊

[distantnative]

@bastianallgeier maybe it could respect the Kirby.json to find the index.php? Just thinking that people will be in their dir for composer etc and so they wouldn't need to switch dirs back and forth

[bastianallgeier]

There wasn't really a public folder check. So far, the CLI assumed that there's a kirby folder in the current folder. But of course it could be done by assuming the index.php location in a www or public folder instead.

[bastianallgeier]

@distantnative the thing is that we no longer need the json file with this solution at all. I think that's actually quite nice because it's not yet another config file.

[iskrisis]

I would prefer to not have to use config file. If i need to run from root/index.php location thats fine it can be solved by different means. I can make alias or npm script to do so.

Question is if there won't be more configuration options required in future.

[samzzi]

There wasn't really a public folder check. So far, the CLI assumed that there's a kirby folder in the current folder. But of course it could be done by assuming the index.php location in a www or public folder instead.

You did this no?

$root = getcwd() . '/kirby';

// maybe a public folder setup?
if (is_dir($root) === false) {
    $root = dirname(getcwd()) . '/kirby';
}

You could do something simular?

$index = getcwd() . '/index.php';

// maybe a public folder setup?
if (!file_exists($index)) {
   $index = getcwd() . '/public/index.php';
}

if (file_exists($index) === true) {
   ...
}

[bastianallgeier]

Yes, exactly. I think we should also try for www. If additional popular names for public folders appear, we can still add them later.

[samzzi]

Just another approach I was thinking about, no way something working but just a snippet to share my train of thought 😅

$files = glob(__DIR__.'/*/index.php', GLOB_BRACE);

foreach($files as $file)
{
    preg_match_all("#(/www/|/public/)#i", $file, $matches);
    $path = $matches[0][0];
}

echo $path;

[pwaldhauer]

When will the version with support for that will be released?

And, to further prevent those issues, why dont just wrap the helper definits in function_exists conditons?

Like this?

if (Helpers::hasOverride('css') === false && !function_exists('css')) { // @codeCoverageIgnore

        function css($url, $options = null): string|null
        {
                return Html::css($url, $options);
        }
}

I think this would be a better approach, because otherwise as a plugin developer I'd have to tell anyone "please write the following snippet to your index.php" to disable the helper, which could make the plugin installation a bit more cumbersome.

[distantnative]

@pwaldhauer we had that in the past with the result that some libraries would just declare their own versions of these functions, Kirby would not register its own without any notice to the user/developer which then down the line resulted in super weird bugs that you were incredibly hard to track down as suddenly there is code in the mix that is not from Kirby, not from the developer either but some other dependency that nobody knows about interfering.

[pwaldhauer]

@distantnative okay. If only someone had invented namespaces. :D but understandable!

[lukasbestle]

Unfortunately you cannot alias functions like you can alias classes with class_alias(). If this was possible, the core functions would always be in the namespace and the aliases could be overridden with custom functions. Then the caller of the function could decide which one they want to call.

But with function aliases not being possible, we would have exactly the same issue of non-overridable functions even if we used namespaces.

[bastianallgeier]

✅