Skip to content

Add Support for PHP among Highlight SDKs#7152

Merged
Vadman97 merged 4 commits intohighlight:mainfrom
ayewo:feat/php-sdk
Dec 22, 2023
Merged

Add Support for PHP among Highlight SDKs#7152
Vadman97 merged 4 commits intohighlight:mainfrom
ayewo:feat/php-sdk

Conversation

@ayewo
Copy link
Copy Markdown
Contributor

@ayewo ayewo commented Nov 15, 2023

Summary

The PHP SDK closely follows the design of Java SDK mentioned in the original issue: #4225

How did you test this change?

Automated tests.

Are there any deployment considerations?

Requires a minimum of PHP v8.2+ as some Otel dependencies wont work with older versions like PHP v7.4.

Does this work require review from our design team?

Nope.

/claim #4225

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Nov 15, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 516e4d9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR 
Some errors occurred when validating the changesets config:
The package or glob expression "@highlight-run/rrdom" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@highlight-run/rrdom-nodejs" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@highlight-run/rrweb" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@highlight-run/rrweb-player" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@highlight-run/rrweb-snapshot" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@highlight-run/rrweb-types" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@highlight-run/rrweb-web-extension" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrvideo" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@algora-pbc algora-pbc bot mentioned this pull request Nov 15, 2023
Open
@ayewo
Copy link
Copy Markdown
Contributor Author

ayewo commented Nov 15, 2023

@Vadman97 @jay-khatri this is ready for review.

@Vadman97 Vadman97 requested review from a team, SpennyNDaJets and Vadman97 and removed request for a team November 15, 2023 22:24
Vadman97
Vadman97 reviewed Nov 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Vadman97 Vadman97 left a comment

Choose a reason for hiding this comment

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

thank you for your contribution @ayewo! looks great overall; just asking a few clarifying questions.

could you also add documentation for setting up the SDK similar to https://www.highlight.io/docs/getting-started/backend-sdk/java/other and https://www.highlight.io/docs/sdk/java?

@ayewo
Copy link
Copy Markdown
Contributor Author

ayewo commented Dec 19, 2023

@Vadman97 I’ve addressed all the feedback raised in your review.

@Vadman97
Copy link
Copy Markdown
Member

Vadman97 commented Dec 19, 2023

@ayewo could you run yarn format:all to lint the new code?

@ayewo
Copy link
Copy Markdown
Contributor Author

ayewo commented Dec 20, 2023

could you run yarn format:all to lint the new code?

@Vadman97 done.

@Vadman97
Copy link
Copy Markdown
Member

Vadman97 commented Dec 20, 2023

Thanks @ayewo ! Taking a close look again at this today. Meanwhile, do you have any ideas for how we would publish the SDK for our customers to use? Do we upload it to some artifacts repository like with java / npm / pip ? Would like to see the functional tests + the artifact release happening as part of the CI.

@Vadman97
Copy link
Copy Markdown
Member

Vadman97 commented Dec 21, 2023

ah, looks like we release tags and packagist will pick them up

@ayewo
Copy link
Copy Markdown
Contributor Author

ayewo commented Dec 21, 2023

Thanks @ayewo ! Taking a close look again at this today.

@Vadman97 Appreciate that. I would like to have this wrapped up as soon as possible.

Meanwhile, do you have any ideas for how we would publish the SDK for our customers to use? Do we upload it to some artifacts repository like with java / npm / pip ? Would like to see the functional tests + the artifact release happening as part of the CI.

With respect to the yarn CLI and the npm registry, PHP's equivalents are composer and Packagist.

So, cutting a new release for the PHP SDK will be similar to how you already do it for other languages using GH Actions.

All the concerns you've raised so far: CI, packaging/release etc are operational concerns so I think it is best to create separate issue + bounty to discuss and implement them rather than draw out the review of this PR.

Vadman97
Vadman97 approved these changes Dec 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Vadman97 Vadman97 left a comment

Choose a reason for hiding this comment

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

thank you for your contribution @ayewo 🎉

@Vadman97 Vadman97 merged commit fa0e424 into highlight:main Dec 22, 2023
@ayewo
Copy link
Copy Markdown
Contributor Author

ayewo commented Dec 23, 2023

@Vadman97 thank you 🙏🏻!

Once you have the new issue drafted for the CI and release stuff, feel free to tag me here or on Discord.

@ayewo ayewo mentioned this pull request Feb 7, 2024
Merged
@Vadman97 Vadman97 mentioned this pull request Feb 7, 2024
Closed
@matthiscock
Copy link
Copy Markdown

matthiscock commented Apr 15, 2024

ITs great that Laravel has been integrated - but cant see any docs on it
How do we go about using this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Vadman97 Vadman97 Vadman97 approved these changes

@SpennyNDaJets SpennyNDaJets Awaiting requested review from SpennyNDaJets SpennyNDaJets was automatically assigned from highlight/full-stack-devs

Assignees

No one assigned

Labels

🙋 Bounty claim

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ayewo @Vadman97 @matthiscock