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.

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

fix: Use late static binding in AbstractProvider #123

Merged
merged 1 commit into from
May 7, 2024
Merged

fix: Use late static binding in AbstractProvider #123

merged 1 commit into from
May 7, 2024

Conversation

schodemeiss
Copy link
Contributor

This PR

Switches from self::$NAME to static::$NAME in the getMetadata method of the AbstractProvider class. This change is to ensure the name of the extending provider is used in the metadata. Previously, when extending from AbstractProvider, the metadata name was always AbstractProvider.

How to test

Create a class that extends AbstractProvider, give it a different name, then call "getMetadata" and see that the name is now the name of the extended class and not "AbstractProvider"

@schodemeiss schodemeiss requested a review from tcarrio as a code owner April 30, 2024 07:45
@tcarrio
Copy link
Member

tcarrio commented May 2, 2024

Great catch @schodemeiss 👏

I would like to have a unit test that covers this functioning as we expect. Could you include one for this?

@tcarrio
Copy link
Member

tcarrio commented May 2, 2024

For both PRs, your commits need to be signed off for the DCO.

@schodemeiss
Copy link
Contributor Author

For both PRs, your commits need to be signed off for the DCO.

I'll write tests for both. I don't know what a/the DCO is though? Apologies!

@beeme1mr
Copy link
Member

beeme1mr commented May 2, 2024

I'll write tests for both. I don't know what a/the DCO is though? Apologies!

Hey @schodemeiss, no worries! A DCO is the Developer Certificate of Origin. It's a way to signify that you're willing and able to contribute your code to the CNCF.

To add your Signed-off-by line to every commit in this branch:

  1. Ensure you have a local copy of your branch by checking out the pull request locally via command line.
  2. In your local branch, run: git rebase HEAD~1 --signoff
  3. Force push your changes to overwrite the branch: git push --force-with-lease origin fix/name-in-abstract-provider

@schodemeiss
Copy link
Contributor Author

schodemeiss commented May 3, 2024

Ok -- that test covers that case nicely, and I've force pushed adding a sign off to the commit. Hope that's what you were after. I'll do #121 next.

Switched from self::$NAME to static::$NAME in the getMetadata method of the AbstractProvider class. This change is to ensure the name of the extending provider is used in the metadata. Previously, when extending from AbstractProvider, the metadata name was always AbstractProvider.

Signed-off-by: Andrew Menino-Barlow <[email protected]>
@tcarrio
Copy link
Member

tcarrio commented May 7, 2024

That covers it. Utilizing the AbstractProvider for the test provided is 👌

Ran the full suite of Actions. If that's all set I'll approve ✅

@tcarrio
Copy link
Member

tcarrio commented May 7, 2024

LGTM! Merging @schodemeiss. Thanks for your contribution!

@tcarrio tcarrio merged commit 2123274 into open-feature:main May 7, 2024
5 checks passed
tcarrio pushed a commit that referenced this pull request May 7, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.0.6](2.0.5...2.0.6)
(2024-05-07)


### 🐛 Bug Fixes

* Improve error handling in OpenFeatureClient
([#121](#121))
([58e97d2](58e97d2))
* Use late static binding in AbstractProvider
([#123](#123))
([2123274](2123274))


### 🧹 Chore

* **deps:** update dependency psalm/plugin-phpunit to ^0.19.0
([#119](#119))
([09f9d47](09f9d47))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@schodemeiss
Copy link
Contributor Author

LGTM! Merging @schodemeiss. Thanks for your contribution!

You're welcome! Thanks for merging it in.

@schodemeiss schodemeiss deleted the fix/name-in-abstract-provider branch May 7, 2024 05:55
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.

3 participants