Skip to content

Conversation

@felixarntz
Copy link
Member

Our registry so far does not allow this, but it's a crucial requirement IMO. Code needs to be able to cycle through all registered providers, e.g. in our WordPress specific package this need is already relevant.

Copy link
Member

@JasonTheAdams JasonTheAdams left a comment

Choose a reason for hiding this comment

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

Good add! One really minor suggestion, so I'll go ahead and approve. 👍

@github-actions
Copy link

github-actions bot commented Sep 2, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: JasonTheAdams <jason_the_adams@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@felixarntz
Copy link
Member Author

@JasonTheAdams The change you made assumes this is supposed to return provider class names, but I was intending for it to return provider IDs, per the method name. So something is not right here now, one way or another.

Should we maybe add methods for both? getRegisteredProviderIds and getRegisteredProviderClassNames?

@JasonTheAdams
Copy link
Member

Oh goodness, my bad. I saw the $array_keys($this->providerClassnames) and made the assumption. 🤦‍♂️

Why not have it return the id => class structure we have internally?

@felixarntz
Copy link
Member Author

@JasonTheAdams

Why not have it return the id => class structure we have internally?

IMO too low-level. I think methods for exactly what you want (one or the other) are more intuitive.

@JasonTheAdams
Copy link
Member

@felixarntz Works for me! In that case, lets stick with what you have. The dev can use getProviderClassName to get the class for a given id.

Sorry for messing it up. 😆

@JasonTheAdams
Copy link
Member

I put it back to how you had it when I first approved. So let's just pretend I wasn't "helpful" and merge this bad boy. 😂

@JasonTheAdams JasonTheAdams merged commit 7e178f4 into trunk Sep 2, 2025
5 checks passed
@JasonTheAdams JasonTheAdams deleted the enhance/get-provider-ids branch September 2, 2025 18:04
@JasonTheAdams JasonTheAdams mentioned this pull request Sep 2, 2025
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants