Problem/Motivation

PHP 8.4 require to upgrade open-telemetry packages

Steps to reproduce

See https://github.com/open-telemetry/opentelemetry-php/issues/1296

Proposed resolution

  1. Upgrade all: composer update -W open-telemetry/* (details below).
  2. Disable the composer plugin for tbachert/spi.
  3. Add configuration for drupal/core-vendor-hardening.
  4. Deal with docs in #3478895: Document new Composer plugin tbachert/spi required by core-dev.
core$ ~/.config/composer/vendor/bin/composer-lock-diff --no-links
+----------------------------------+---------+---------+
| Dev Changes                      | From    | To      |
+----------------------------------+---------+---------+
| google/protobuf                  | v3.25.4 | v4.28.3 |
| open-telemetry/api               | 1.0.3   | 1.1.1   |
| open-telemetry/context           | 1.0.2   | 1.1.0   |
| open-telemetry/exporter-otlp     | 1.0.4   | 1.1.0   |
| open-telemetry/gen-otlp-protobuf | 1.1.0   | 1.2.1   |
| open-telemetry/sdk               | 1.0.8   | 1.1.2   |
| open-telemetry/sem-conv          | 1.27.0  | 1.27.1  |
| php-http/discovery               | 1.19.4  | 1.20.0  |
| brick/math                       | NEW     | 0.12.1  |
| nyholm/psr7-server               | NEW     | 1.1.0   |
| ramsey/collection                | NEW     | 2.0.0   |
| ramsey/uuid                      | NEW     | 4.7.6   |
| tbachert/spi                     | NEW     | v1.0.2  |
+----------------------------------+---------+---------+

We do not need to enable the composer plugin according to this comment: #3478895-19: Document new Composer plugin tbachert/spi required by core-dev.

This MR adds several packages, but only tbachert/spi includes a test/ directory when installed with composer, so that is the only one added to drupal/core-vendor-hardening config. That will be redundant if https://github.com/Nevay/spi/pull/5 is merged.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3484463-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3484463

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

andypost created an issue. See original summary.

andypost opened merge request !9999

andypost opened merge request !10000

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
Issue tags: +PHP 8.4
Parent issue: » #3427903: [META] Make Drupal 10/11 compatible with PHP 8.4
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Congrats @andypost on being the 10,000 MR.

Appears the 11.x needs a rebase.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Status: Needs work » Needs review

Thanks, rebased as mink and SF7.2 are in first

-| open-telemetry/gen-otlp-protobuf | 1.1.0  | 1.2.0  |
+| open-telemetry/gen-otlp-protobuf | 1.1.0  | 1.2.1  |
smustgrave’s picture

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

Rebase seems good.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

I notice that the MR for 10.4.x sorts the entries under allow-plugins. +1 for that.

In that section, both MRs add "tbachert/spi": false. That seems like the right thing to do based on the comment #3478895-19: Document new Composer plugin tbachert/spi required by core-dev:

Right now, opentelemetry should work just fine with that plugin disabled (it's currently only used by a new declarative configuration feature that's still being actively developed).

I ran composer update open-telemetry/* myself (using the latest 11.x) and got the same changes to the two composer.json files. +1

I also tried composer update -W open-telemetry/*. That also updated google/protobuf. I guess our practice is to update all PHP dependencies at once when preparing a new minor release, so there is no need to update that now.

+1 for RTBC

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

@benjifisher thanks! good idea! so I updated both MRs using -W and issue summary

and for 11.x

+--------------------+---------+---------+
| Dev Changes        | From    | To      |
+--------------------+---------+---------+
| google/protobuf    | v3.25.4 | v4.28.3 |
| php-http/discovery | 1.19.4  | 1.20.0  |
+--------------------+---------+---------+

for 10.4.x

+------------------------+---------+---------+
| Dev Changes            | From    | To      |
+------------------------+---------+---------+
| google/protobuf        | v3.25.3 | v4.28.3 |
| php-http/discovery     | 1.19.4  | 1.20.0  |
| symfony/polyfill-php82 | v1.29.0 | v1.31.0 |
+------------------------+---------+---------+
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Reviewed & tested by the community » Needs review

Filed https://github.com/Nevay/spi/pull/5 and updated core's vendor hardening to clean-up tests

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3484463-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

bot checking against wrong branch

smustgrave’s picture

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

Rebase seems good.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue summary: View changes

I checked that tbachert/spi is the only new package that includes tests/ when installed with composer, so that is the only one that needs an addition to drupal/core-vendor-hardening.

I checked that there are no other packages that depend on google/protobuf, so the major-version upgrade should not have any surprises:

$ ddev composer why google/protobuf
open-telemetry/gen-otlp-protobuf 1.2.1 requires google/protobuf (^3.22 || ^4.0)

That is with the feature branch (or running composer update -W open-telemetry/* myself). With 11.x, I get this, which explains the major-version upgrade:

$ ddev composer why google/protobuf
open-telemetry/gen-otlp-protobuf 1.1.0 requires google/protobuf (^3.3.0)

I added some more detail to the Proposed resolution in the issue summary.

+1 for RTBC

  • catch committed 839c6d8a on 11.1.x 
    Issue #3484463 by andypost, benjifisher: Upgrade open-telemetry packages...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks like the 10.5 MR needs a rebase.

catch closed merge request !9999

andypost opened merge request !10143

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Patch (to be ported) » Needs review

Created MR for 10.5.x using composer update -W open-telemetry/* and backporting cspell and tests clean-up

  • catch committed 288bcef1 on 10.5.x 
    Issue #3484463 by andypost, benjifisher: Upgrade open-telemetry packages...

  • catch committed 4c074e33 on 10.4.x 
    Issue #3484463 by andypost, benjifisher: Upgrade open-telemetry packages...
catch’s picture

catch
he/him
Primary language English
commented
Version: 10.5.x-dev » 10.4.x-dev
Status: Needs review » Fixed

Thanks - committed/pushed the respective MRs to 10.5.x and 10.4.x too now.

catch closed merge request !10143

catch closed merge request !10000

  • catch committed b47d7a0f on 11.x 
    Issue #3484463 by andypost, benjifisher: Upgrade open-telemetry packages...

Status: Fixed » Closed (fixed)

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