blackfireio / php-sdk

The Blackfire PHP SDK
https://blackfire.io
MIT License
150 stars 22 forks source link

Call to undefined method BlackfireProbe::addMarker() #23

Closed staabm closed 7 years ago

staabm commented 7 years ago

I just integrated blackfire-sdk v1.7.1 in our framework and run into this error on some servers which run with blackfire v1.11.1

I guess the condition in autostart.php does not fit for the cases in which blackfire extension is loaded in a old version which does not yet contain the BlackfireProbe class.

I expected the blackfire sdk to work like a polyfill so I can use BlackfireProbe::addMarker without a class_exists() check on the caller side... is my assumption correct?

romainneutron commented 7 years ago

Hello,

The BlackfireProbe php sdk should not be used in production. It's a polyfill, but you can not use it to polyfill if you're loading the extension.

The only way to prevent this is feature detection using method_exists or version check.

staabm commented 7 years ago

Since blackfire is explicitly marketed as beeing usefull in production you should also add to the blackfire docs that the SDK instead is not meant for use in production systems.

thx for the response.

staabm commented 7 years ago

nevertheless the sdk shouldn't error with blackfire 1.11, should it?

see https://github.com/blackfireio/php-sdk/blob/master/src/autostart.php#L12

nicolas-grekas commented 7 years ago

About @romainneutron's comment about prod vs non-prod, that's a confusion: it is prod-OK to not have the extension and still use the BlackfireProbe class. It will just turn things into no-ops.

I think a proper solution may be to add a conflict with ext-blackfire < 0.15.0 in your composer.json file. I see no other way to make that work otherwise, except add this method_exists check on your side.

staabm commented 7 years ago

Couldnt the check at https://github.com/blackfireio/php-sdk/blob/master/src/autostart.php#L12 only check for class-existance...?

That way in old blackfire ext version one would get the noop BlackfireProbe...?

nicolas-grekas commented 7 years ago

That couldn't work because when the extension is installed, the class is always defined at bootstrap, way before we have any chance to load the polyfill, isn't it?

staabm commented 7 years ago

Maybe I dont know enough about the ext internals, but when the class is provided via extension then it shouldn't be autoloaded from the sdk and the builtin class should work fine?

When the extension is available but too old and doesnt containt the class the noop class would be loaded

When no extension is available the noop class should be used

nicolas-grekas commented 7 years ago

The extension has the class since the beginning, but the addMarker method is new since 1.15.0, that's why it's that annoying and we can't do much, except adding this conflict rule or method_exists check.

staabm commented 7 years ago

Ohh I see.