Skip to content

Conversation

@JeroenVanOort
Copy link
Contributor

I would like to use this package in a project using PHP 8, so I added compatibility for it.

Copy link
Member

@KinaneD KinaneD left a comment

Choose a reason for hiding this comment

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

Hello @JeroenVanOort,
Thanks for the contribution!

I like the change to deal with spans rather than arrays. My only concern is support of the upcoming Zipkin release that is currently under development. I suggest we avoid that until v3.0 is officially released.

Other than that, it is good to go. @Mulkave anything to add?

Thanks.

"illuminate/routing": "~5.5.0|~5.6.0|~5.7.0|~5.8.0|^6.0|^7.0|^8.0",
"illuminate/support": "~5.5.0|~5.6.0|~5.7.0|~5.8.0|^6.0|^7.0|^8.0",
"openzipkin/zipkin": "~2.0.2",
"openzipkin/zipkin": "~2.0.2|3.0.x-dev",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should support a dev release of Zipkin at this stage, once it is released we can do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love to avoid it too, I just hope they'll release v3 soon.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, better keep support for stable releases only.

* @return array
*/
protected function shiftSpan(array &$spans): array
protected function shiftSpan(array &$spans): Span
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Mulkave Mulkave self-requested a review September 23, 2021 12:40
"illuminate/routing": "~5.5.0|~5.6.0|~5.7.0|~5.8.0|^6.0|^7.0|^8.0",
"illuminate/support": "~5.5.0|~5.6.0|~5.7.0|~5.8.0|^6.0|^7.0|^8.0",
"openzipkin/zipkin": "~2.0.2",
"openzipkin/zipkin": "~2.0.2|3.0.x-dev",
Copy link
Member

Choose a reason for hiding this comment

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

Agreed, better keep support for stable releases only.

@@ -0,0 +1,29 @@
FROM php:8.0-cli-alpine
Copy link
Member

Choose a reason for hiding this comment

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

interesting stuff! Thanks for adding this @JeroenVanOort

Do you use it with your editor?

@Mulkave
Copy link
Member

Mulkave commented Sep 23, 2021

@JeroenVanOort i ran ./vendor/bin/phpunit within the worspace container and got the following:

Warning: Private methods cannot be final as they are never overridden by other classes in /var/www/html/vendor/phpunit/phpunit/src/Util/Configuration.php on line 176
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

EEE
Fatal error: Declaration of Vinelab\Tracing\Drivers\Zipkin\Propagation\IlluminateHttp::get($carrier, string $key): ?string must be compatible with Zipkin\Propagation\Getter::get($carrier, $key) in /var/www/html/src/Drivers/Zipkin/Propagation/IlluminateHttp.php on line 20

Are you sure that tests are passing for you?

UPDATE it works after a composer update so never mind 😅

@jcchavezs
Copy link
Contributor

jcchavezs commented Sep 23, 2021 via email

@KinaneD KinaneD requested review from KinaneD and removed request for KinaneD September 28, 2021 13:24
Copy link
Member

@KinaneD KinaneD left a comment

Choose a reason for hiding this comment

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

Will merge and release under v2.1.0-rc a pre-release.

@KinaneD KinaneD merged commit 798615e into Vinelab:master Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants