getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 169 forks source link

Cannot redeclare dump() when installing symfony/var-dumper (dependency of Ray) #4462

Closed samzzi closed 2 years ago

samzzi commented 2 years ago

Description

When I install spatie/ray or genxbe/kirby3-ray it immediately throws an error Cannot redeclare dump(). If I add the constant KIRBY_HELPER_DUMP it doesn't seem to help. I'm guessing because the error is being throwed before the define is defined.

Solution is to add if(function_exists(..)) but that was there before so I guess there is a reason why this one was deleted. So I'm not sure if it's ok to do a PR and re-add it 🤷‍♂️

Expected behavior
Don't receive a redeclare error on dump.

To reproduce

  1. Install spatie/ray or genxbe/kirby3-ray
  2. Site will give error now Fatal error: Cannot redeclare dump() (previously declared in /.../vendor/symfony/var-dumper/Resources/functions/dump.php:18) in /.../kirby/config/helpers.php on line 114

Your setup

Kirby Version : 3.7.0.2

Your system

distantnative commented 2 years ago

At which position did you set the constant to false? The former I'd check was removed because we faced big trouble when other libraries would register their functions and Kirby would silently just not register its functions - would lead to really strange errors and the weirdest nightmare for the user to even figure out what causes the problem

afbora commented 2 years ago

You should define KIRBY_HELPER_DUMP from root ìndex.php and before autoload.

https://getkirby.com/docs/reference/templates/helpers#deactivate-a-helper-globally

samzzi commented 2 years ago

@afbora @distantnative indeed, I tried to add it to the genxbe/kirby3-ray plugin. It works when added to the root index.php. I have to be honest though it doesn't feel right. Ray is such a widely used package and everybody who wants to use it now will have to manually add this to the root index.php 🤔

This would mean that for all plugins that use any packages with redeclared functions we have to ask our users to manually change their root index.php. (although I get your argument though...)

I probably know the answer already but it's not like we can make an exception for dump (since it's not kirby critcal) and add a if(function_exists).. there? 😅

If it is what it is I'll update the README of my plugin to let people know they have to manually add that 🤷‍♂️

afbora commented 2 years ago

The following comment from @lukasbestle will explain the situation to you:

Only if composer autoloads the plugin before Kirby. But since plugins have a require rule for Kirby, I think Composer will always load them after. And even if not, it‘s a gamble (probably based on alphabetical order). So I think it would be best to tell people “this plugin overrides helper X. Please put the following in your index.php”. Then it’s also a lot more explicit and people won’t be surprised.

samzzi commented 2 years ago

Fair enough 🤷‍♂️

sschlein commented 9 months ago

I'll bring that up again because Kirby doesn't work with Herd Pro (or it doesn't if you want to use the Dump feature, which is a main argument to use the Pro version) and since there is an official video on how to use Herd with Kirby, that sounds like a reason to discuss the issue again.

https://www.youtube.com/watch?v=Mb_-bo77IPc

lukasbestle commented 9 months ago

I've moved that to a new issue so it won't get lost.