Problem/Motivation

When running performance tests with OpenTelemetry enabled, the Symfony debug autoloader triggers deprecation message for the possibility that PHP will add return types to ArrayAccess. These aren't triggered in regular pipelines because the actual OpenTelemetry sending code doesn't run for those - depends on an environment variable.

We can just add them to the deprecation skip list.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3475533

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch opened merge request !9541

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems straight forward.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work

One inline comment

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Replied to the comment - I think it's correct as-is.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Fair point, this is a PHP native class.

BTW I am now curious to understand from where the DebugClassloader is getting the potential typehint given it’s not in an annotation of a .php file. I will dig a bit.

mondrake’s picture

mondrake
Location 🇮🇹
commented

What is the class/method that is causing the deprecation, though? I tried removing the return types from the ArrayAccess implementation methods in Drupal\Core\Template\Attribute and run locally the Drupal\Tests\Core\Template\AttributeTest test, but it does not trigger in that case.

catch’s picture

catch
he/him
Primary language English
commented

@mondrake it's in the OpenTelemetry code base, not Drupal:

1) /builds/project/drupal/vendor/symfony/error-handler/DebugClassLoader.php:341
Method "ArrayAccess::offsetGet()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "OpenTelemetry\SDK\Common\Attribute\AttributesBuilder" now to avoid errors or add an explicit @return annotation to suppress this message.

https://git.drupalcode.org/project/drupal/-/jobs/2818033

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work

OK but then this is an indirect deprecation, and I would suggest to add the ignore a bit below in the apposite section

# Indirect deprecations. These are not in Drupal's remit to fix, but it is
# worth keeping track of dependencies' issues.
%Method "[^"]+" might add "[^"]+" as a native return type declaration in the future. Do the same in implementation "org\\bovigo\\vfs\\[^"]+" now to avoid errors or add an explicit @return annotation to suppress this message%
mondrake’s picture

mondrake
Location 🇮🇹
commented

With symfony/phpunit-bridge we could skip indirect deprecations; with our replacement not, at least as-is.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

That's a good idea, hadn't even noticed that section...

Made a commit for this, but for whatever reason I can't currently reproduce the deprecation notice running tests locally so not properly tested yet.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Test failures seem random to me.

Would be good to run the performance tests, but I cannot trigger them manually.

catch’s picture

catch
he/him
Primary language English
commented

Kicked off a run with the OTEL_COLLECTOR environment variable set, which should be close/identical to the scheduled job.

https://git.drupalcode.org/project/drupal/-/jobs/2818376

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Thanks

quietone’s picture

quietone commented

All questions have been answered. I have updated credit.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Requeued pipeline

  • larowlan committed c39eaf1a on 11.x 
    Issue #3475533 by catch, mondrake: Suppress Symfony debug classloader...

larowlan closed merge request !9541

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 11.x - does this need a backport?

smustgrave’s picture

smustgrave commented
Status: Patch (to be ported) » Fixed

With 7 months of no follow up going to say it didn’t need to be backporter?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.