Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Drop PHP 7.4 support #1244

Merged
merged 15 commits into from
Mar 4, 2024
Merged

Drop PHP 7.4 support #1244

merged 15 commits into from
Mar 4, 2024

Conversation

brettmc
Copy link
Collaborator

@brettmc brettmc commented Feb 29, 2024 •
edited
Loading

  • update composer.json minimums, and run rector over the code to update what it can. changed some static analysis config to fix some rector-induced failures.
  • adding some union type-hints which rector didn't, and remove the corresponding @param phpdoc
  • update typehint for metric aggregation, and note a todo for 8.1+ to convert to an enum

To do:

  • fix TypeError: WeakMap key must be an object test failures with batch exporter self-diagnostics
  • remove 7.4 from opentelemetry-php/gh-workflows/.github/workflows/validate-packages.yml

Fixes: #1243

@brettmc
drop php 7.4 support
8342aba 
update composer.json minimums, and run rector over the code to update what it can. changed some static analysis
config to fix some rector-induced failures.
@brettmc
adding union typehints
71dc03a
@codecov Codecov
Copy link

codecov bot commented Feb 29, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 81.91126% with 53 lines in your changes are missing coverage. Please review.

Project coverage is 84.62%. Comparing base (cc56628) to head (22a20e1).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1244      +/-   ##
============================================
+ Coverage     83.08%   84.62%   +1.53%     
+ Complexity     2274     2136     -138     
============================================
  Files           285      284       -1     
  Lines          6456     6054     -402     
============================================
- Hits           5364     5123     -241     
+ Misses         1092      931     -161
Flag Coverage Δ
7.4 ?
8.0 84.57% <81.91%> (+1.58%) ⬆️
8.1 84.60% <81.91%> (+1.45%) ⬆️
8.2 84.60% <81.91%> (+1.45%) ⬆️
8.3 84.60% <81.91%> (+1.45%) ⬆️
8.4 84.60% <81.91%> (+1.45%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/API/Baggage/Baggage.php 82.14% <100.00%> (-0.62%) ⬇️
src/API/Baggage/Propagation/Parser.php 100.00% <100.00%> (ø)
src/API/Behavior/Internal/LogWriterFactory.php 78.94% <ø> (ø)
src/API/Globals.php 93.54% <ø> (-0.74%) ⬇️
src/API/Instrumentation/CachedInstrumentation.php 100.00% <100.00%> (ø)
src/API/Instrumentation/InstrumentationTrait.php 95.23% <ø> (ø)
src/API/Logs/EventLogger.php 100.00% <100.00%> (ø)
src/API/Logs/LogRecord.php 100.00% <100.00%> (ø)
src/API/Logs/Map/Psr3.php 100.00% <100.00%> (ø)
src/API/Metrics/Noop/NoopMeterProvider.php 0.00% <ø> (ø)
... and 160 more

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc56628...22a20e1. Read the comment docs.

@brettmc
Copy link
Collaborator Author

brettmc commented Feb 29, 2024 •
edited
Loading

The WeakMap issue I was hitting is actually a bug fixed in 8.0.22, 8.1.9 and 8.2.0 - found and fixed by otel's own Nevay. I just hadn't updated my 8.0 build environment for a really long time.

@brettmc brettmc marked this pull request as ready for review March 1, 2024 04:38
@brettmc brettmc requested a review from a team March 1, 2024 04:38
agoallikmaa
agoallikmaa approved these changes Mar 1, 2024
View reviewed changes
Copy link

@agoallikmaa agoallikmaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose symfony/polyfill-php80 dependency can also be removed now?

Nevay
Nevay reviewed Mar 1, 2024
View reviewed changes
Copy link
Contributor

@Nevay Nevay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more methods where union types can be added:

  • SpanBuilderInterface::setParent() and ::setAttribute()
  • MeterInterface $advisory parameter
  • ObserverInterface::observe()
  • LogRecord::setAttribute()

We can use WeakMap directly now and can remove:

  • OpenTelemetry\API\Instrumentation\CachedInstrumentation::createWeakMap()
  • OpenTelemetry\SDK\Common\Util\WeakMap

src/API/Baggage/Entry.php Outdated
$value,
MetadataInterface $metadata
private mixed $value,
private MetadataInterface $metadata
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private MetadataInterface $metadata
private MetadataInterface $metadata,

Should add trailing comma to changed constructors to reduce diff if we add more parameters.

Copy link
Collaborator Author

@brettmc brettmc Mar 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a php-cs-fixer rule to apply this to all multiline function declarations

src/Context/Propagation/MultiTextMapPropagator.php Outdated Show resolved Hide resolved
src/Contrib/Otlp/ProtobufSerializer.php Outdated Show resolved Hide resolved
ChrisLightfootWild
ChrisLightfootWild approved these changes Mar 3, 2024
View reviewed changes
Copy link
Contributor

@ChrisLightfootWild ChrisLightfootWild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 🧹 💪

src/Context/ContextKey.php Outdated Show resolved Hide resolved
@brettmc brettmc added this to the 1.1.x milestone Mar 4, 2024
@bobstrecansky bobstrecansky merged commit f798bb6 into open-telemetry:main Mar 4, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nevay Nevay Nevay left review comments

@ChrisLightfootWild ChrisLightfootWild ChrisLightfootWild approved these changes

@bobstrecansky bobstrecansky bobstrecansky approved these changes

@agoallikmaa agoallikmaa agoallikmaa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.

drop PHP 7.4 support
5 participants
@brettmc @agoallikmaa @bobstrecansky @Nevay @ChrisLightfootWild