opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
508 stars 56 forks source link

Ensure carrier is passed by reference in MockTracer::inject #107

Closed mszabo-wikia closed 4 years ago

mszabo-wikia commented 4 years ago

Short description of what this PR does:

The MockTracer class provided by opentracing-php allows users to provide callables implementing tracing context injection for a given format. The inject() method implementation, when called with a given context, format and carrier, looks up the user-provided callable for the given format, and invokes it via PHP's call_user_func function to perform the injection.

This implementation is broken, because call_user_func passes parameters to the target by value, rather than by reference.[1] Since the injection process relies on the carrier being passed by reference, so that the injection callable can modify the carrier, this will cause a PHP Warning and cause the injection to fail, e.g.:

Parameter 2 to OpenTracing\Tests\Mock\MockTracerTest::OpenTracing\Tests\Mock{closure}() expected to be a reference, value given

This patch changes MockTracer::inject to invoke the injector callable directly rather than via call_user_func, and updates the corresponding unit test.

Checklist

Closes #106

jcchavezs commented 4 years ago

Thanks for this @mszabo-wikia! Is it worth to enable warnings in CI so we can catch such things?

piotrooo commented 4 years ago

Is it worth to enable warnings in CI so we can catch such things?

Yes! We should be as strict as possible.

jcchavezs commented 4 years ago

@piotrooo @mszabo-wikia see https://github.com/opentracing/opentracing-php/pull/108

cawolf commented 4 years ago

Anything missing for this PR? LGTM