Add Symfony 5 support

[piotrantosik]

HI,

I try to introduce Symfony 5 auto-detect. I prepare this code, but totally I don't have an idea how to check locally if it's working properly, because agent built in dev-shell requires libpcre.so.3 that isn't available on alpine that I use for docker container.
Also, dev-env don't work on latest dev branch, I remove python-yaml library from dockerfile as was done on GH pipelines - 2d2e5f3

This PR fix #113

[CLAassistant]

All committers have signed the CLA.

[nr-opensource-bot]

Can one of the admins verify this patch?

[zsistla]

Hi @piotrantosik , thanks so much for create the PR with this fix! We really appreciate your engagement.

Typically, when a framework version changes, unless there are systemically wide differences, we don't create a new framework detection in order to reduce duplication of code (and reduce the chance of fixes not getting propagated). The detection is an internal designation only and is seen only in internal logs and does not affect how data shows up in the UI. For instance, you see here that symfony2 is actually symfony3 as well and even though internally it says symfony2, it actually works well to detect either symfony3 or symfony2.

For symfony 5, from an instrumentation standpoint, the main difference is the need to wrap a new call for the errorlistener but the other instumentation code remains the same.

For instance, with the exception of the errorlistener, the agent is already instrumenting symfony5 calls since they are mostly duplicates of symfony4.

As an alternative to creating a new symfony5.c file with duplicate code paths, would you consider moving the new function wrapper to the symfony4.c file?
That way, if the agent sees this call `Symfony\Component\HttpKernel\"

                                "EventListener\\RouterListener::onKernelRequest

It is wrapped, but otherwise, if it sees this callSymfony\Component\HttpKernel\"
"EventListener\ErrorListener::onKernelException` it is also wrapped.

Then we could update the comment here to say also symfony 5, similar to what was done for symfony2 and 3.

And add the new function and the new wrapper to the symfony4.c file (where symfony4 really indicates symfony4+) with one update to this line.

This would simplify the changes all the other changes as everything else could go away and only symfony4.c would be required to be updated.

Again, thanks so much for addressing this fix.

[zsistla]

Apologies regarding the python-yaml issue that occurred in the prototype due to a change that happened in a base image we pull from. Our team has actually been updating the prototype to enable a wider variety of tests beyond the redis and mysqli ones it currently supports so the new change hadn't been rolled out yet.

[piotrantosik]

Thanks @zsistla for fast reply and detailed explaining! Right now I know more about how it works.
Following your advice, I have updated the PR to simplify code - basically, drop symfony5.c and add simply if condition in error wrapper.
I look forward to your feedback.

[zsistla]

ok jenkins

[piotrantosik]

build status is false negative - on my fork it's green https://github.com/piotrantosik/newrelic-php-agent/actions/runs/1360606524

[zsistla]

ok jenkins

[zsistla]

ok jenkins

[zsistla]

Thanks again for your work on this.

This might not be the best differentiator since the ExceptionEvent class was introduced in Symfony 4.3. .

What do you think about if we try something simpler like the following to cover both cases so that we handle it either way:

exception = nr_php_call(event, "getThrowable");
if (!nr_php_is_zval_valid_object(exception)) {
    exception = nr_php_call(event, "getException");
  }

That way if it is a valid throwable we get that; otherwise, we try for the exception.

[zsistla]

Thanks again for your work on this. You mention this being hard to test in your local alpine environment due to the libpcre.so.3 dependency of the agent built in the shell. Would it be possible to get a local symfony in the dev shell? Or alternatively, provide a php file in a comment here that triggers the call to ErrorListener?

[piotrantosik]

Thanks, I found hax & tricky solution but it's possible to test built extension in dev shell. I'm back with updated PR at the weekend, I also try to add a named CLI transactions.

[piotrantosik]

@zsistla I update code as You suggested, also I add a solution for proper naming transactions for CLI. I tested with shell-dev, everything works properly, agent automatically detect Symfony: debug: detected framework = 'Symfony4'

Some screens, web:

CLI:

[zsistla]

Great! Thanks so much for the updates. The ErrorListener portion is looking good, and I'll be reviewing the newly added Symfony\\Component\\Console\\Command\\Command::run portions today.

In the meantime, I'll kick off some test runs on our end.

[zsistla]

ok jenkins

[piotrantosik]

hi @zsistla, I want to check if everything from my side it's ok, all tests are passed?

[mfulb]

Thank you for the contribution @piotrantosik!