-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
[#9] Placeholder service provider #30
base: master
Are you sure you want to change the base?
[#9] Placeholder service provider #30
Conversation
Marking as draft as code is untested |
Thank you for your PR. Even though it's a duplicate it's quite interesting to see a different approach. You seem to be missing a way for another plugin to resolve/replace placeholders though? (You don't need to add that if you want to do something else now that you noticed there's a PR for it already. Depends on if @eyvallahabi wants to continue their PR I guess) But I like the way placeholders can be registered by other plugins in comparison to the other PR, I just feel that it might be more complex than necessary as it basically just mirrors what PAPI does. (I don't really see the usefulness of the author, version and plugin being exposed in the API. Imo. that's an implementation detail of the service provider so a simple @eyvallahabi maybe you can find a similar but simpler approach for your PR? |
Thanks for taking the time to reply and look over. You're totally right, I forgot to implement an equivalent of PAPI's My thinking behind copying over the author/version/plugin was that then a server owner or dev or whatever could then trace a placeholder through their provider (PAPI/whomever) to the original, rather than it being traced back to Tresor. The callback approach you mention probably be the most plugin agnostic approach, would be happy to adopt that too. If the other PR user doesn't come back in the next week or so I'll look at picking this back up, don't want to hijack something they're half-way through. |
Hi,
#9 - Apologies I didn't see another PR had been raised for this already & don't intend to create a 'race' of any sorts - I'm not looking for the IssueHunt.
I've implemented a very rough approach and just implemented an abstraction layer directly for PlaceholderApi and maintained PlaceholderApi's approach of leaving the translation of 'params' completely up to the external implementation, rather than looking to create a mapping within Tresor, though I appreciate this may not be what was in mind?
The end-goal in my head is that when adding other Providers in the future, a translation can be created between PAPI's format and their format.
Though I'm not aware of other providers / Placeholder plugins, I had a brief look at MVDW but it appeared that it's exclusively for MVDW's premium plugins and are all available via PAPI regardless?
Let me know, I'm quite keen to contribute to some open source repos and this seemed like a good issue to pick up but if I've missed the intended mark I'm happy to adjust this or look at another issue (given the other PR raised for #9 too).